common-instancetypes: Ignore attempts to reconcile virt-operator owned objects from bundle #661

lyarwood · 2023-08-16T16:37:38Z

What this PR does / why we need it:

This PR introduces IgnoreObjectOwnedByVirtOperator to reconcileBuilder that as the name suggests ignores attempts to reconcile resources that are labelled as being owned by KubeVirt's virt-operator.

This is then used by the common-instancetypes operand ahead of deployment of these resources being enabled through virt-operator by kubevirt/kubevirt#10309.

Future changes will also introduce a configurable to disable common-instancetypes deployment within SSP and ultimately deprecate the functionality ahead of removal in a future release of SSP.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Release note:

NONE

openshift-ci · 2023-08-16T16:37:41Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

sonarcloud · 2023-08-16T16:41:10Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

lyarwood · 2023-08-17T07:52:03Z

internal/common/resource.go

@@ -161,6 +167,11 @@ func (r *reconcileBuilder) Options(options ReconcileOptions) ReconcileBuilder {
 	return r
 }

+func (r *reconcileBuilder) IgnoreObjectOwnedByVirtOperator() ReconcileBuilder {


FWIW if folks think we might need to ignore other types of resource in the future I can make this more generic passing a function as with the UpdateFunc and StatusFunc methods but at the moment I think it's better to be explicit.

IMHO this is too specific to be in the reconcileBuilder. Can you move the logic to the functions that reconcile InstanceTypes and Preferences? reconcileVirtualMachineClusterInstancetypesFuncs() and reconcileVirtualMachineClusterPreferencesFuncs()

There you can check if the resource is owned by virt-operator or not.

lyarwood · 2023-08-17T07:53:30Z

I'll respin with more tests later today but comments welcome in the meantime.

0xFelix · 2023-08-17T09:01:13Z

internal/common/resource.go

@@ -15,6 +15,7 @@ import (
 	rbac "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
+	kubevirtCorev1 "kubevirt.io/api/core/v1"


Import as kvirtv1?

what about kubevirtv1?

$ git grep -h "kubevirt.io/api/core/v1" | awk '{ print $1 }' | sort | uniq corev1 k6tv1 kubevirt kubevirtCorev1 kubevirt.io/api/core/v1 kubevirtv1 v1

Or just virtv1, that's what's used in kubevirt/kubevirt a lot.

0xFelix · 2023-08-17T09:12:07Z

internal/common/resource.go

+	// During an upgrade to v0.19.0 we might encounter virt-operator attempting
+	// to reconcile the same set of common-instancetype resources. If configured
+	// ignore these requests to reconcile such objects once owned by virt-operator
+	if r.options.IgnoreObjectOwnedByVirtOperator && isOwnedByVirtOperator(existing) {


Put this at the start of Reconcile instead to short-circuit the whole reconciliation?

Do we want to put objects owned by virt-operator into the VersionCache?

Moving this earlier would cause another client lookup for every object for every reconcile. We can move this but I'm not sure it's worth it.

In terms of the VersionCache yeah we can avoid this but AFAICT there's no impact of this being registered?

Alright, then let's keep it here.

Just wanted to make sure we don't create any issues by caching resources we do not own.

Moving this earlier would cause another client lookup for every object for every reconcile. We can move this but I'm not sure it's worth it.

Client lookups are cheap, because everything is cached in memory. The API client watches the cluster, and any Get() or List() function calls just read the cahce.

0xFelix · 2023-08-17T09:15:37Z

Can / should we test that SSP still reaches phase Deployed when virt-operator takes over?

lyarwood · 2023-08-17T13:43:53Z

Can / should we test that SSP still reaches phase Deployed when virt-operator takes over?

Yup that's easy enough to mock in an e2e test and would give us coverage in SSP without needing HCO or an updated virt-operator.

github-actions · 2023-10-11T14:22:16Z

/cc sradco

lyarwood · 2023-10-11T14:22:23Z

/hold cancel

0xFelix

Are you still going to add an e2e for the upgrade scenario?

0xFelix · 2023-10-12T07:52:57Z

internal/common/resource.go

@@ -391,6 +418,10 @@ func logOperation(result OperationResult, resource client.Object, logger logr.Lo
 		logger.Info(fmt.Sprintf("Deleted %s resource: %s",
 			resource.GetObjectKind().GroupVersionKind().Kind,
 			resource.GetName()))
+	case OperationResultIgnored:
+		logger.Info(fmt.Sprintf("Ignored %s resource: %s",


Suggested change

logger.Info(fmt.Sprintf("Ignored %s resource: %s",

logger.Info(fmt.Sprintf("Ignored %s resource owned by virt-operator: %s",

WDYT?

github-actions · 2023-10-23T12:48:23Z

/cc sradco

akrejcir

I think this logic can be moved to specific reconcile functions.

Have you considered removing the SSP owner annotations from the InstanceTypes and Preferences that are owned by virt-operator? Then they would no longer trigger SSP reconciliation.

akrejcir · 2023-10-23T14:20:40Z

internal/common/resource.go

@@ -161,6 +167,11 @@ func (r *reconcileBuilder) Options(options ReconcileOptions) ReconcileBuilder {
 	return r
 }

+func (r *reconcileBuilder) IgnoreObjectOwnedByVirtOperator() ReconcileBuilder {


IMHO this is too specific to be in the reconcileBuilder. Can you move the logic to the functions that reconcile InstanceTypes and Preferences? reconcileVirtualMachineClusterInstancetypesFuncs() and reconcileVirtualMachineClusterPreferencesFuncs()

There you can check if the resource is owned by virt-operator or not.

akrejcir · 2023-10-23T14:22:13Z

internal/common/resource.go

+	// During an upgrade to v0.19.0 we might encounter virt-operator attempting
+	// to reconcile the same set of common-instancetype resources. If configured
+	// ignore these requests to reconcile such objects once owned by virt-operator
+	if r.options.IgnoreObjectOwnedByVirtOperator && isOwnedByVirtOperator(existing) {


Moving this earlier would cause another client lookup for every object for every reconcile. We can move this but I'm not sure it's worth it.

Client lookups are cheap, because everything is cached in memory. The API client watches the cluster, and any Get() or List() function calls just read the cahce.

akrejcir · 2023-10-23T14:23:33Z

internal/common/resource.go

+	if obj.GetLabels() == nil {
+		return false
+	}
+	managedValue, managedOk := obj.GetLabels()[kubevirtCorev1.ManagedByLabel]


You can use constant AppKubernetesManagedByLabel, instead of the imported one kubevirtCorev1.ManagedByLabel.

github-actions · 2023-10-24T11:25:49Z

/cc sradco

0xFelix · 2023-10-24T11:26:40Z

Have you considered removing the SSP owner annotations from the InstanceTypes and Preferences that are owned by virt-operator? Then they would no longer trigger SSP reconciliation.

virt-operator reconstructs the metadata of objects during reconciliation, so this should already be the case.

internal/common/resource.go

internal/operands/common-instancetypes/reconcile.go

internal/operands/common-instancetypes/reconcile_test.go

With this change the common-instancetypes operand ignores resources that are labelled as being owned by KubeVirt's virt-operator during reconciliation. Co-authored-by: Lee Yarwood <[email protected]> Signed-off-by: Felix Matouschek <[email protected]>

sonarcloud · 2023-10-25T14:20:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

akrejcir · 2023-10-25T14:24:30Z

/lgtm
/approve

kubevirt-bot · 2023-10-25T14:24:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrejcir

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [akrejcir]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-bot requested review from codingben and ksimon1 August 16, 2023 16:37

kubevirt-bot added the size/L label Aug 16, 2023

lyarwood force-pushed the common-instancetypes-ignore-virt-operator-resources branch from e51cb67 to 441a305 Compare August 16, 2023 16:40

kubevirt-bot added size/M and removed size/L labels Aug 16, 2023

lyarwood mentioned this pull request Aug 16, 2023

Add virt-operator deployment of common-instancetypes proposal kubevirt/community#231

Closed

lyarwood commented Aug 17, 2023

View reviewed changes

0xFelix reviewed Aug 17, 2023

View reviewed changes

lyarwood force-pushed the common-instancetypes-ignore-virt-operator-resources branch from 441a305 to 02f76a1 Compare October 11, 2023 14:21

lyarwood marked this pull request as ready for review October 11, 2023 14:22

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 11, 2023

kubevirt-bot requested a review from sradco October 11, 2023 14:22

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 11, 2023

kubevirt-bot requested a review from akrejcir October 11, 2023 14:23

lyarwood mentioned this pull request Oct 11, 2023

common-instancetypes: Add DeployCommonInstancetypes featureGate #702

Merged

0xFelix reviewed Oct 12, 2023

View reviewed changes

0xFelix force-pushed the common-instancetypes-ignore-virt-operator-resources branch from 02f76a1 to 7f6d954 Compare October 23, 2023 12:48

kubevirt-bot removed the size/M label Oct 23, 2023

kubevirt-bot added the size/L label Oct 23, 2023

akrejcir reviewed Oct 23, 2023

View reviewed changes

0xFelix force-pushed the common-instancetypes-ignore-virt-operator-resources branch from 7f6d954 to 434d4c5 Compare October 24, 2023 11:25

akrejcir reviewed Oct 24, 2023

View reviewed changes

0xFelix force-pushed the common-instancetypes-ignore-virt-operator-resources branch from 434d4c5 to ed32b16 Compare October 24, 2023 13:30

0xFelix force-pushed the common-instancetypes-ignore-virt-operator-resources branch from ed32b16 to 87edf8d Compare October 25, 2023 14:19

kubevirt-bot assigned akrejcir Oct 25, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2023

kubevirt-bot merged commit 004dfe4 into kubevirt:main Oct 25, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common-instancetypes: Ignore attempts to reconcile virt-operator owned objects from bundle #661

common-instancetypes: Ignore attempts to reconcile virt-operator owned objects from bundle #661

lyarwood commented Aug 16, 2023 •

edited

Loading

openshift-ci bot commented Aug 16, 2023

sonarcloud bot commented Aug 16, 2023

lyarwood Aug 17, 2023

akrejcir Oct 23, 2023

lyarwood commented Aug 17, 2023

0xFelix Aug 17, 2023

lyarwood Aug 17, 2023

0xFelix Aug 17, 2023

0xFelix Aug 17, 2023

0xFelix Aug 17, 2023

lyarwood Aug 17, 2023

0xFelix Aug 17, 2023

akrejcir Oct 23, 2023

0xFelix commented Aug 17, 2023

lyarwood commented Aug 17, 2023

github-actions bot commented Oct 11, 2023

lyarwood commented Oct 11, 2023

0xFelix left a comment

0xFelix Oct 12, 2023

github-actions bot commented Oct 23, 2023

akrejcir left a comment

akrejcir Oct 23, 2023

akrejcir Oct 23, 2023

akrejcir Oct 23, 2023

github-actions bot commented Oct 24, 2023

0xFelix commented Oct 24, 2023

sonarcloud bot commented Oct 25, 2023

akrejcir commented Oct 25, 2023

kubevirt-bot commented Oct 25, 2023

	logger.Info(fmt.Sprintf("Ignored %s resource: %s",
	logger.Info(fmt.Sprintf("Ignored %s resource owned by virt-operator: %s",

common-instancetypes: Ignore attempts to reconcile virt-operator owned objects from bundle #661

common-instancetypes: Ignore attempts to reconcile virt-operator owned objects from bundle #661

Conversation

lyarwood commented Aug 16, 2023 • edited Loading

openshift-ci bot commented Aug 16, 2023

sonarcloud bot commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyarwood commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xFelix commented Aug 17, 2023

lyarwood commented Aug 17, 2023

github-actions bot commented Oct 11, 2023

lyarwood commented Oct 11, 2023

0xFelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 23, 2023

akrejcir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 24, 2023

0xFelix commented Oct 24, 2023

sonarcloud bot commented Oct 25, 2023

akrejcir commented Oct 25, 2023

kubevirt-bot commented Oct 25, 2023

lyarwood commented Aug 16, 2023 •

edited

Loading