fix: Set resource version for CRs #1823

rexagod · 2024-10-08T19:14:10Z

Set resource version for all monitoring.go GVRs. Earlier, this was not done by the machinery responsible for it, which caused manifest applications that had no resource version present to encounter the following error:

metadata.resourceVersion: Invalid value: 0x0: must be specified for an update

This patch addresses that regression, which was introduced originally in #1575.

EDIT: I believe this is not a regression since 4.17 did not include #1575.

deads2k · 2024-10-09T12:35:29Z

test/e2e-monitoring/monitoring_test.go

+	"k8s.io/client-go/dynamic"
+)
+
+func TestResourceVersionApplication(t *testing.T) {


when were and how does this run?

@rexagod I added the CI wiring in openshift/release#57664, if you add something similar to the test-e2e-encryption rule in the Makefile https://github.com/openshift/library-go/blob/master/Makefile#L26-L27, the test will be run in CI

dgrisonnet · 2024-10-09T14:13:56Z

The code change looks good to me, pending the resolution of #1823 (comment)

deads2k · 2024-10-09T19:05:08Z

/approve
/assign @p0lyn0mial

for lgtm

pkg/operator/resource/resourceapply/monitoring.go

rexagod · 2024-10-10T10:42:06Z

pkg/operator/resource/resourceapply/monitoring.go

+	existingCopy.Object["metadata"], err = runtime.DefaultUnstructuredConverter.ToUnstructured(&existingObjectMetaTyped)
+	if err != nil {
+		return nil, false, err
+	}


Note that this is a breaking change, as earlier the method was only used to detect if the metadata fields differed, but not actually mutate them (which was intended).

However, in line with @p0lyn0mial's comment above, introducing this means a more consistent behavior (same as what we do for spec), while also offloading the merging behavior here to ensure the same practices OpenShift-wide when updating labels (merging instead of replacing).

Previously the metadata from the required obj were merged to the existing obj but the required obj was stored in the database. I thin this behaviour was unexpected, am I right ?

Yes, name and namespace fields were replaced, while the others were merged, however, instead of using the merged fields, required ones were given precedence. I didn't modify this behavior in my other PR since this was legacy, albeit less preferable, which is why we had to perform the "merge" on our end without breaking things.

pkg/operator/resource/resourceapply/monitoring.go

p0lyn0mial · 2024-10-11T06:50:08Z

@rexagod In general, I have a couple of suggestions for you to consider:

It seems that ApplyUnstructuredResourceImproved method could be made unexported/private. Do you agree?
The calling functions of the applyUnstructuredResourceImproved pass nil for defaultingFunc and equalityChecker, so I would like to remove these arguments from applyUnstructuredResourceImproved.
I would like to remove the Fail-fast if the resource versions differ code, since we merge into the existing object, not the required one.
I would like to remove an extra copy from the ensureGenericsSpec method.
I would like to create a new method, EnsureObjectMetaForUnstructured, where EnsureObjectMeta is currently located.
I would like to move the code that merges metadata into this new function (code from line 105 to line 144).

What do you think?

rexagod · 2024-10-11T09:14:10Z

It seems that ApplyUnstructuredResourceImproved method could be made unexported/private. Do you agree?

The motivation to move to an unstructured.Unstructured approach was to accommodate for resources that do not have their own Apply<Resource> methods exposed. The ones in the library right now are only there for the sake of backwards-compatibility.

The calling functions of the applyUnstructuredResourceImproved pass nil for defaultingFunc and equalityChecker, so I would like to remove these arguments from applyUnstructuredResourceImproved.

To refer to my same reasoning as above, those are nil for the exposed resource-dedicated methods as that was the original behavior. However, when using ApplyUnstructuredResourceImproved for either (a) unknown resources (ones that do not have a dedicated method here), or (b) more control over the caching mechanism, those are still needed to leverage what the machinery has to offer. For instance, the exposed methods do not use a cache as well, to preserve older behavior, but nonetheless, something the user may still need.

I would like to remove the Fail-fast if the resource versions differ code, since we merge into the existing object, not the required one.

Done.

I would like to remove an extra copy from the ensureGenericsSpec method.

Done.

I would like to create a new method, EnsureObjectMetaForUnstructured, where EnsureObjectMeta is currently located. I would like to move the code that merges metadata into this new function (code from line 105 to line 144).

Done.

rexagod · 2024-10-11T09:28:45Z

Thank you for the thorough review, @p0lyn0mial. I'd like to point out that I've refactored monitoring.go code to make it more consistent and readable (for instance, the ensureGenericSpec signature changes), in addition to the much-needed refactors you pointed out owing to the older code smells that existed pre-#1575. PLMK if I missed something.

p0lyn0mial · 2024-10-11T09:35:54Z

pkg/operator/resource/resourceapply/monitoring.go

@@ -88,7 +84,8 @@ func ApplyUnstructuredResourceImproved(
 	}
 	existing, err := client.Resource(resourceGVR).Namespace(namespace).Get(ctx, name, metav1.GetOptions{})
 	if errors.IsNotFound(err) {
-		want, err := client.Resource(resourceGVR).Namespace(namespace).Create(ctx, required, metav1.CreateOptions{})
+		var want *unstructured.Unstructured


nit: since we are returning from this branch anyway I would remove this variable and just simply want, err := client

The err here was shadowed, but I've renamed it to avoid ambiguity.

p0lyn0mial · 2024-10-11T09:36:40Z

pkg/operator/resource/resourceapply/monitoring.go

-	}
-	err = runtime.DefaultUnstructuredConverter.FromUnstructured(requiredObjectMeta, &requiredObjectMetaTyped)
+	// Replace and/or merge certain metadata fields.
+	didMetadataModify := ptr.To(false)


could you change to a non-pointer variable to avoid dereferencing bellow ? (didMetadataModify := false)

p0lyn0mial · 2024-10-11T09:36:51Z

pkg/operator/resource/resourceapply/monitoring.go

 	// Deep-check the spec objects for equality, and update the cache in either case.
 	if defaultingFunc == nil {
 		defaultingFunc = noDefaulting
 	}
 	if equalityChecker == nil {
 		equalityChecker = equality.Semantic
 	}
-	existingCopy, didSpecModify, err := ensureGenericSpec(required, existingCopy, defaultingFunc, equalityChecker)
+	didSpecModify := ptr.To(false)


p0lyn0mial · 2024-10-11T09:37:46Z

pkg/operator/resource/resourceapply/monitoring.go

-	actual, err := client.Resource(resourceGVR).Namespace(namespace).Update(ctx, required, metav1.UpdateOptions{})
-	resourcehelper.ReportUpdateEvent(recorder, required, err)
-	cache.UpdateCachedResourceMetadata(required, actual)
+	var actual *unstructured.Unstructured


I would also remove it and simply do actual, err :=

p0lyn0mial · 2024-10-11T09:39:24Z

pkg/operator/resource/resourcemerge/object_merger.go

@@ -18,6 +22,47 @@ func EnsureObjectMeta(modified *bool, existing *metav1.ObjectMeta, required meta
 	MergeOwnerRefs(modified, &existing.OwnerReferences, required.OwnerReferences)
 }

+func EnsureObjectMetaForUnstructured(didMetadataModify *bool, existingCopy *unstructured.Unstructured, required *unstructured.Unstructured) error {


nit: for consistency with the EnsureObjectMeta could you rename the params to modified , existing , required?

p0lyn0mial · 2024-10-16T11:08:11Z

let's try with #1836 - i have run it on my local machine:

❯ make test-e2e-monitoring
go test -mod=vendor -race ./test/e2e-monitoring/...
ok  	github.com/openshift/library-go/test/e2e-monitoring	(cached)

i'm waiting for the ci job to finish, if it works then we need something similar for the e2e-aws-encryption.

dgrisonnet · 2024-10-16T13:29:39Z

/test e2e-aws-encryption (and local invocation) returns a make: Nothing to be done for 'test-e2e-encryption' at the moment

Oh I see, I missed that. If you add the following line, it should fix it:

diff --git a/Makefile b/Makefile                                 
index 43276499c..b466e269a 100644
--- a/Makefile
+++ b/Makefile
@@ -24,6 +24,7 @@ verify-podnetworkconnectivitychecks:
 	$(MAKE) -C pkg/operator/connectivitycheckcontroller verify
 
 test-e2e-encryption: GO_TEST_PACKAGES :=./test/e2e-encryption/...
+test-e2e-encryption: test-unit
 .PHONY: test-e2e-encryption
 
 test-e2e-monitoring

It must have been missed in the past

dgrisonnet · 2024-10-16T13:31:03Z

Hmm actually it reminded me of something and it looks like I had a PR for it that never merged: #1470

p0lyn0mial · 2024-10-16T13:37:29Z

@rexagod #1836 worked, we have two options we can merge it or you can pull the changes to your pr.

rexagod · 2024-10-16T15:53:53Z

i'm waiting for the ci job to finish, if it works then we need something similar for the e2e-aws-encryption.

Since my contextual test-e2e-encryption-knowledge is limited and the fact that we've identified at-least a couple of issues [1][2] around the test, I'd reckon it'd be better if one of the previous authors with some spare cycles can take a look.

Otherwise, I'll revisit this as soon as I can find some.

p0lyn0mial · 2024-10-17T06:38:50Z

Since my contextual test-e2e-encryption-knowledge is limited and the fact that we've identified at-least a couple of issues [1][2] around the test, I'd reckon it'd be better if one of the previous authors with some spare cycles can take a look.

@dgrisonnet has a pr for it - #1470

p0lyn0mial

Just one last comment, otherwise LGTM.

p0lyn0mial · 2024-10-17T06:40:40Z

test/e2e-monitoring/monitoring_test.go

+		t.Fatalf("Failed to update resource: %v", err)
+	}
+	require.False(t, didUpdate)
+


Would it make sense to GET the updated obj and compare it to an expected one?

Added, PTAL.

dgrisonnet · 2024-10-17T07:07:08Z

Makefile

@@ -26,5 +26,6 @@ verify-podnetworkconnectivitychecks:
 test-e2e-encryption: GO_TEST_PACKAGES :=./test/e2e-encryption/...
 .PHONY: test-e2e-encryption

-test-e2e-monitoring:
+test-e2e-monitoring: GO_TEST_PACKAGES :=./test/e2e-monitoring/...


I think it would be useful to add verbosity in case the test starts failing in CI in the future.
You can do that by adding:

test-e2e-encryption: GO_TEST_FLAGS += -v

I've added similar flags in addition to this, however, one that seemed quite off was the 4 hour timeout limit for the encryption test. Is that taking into account certain build overheads (which only happen in the CI)? I've added a 5 minute timeout for the monitoring test (the test itself takes less than a minute), but LMK if that should be increased to a similar amount.

p0lyn0mial · 2024-10-17T11:56:16Z

Makefile

 .PHONY: test-e2e-encryption

-test-e2e-monitoring:
+test-e2e-monitoring: GO_TEST_PACKAGES :=./test/e2e-monitoring/...
+test-e2e-encryption: GO_TEST_FLAGS += -v


rename to test-e2e-monitoring

Ah! 🤦🏼

p0lyn0mial · 2024-10-17T11:56:25Z

Makefile

-test-e2e-monitoring:
+test-e2e-monitoring: GO_TEST_PACKAGES :=./test/e2e-monitoring/...
+test-e2e-encryption: GO_TEST_FLAGS += -v
+test-e2e-encryption: GO_TEST_FLAGS += -timeout 5m


please remove this entry

p0lyn0mial · 2024-10-17T11:56:37Z

Makefile

+test-e2e-monitoring: GO_TEST_PACKAGES :=./test/e2e-monitoring/...
+test-e2e-encryption: GO_TEST_FLAGS += -v
+test-e2e-encryption: GO_TEST_FLAGS += -timeout 5m
+test-e2e-encryption: GO_TEST_FLAGS += -p 1


please remove this entry

p0lyn0mial · 2024-10-17T11:57:08Z

Makefile

@@ -24,7 +24,15 @@ verify-podnetworkconnectivitychecks:
 	$(MAKE) -C pkg/operator/connectivitycheckcontroller verify

 test-e2e-encryption: GO_TEST_PACKAGES :=./test/e2e-encryption/...
+test-e2e-encryption: GO_TEST_FLAGS += -v


let's not change flags for test-e2e-encryption in this PR.

can you remove changes to test-e2e-encryption? this part will be updated in #1470

p0lyn0mial · 2024-10-17T12:03:04Z

test/e2e-monitoring/monitoring_test.go

+	require.False(t, didUpdate)
+	existingResource, err = dynamicClient.Resource(gvr).Namespace(resource.GetNamespace()).Get(context.TODO(), resource.GetName(), metav1.GetOptions{})
+	require.NoError(t, err)
+	require.Equal(t, gotUnstructured.UnstructuredContent(), existingResource.UnstructuredContent())


does require.Equal know how to compare maps reliably ?

instead of comparing to the obj we received from the apply function shouldn't we compare to the expected one ?

expected object is the one we created at the beginning of the test (resource) which was later updated at line 90, right ? (excluding fields like ResourceVersion)

by comparing against the expected object we know there no other (unexpected) modifications were made.

does require.Equal know how to compare maps reliably ?

ok, it looks like it uses reflect.DeepEqual under the hood.

I would prefer comparing against the expected object. Now, I think we should compare both the received object and the stored object against the expected one, since callers might use the received object as well, wdyt?

I don't expect many fields to be changed by the server. We could just overwrite or clear the ones we know will be provided by the server, such as RV.

Sorry, can you clarify what the expected, received, and stored objects are in this context?

To me (and I'm probably wrong here), unstructuredResource seems the same as the expected and stored object (as it's what we originally create in the beginning, and what we store?), whereas gotUnstructured is the one we receive?

Also, I believe we are comparing the received object with the expected one here: 17a435d (#1823)? Not sure what the stored object is though (do you mean the one in the cluster?).

Pushed a patch that compares the expected (original) resource to the existing (cluster) and the received (returned by ApplyUnstructuredResourceImproved).

Set resource version for all `monitoring.go` GVRs. Earlier, this was not done by the machinery responsible for it, which caused manifest applications that had no resource version present to encounter the following error: ``` metadata.resourceVersion: Invalid value: 0x0: must be specified for an update ``` This patch addresses that regression, which was introduced originally in openshift#1575. Signed-off-by: Pranshu Srivastava <[email protected]>

rexagod · 2024-10-17T15:04:16Z

Makefile

-test-e2e-monitoring:
+test-e2e-monitoring: GO_TEST_PACKAGES :=./test/e2e-monitoring/...
+test-e2e-monitoring: GO_TEST_FLAGS += -v
+test-e2e-monitoring: GO_TEST_FLAGS += -timeout 5m


https://github.com/openshift/library-go/pull/1823/files#r1804621250

in a follow-up pr i would remove the timeout (let's use the default timeout) and the -p flag.

openshift-ci · 2024-10-17T16:07:44Z

@rexagod: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

p0lyn0mial · 2024-10-17T18:55:09Z

/lgtm

@rexagod many thanks for the PR.

openshift-ci · 2024-10-17T18:56:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, p0lyn0mial, rexagod

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~~ [deads2k,p0lyn0mial]

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

openshift-ci bot requested review from bertinatto and deads2k October 8, 2024 19:15

openshift-ci bot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Oct 8, 2024

rexagod changed the title ~~regression: Set resource version for CRs~~ fix: Set resource version for CRs Oct 8, 2024

dgrisonnet mentioned this pull request Oct 8, 2024

WIP: update library-go openshift/cluster-kube-apiserver-operator#1752

Open

rexagod mentioned this pull request Oct 9, 2024

OCPBUGS-17113: Lazy updates for Prometheus openshift/cluster-monitoring-operator#2395

Open

2 tasks

deads2k reviewed Oct 9, 2024

View reviewed changes

dgrisonnet mentioned this pull request Oct 9, 2024

Add new e2e-monitoring test to library-go openshift/release#57664

Merged

openshift-ci bot assigned p0lyn0mial Oct 9, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2024

p0lyn0mial reviewed Oct 10, 2024

View reviewed changes

pkg/operator/resource/resourceapply/monitoring.go Outdated Show resolved Hide resolved

rexagod commented Oct 10, 2024

View reviewed changes

p0lyn0mial reviewed Oct 11, 2024

View reviewed changes

pkg/operator/resource/resourceapply/monitoring.go Outdated Show resolved Hide resolved

rexagod marked this pull request as draft October 11, 2024 09:03

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 11, 2024

rexagod marked this pull request as ready for review October 11, 2024 09:14

openshift-ci 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, 2024

rexagod force-pushed the regression-1575 branch from b72b0ca to 27d0dbc Compare October 11, 2024 09:14

openshift-ci bot requested review from deads2k and jsafrane October 11, 2024 09:14

p0lyn0mial reviewed Oct 11, 2024

View reviewed changes

p0lyn0mial mentioned this pull request Oct 16, 2024

NO-JIRA: add test-e2e-monitoring placeholder test #1836

Closed

rexagod force-pushed the regression-1575 branch from f7f48cc to 3cd47e3 Compare October 16, 2024 14:22

p0lyn0mial reviewed Oct 17, 2024

View reviewed changes

dgrisonnet reviewed Oct 17, 2024

View reviewed changes

rexagod force-pushed the regression-1575 branch from 3cd47e3 to 9c3a79d Compare October 17, 2024 11:47

p0lyn0mial reviewed Oct 17, 2024

View reviewed changes

rexagod force-pushed the regression-1575 branch 5 times, most recently from 8c680f3 to 9efe8f5 Compare October 17, 2024 15:01

rexagod force-pushed the regression-1575 branch from 9efe8f5 to fc123b2 Compare October 17, 2024 15:02

rexagod commented Oct 17, 2024

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2024

openshift-merge-bot bot merged commit c32b334 into openshift:master Oct 17, 2024
5 checks passed

rexagod mentioned this pull request Oct 20, 2024

chore: use default timeout and proc count #1846

Open

fix: Set resource version for CRs #1823

fix: Set resource version for CRs #1823

Conversation

rexagod commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

dgrisonnet Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

dgrisonnet commented Oct 9, 2024

deads2k commented Oct 9, 2024

rexagod Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Oct 11, 2024

rexagod commented Oct 11, 2024

rexagod commented Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

rexagod Oct 11, 2024 • edited Loading

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

p0lyn0mial commented Oct 16, 2024

dgrisonnet commented Oct 16, 2024

dgrisonnet commented Oct 16, 2024

p0lyn0mial commented Oct 16, 2024

rexagod commented Oct 16, 2024

p0lyn0mial commented Oct 17, 2024

p0lyn0mial 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

rexagod Oct 17, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial Oct 17, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 17, 2024

p0lyn0mial commented Oct 17, 2024

openshift-ci bot commented Oct 17, 2024

rexagod commented Oct 8, 2024 •

edited

Loading

dgrisonnet Oct 9, 2024 •

edited

Loading

rexagod Oct 10, 2024 •

edited

Loading

rexagod commented Oct 11, 2024 •

edited

Loading

rexagod Oct 11, 2024 •

edited

Loading

rexagod Oct 17, 2024 •

edited

Loading

p0lyn0mial Oct 17, 2024 •

edited

Loading