controllers: trigger reconcile event to storage client if hash is not matched #217

rchikatw · 2024-08-29T07:39:31Z

Save hash to storage client status
add an annotation for the subscription channel to the storage client
Add a new field to the status of the storage client
Testing is pending will add results once it's done
I have messed PR (controllers: trigger reconcile event to storage client if hash of desired config does not match #200) which has similar changes and due to which its closed automatically hence creating the current pr.

service/status-report/main.go

internal/controller/storageclient_controller.go

leelavg · 2024-09-10T04:59:16Z

LGTM but will wait for Ohad's approval.

nb-ohad · 2024-09-10T18:17:41Z

/hold

@rchikatw Something here seems to be in misalignment with the design.
I can see 3 different values taking into consideration

The annotation
The value on the status
The value coming from the provider

One of them is unnecessary and redundant, I will vote out the status value.

rchikatw · 2024-09-11T07:23:25Z

/hold

@rchikatw Something here seems to be in misalignment with the design. I can see 3 different values taking into consideration

The annotation

The value on the status

The value coming from the provider

One of them is unnecessary and redundant, I will vote out the status value.

If we decided to remove the status section from storage client then there is no point in keeping the root field at the storage config response here

nb-ohad · 2024-09-11T12:25:24Z

pkg/utils/k8sutils.go

@@ -41,6 +41,9 @@ const StatusReporterImageEnvVar = "STATUS_REPORTER_IMAGE"
 // Value corresponding to annotation key has subscription channel
 const DesiredSubscriptionChannelAnnotationKey = "ocs.openshift.io/subscription.channel"

+// Value corresponding to annotation key has desired client hash
+const DesiredConfigHashAnnotationKey = "ocs.openshift.io/desired.config.hash"


This is the name of an annotation on the CR so by default it represents the desired state. There is no reason to mention desired in the name of the annotation.

The encoding of the data hash might change in the future, I don't think we need to mention that as part of the name as it is an impl detail.

The annotation name does not describe what kind of function this annotation represents

how about changing it to consumer.config.state

What about ocs.openshift.io/provider-side-state? or something like that?

just ocs.openshift.io/provider-state is good enough i guess

provider-state implies this represents the entire provider state which is wrong.
provider-side-state implies this represent some state on the provider side

nb-ohad · 2024-09-11T12:28:16Z

service/status-report/main.go

@@ -119,7 +119,8 @@ func main() {

 	storageClientCopy := &v1alpha1.StorageClient{}
 	storageClient.DeepCopyInto(storageClientCopy)
-	if utils.AddAnnotation(storageClient, utils.DesiredSubscriptionChannelAnnotationKey, statusResponse.DesiredClientOperatorChannel) {
+	if utils.AddAnnotation(storageClient, utils.DesiredSubscriptionChannelAnnotationKey, statusResponse.DesiredClientOperatorChannel) ||
+		utils.AddAnnotation(storageClient, utils.DesiredConfigHashAnnotationKey, statusResponse.DesiredConfigHash) {
 		// patch is being used here as to not have any conflicts over storageclient cr changes as this annotation value doesn't depend on storageclient spec
 		if err := cl.Patch(ctx, storageClient, client.MergeFrom(storageClientCopy)); err != nil {


Are we sure that patch is the correct approach for this new annotation? What happens if a reconcile is in motion while you are trying to read and update the annotation. It might be that be the time you get to the patch command the CR already changed

I'm not sure which method I should use in this case. If I use "update" on CR, it sends the entire object to the API server, while "patch" sends only the changes that need to be made. There is a greater risk of conflict when using "update" and a lesser risk when using "patch," but in both cases, we can still encounter issues.
Even if I use PartialMetadata there will be same issue I feel

We want the conflict because we do not want to update based on stale data. Let's use update instead of paths for this new annotation. The old annotation should continue to use patch

I will use .Update because it will consider the resource version while updating the resource.

nb-ohad · 2024-09-11T12:29:09Z

@rchikatw Why do the changes in this PR require changes in hundreds of vendor files?

leelavg · 2024-09-16T08:54:04Z

@rchikatw Why do the changes in this PR require changes in hundreds of vendor files?

should be solved by provider: split protobuf related pkgs into a separate go module ocs-operator#2798

rchikatw · 2024-09-16T08:57:11Z

@rchikatw Why do the changes in this PR require changes in hundreds of vendor files?

should be solved by provider: split provider related pkgs into 3 go modules ocs-operator#2798

Sure once this is merged I will rebase it.

if hash is not matched Signed-off-by: rchikatw <[email protected]>

leelavg · 2024-09-17T10:59:08Z

@rchikatw new proto changes aren't being pulled?

rchikatw · 2024-09-17T11:31:31Z

/retest

Signed-off-by: rchikatw <[email protected]>

rchikatw · 2024-09-24T10:13:10Z

@rchikatw new proto changes aren't being pulled?

It's pulled now. It took some time to push that change; meanwhile, you saw my PR.

openshift-ci · 2024-09-24T14:27:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leelavg, rchikatw

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~~ [leelavg]

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

leelavg · 2024-09-24T15:15:52Z

/unhold

From Ohad

The PR is so big I am unable to access the git without my tab crashing

@lgangava Can you please remove the hold on my behalf?

rchikatw · 2024-09-25T05:25:02Z

/cherry-pick release-4.17

openshift-cherrypick-robot · 2024-09-25T05:25:38Z

@rchikatw: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked.

Error message

status code 406 not one of [200], body: {"message":"Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead.","errors":[{"resource":"PullRequest","field":"diff","code":"too_large"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#list-pull-requests-files","status":"406"}

In response to this:

/cherry-pick release-4.17

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.

rchikatw commented Aug 29, 2024

View reviewed changes

service/status-report/main.go Show resolved Hide resolved

openshift-merge-robot added the needs-rebase label Sep 3, 2024

rchikatw force-pushed the readconfighash branch from a46f21f to f18a18e Compare September 9, 2024 09:47

openshift-merge-robot removed the needs-rebase label Sep 9, 2024

rchikatw force-pushed the readconfighash branch 3 times, most recently from f75007b to 4247517 Compare September 9, 2024 11:34

leelavg reviewed Sep 9, 2024

View reviewed changes

service/status-report/main.go Outdated Show resolved Hide resolved

internal/controller/storageclient_controller.go Outdated Show resolved Hide resolved

rchikatw force-pushed the readconfighash branch from 4247517 to f6b8bb8 Compare September 10, 2024 04:36

openshift-ci bot added the do-not-merge/hold label Sep 10, 2024

rchikatw force-pushed the readconfighash branch from f6b8bb8 to 3cb78a7 Compare September 11, 2024 12:09

nb-ohad requested changes Sep 11, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad Sep 11, 2024

rchikatw force-pushed the readconfighash branch from 3cb78a7 to 75cfdc2 Compare September 17, 2024 06:52

pull-request-size bot added the size/L label Sep 17, 2024

rchikatw force-pushed the readconfighash branch from 75cfdc2 to d472312 Compare September 17, 2024 07:35

pull-request-size bot added size/S and removed size/L labels Sep 17, 2024

rchikatw force-pushed the readconfighash branch from d472312 to 6791922 Compare September 17, 2024 08:38

openshift-merge-robot added the needs-rebase label Sep 17, 2024

rchikatw force-pushed the readconfighash branch from 6791922 to d4a4cfc Compare September 17, 2024 08:41

openshift-merge-robot removed the needs-rebase label Sep 17, 2024

rchikatw force-pushed the readconfighash branch from d4a4cfc to 0e5bdc0 Compare September 17, 2024 08:42

controllers: trigger reconcile event to storage client

2d459aa

if hash is not matched Signed-off-by: rchikatw <[email protected]>

rchikatw force-pushed the readconfighash branch from 0e5bdc0 to 2d459aa Compare September 17, 2024 09:05

rchikatw force-pushed the readconfighash branch from 4fe8766 to 2d459aa Compare September 17, 2024 17:11

rchikatw added 2 commits September 18, 2024 13:49

vendor: update vendor changes

11d177b

Signed-off-by: rchikatw <[email protected]>

bundle: updating bundle changes

f0b8dd7

Signed-off-by: rchikatw <[email protected]>

leelavg approved these changes Sep 24, 2024

View reviewed changes

openshift-ci bot assigned leelavg Sep 24, 2024

openshift-ci bot added the lgtm label Sep 24, 2024

openshift-ci bot added the approved label Sep 24, 2024

openshift-ci bot removed the do-not-merge/hold label Sep 24, 2024

openshift-merge-bot bot merged commit 11a91c9 into red-hat-storage:main Sep 24, 2024
12 checks passed

rchikatw mentioned this pull request Sep 25, 2024

Bug 2310841: [release-4.17] controllers: trigger reconcile event to storage client if hash is not matched #244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controllers: trigger reconcile event to storage client if hash is not matched #217

controllers: trigger reconcile event to storage client if hash is not matched #217

rchikatw commented Aug 29, 2024 •

edited

Loading

leelavg commented Sep 10, 2024

nb-ohad commented Sep 10, 2024

rchikatw commented Sep 11, 2024

nb-ohad Sep 11, 2024

rchikatw Sep 11, 2024

nb-ohad Sep 17, 2024

rchikatw Sep 17, 2024

nb-ohad Sep 17, 2024

nb-ohad Sep 11, 2024

rchikatw Sep 11, 2024

nb-ohad Sep 17, 2024

rchikatw Sep 17, 2024

nb-ohad commented Sep 11, 2024

leelavg commented Sep 16, 2024

rchikatw commented Sep 16, 2024

leelavg commented Sep 17, 2024

rchikatw commented Sep 17, 2024

rchikatw commented Sep 24, 2024

openshift-ci bot commented Sep 24, 2024

leelavg commented Sep 24, 2024

rchikatw commented Sep 25, 2024

openshift-cherrypick-robot commented Sep 25, 2024

controllers: trigger reconcile event to storage client if hash is not matched #217

controllers: trigger reconcile event to storage client if hash is not matched #217

Conversation

rchikatw commented Aug 29, 2024 • edited Loading

leelavg commented Sep 10, 2024

nb-ohad commented Sep 10, 2024

rchikatw commented Sep 11, 2024

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

nb-ohad commented Sep 11, 2024

leelavg commented Sep 16, 2024

rchikatw commented Sep 16, 2024

leelavg commented Sep 17, 2024

rchikatw commented Sep 17, 2024

rchikatw commented Sep 24, 2024

openshift-ci bot commented Sep 24, 2024

leelavg commented Sep 24, 2024

rchikatw commented Sep 25, 2024

openshift-cherrypick-robot commented Sep 25, 2024

rchikatw commented Aug 29, 2024 •

edited

Loading