Fix external resources updation when json input change #1286

aruniiird · 2021-07-29T09:35:14Z

This PR make sure that when JSON input is changed, corresponding external cluster resources also get updated.

This is on top of PR:#1286 .

openshift-ci · 2021-07-29T09:35:30Z

Hi @aruniiird. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

umangachapagain · 2021-08-03T14:22:02Z

/ok-to-test

umangachapagain · 2021-08-23T07:45:49Z

controllers/storagecluster/external_resources.go

-			r.Log.Info("Monitoring Information found. Monitoring will be enabled on the external cluster.", "StorageCluster", klog.KRef(instance.Namespace, instance.Name))
-			r.monitoringIP = monitoringIP
+			// for any other CephCluster resource, developer has to handle
+			return fmt.Errorf("CephCluster Resource named: %q, is not taken care of", d.Name)


do we really need this? IMO we can drop it.

Agreed, don't do this.

umangachapagain · 2021-08-23T07:49:54Z

controllers/storagecluster/external_resources.go

+			// nothing to be done here,
+			// as all the validation will be done in CephCluster creation
+			if d.Name == "monitoring-endpoint" {
+				continue


Why not do the validations here?
Is there a benefit in moving it to CephCluster creation block?

IIRC this PR fixes a particular bug, so we should avoid making unrelated changes (in case we need to backport and maintain). Refactors should be kept separate from bug fixes.

For the most part I agree. Specifically, the main thing I wanna see when moving code around are two commits, one for the actual logic changes and one for moving it to its new location. These two can come in any order, as makes sense for the task.

That said, while this commit is very nice to have, it seems rather destabilizing for something that I don't think is a blocker for feature freeze. If possible, move these changes to a separate PR so we can evaluate what the minimum number of changes actually is.

I have moved the functionality to a different PR.

IMO we should not be doing the verification/validation at one function and use it entirely in a different function (expecting the first function to be called as the second function is not directly calling the first function).

Let me know we really want to put the verification/validation in the previous place.

There is always a question why not use it in TWO places. But our current testing workflow prevents it.

Will create a backport manually once it is merged

nbalacha · 2021-08-24T10:49:18Z

controllers/storagecluster/external_resources.go

@@ -369,37 +386,51 @@ func (r *StorageClusterReconciler) createExternalStorageClusterResources(instanc
 }

 // createExternalStorageClusterConfigMap creates configmap for external cluster
-func (r *StorageClusterReconciler) createExternalStorageClusterConfigMap(cm *corev1.ConfigMap, found *corev1.ConfigMap, objectKey types.NamespacedName) error {
+func (r *StorageClusterReconciler) createExternalStorageClusterConfigMap(cm, found *corev1.ConfigMap, objectKey types.NamespacedName) error {


I prefer the original signature as it is easier to understand.

nbalacha · 2021-08-24T10:50:18Z

controllers/storagecluster/external_resources.go

 			return err
 		}
 	}
 	return nil
 }

 // createExternalStorageClusterSecret creates secret for external cluster
-func (r *StorageClusterReconciler) createExternalStorageClusterSecret(sec *corev1.Secret, found *corev1.Secret, objectKey types.NamespacedName) error {
+func (r *StorageClusterReconciler) createExternalStorageClusterSecret(sec, found *corev1.Secret, objectKey types.NamespacedName) error {


I prefer the original signature - it is easier to read and understand.

nbalacha · 2021-08-24T10:53:36Z

Please provide more details in the commit message - it is not clear what problem the PR is fixing.

jarrpa

Agreed on all current change requests. The implementation itself seems fairly solid, but I want to exercise some discipline in avoiding unnecessary refactoring when resolving specific critical issues.

jarrpa · 2021-08-26T19:43:01Z

controllers/storagecluster/external_resources.go

-			r.Log.Info("Monitoring Information found. Monitoring will be enabled on the external cluster.", "StorageCluster", klog.KRef(instance.Namespace, instance.Name))
-			r.monitoringIP = monitoringIP
+			// for any other CephCluster resource, developer has to handle
+			return fmt.Errorf("CephCluster Resource named: %q, is not taken care of", d.Name)


Agreed, don't do this.

jarrpa · 2021-08-26T19:49:02Z

controllers/storagecluster/external_resources.go

@@ -369,37 +386,51 @@ func (r *StorageClusterReconciler) createExternalStorageClusterResources(instanc
 }

 // createExternalStorageClusterConfigMap creates configmap for external cluster
-func (r *StorageClusterReconciler) createExternalStorageClusterConfigMap(cm *corev1.ConfigMap, found *corev1.ConfigMap, objectKey types.NamespacedName) error {
+func (r *StorageClusterReconciler) createExternalStorageClusterConfigMap(cm, found *corev1.ConfigMap, objectKey types.NamespacedName) error {


jarrpa · 2021-08-26T19:49:17Z

controllers/storagecluster/external_resources.go

 			return err
 		}
 	}
 	return nil
 }

 // createExternalStorageClusterSecret creates secret for external cluster
-func (r *StorageClusterReconciler) createExternalStorageClusterSecret(sec *corev1.Secret, found *corev1.Secret, objectKey types.NamespacedName) error {
+func (r *StorageClusterReconciler) createExternalStorageClusterSecret(sec, found *corev1.Secret, objectKey types.NamespacedName) error {


jarrpa · 2021-08-26T19:51:51Z

controllers/storagecluster/external_resources.go

+			// nothing to be done here,
+			// as all the validation will be done in CephCluster creation
+			if d.Name == "monitoring-endpoint" {
+				continue


For the most part I agree. Specifically, the main thing I wanna see when moving code around are two commits, one for the actual logic changes and one for moving it to its new location. These two can come in any order, as makes sense for the task.

That said, while this commit is very nice to have, it seems rather destabilizing for something that I don't think is a blocker for feature freeze. If possible, move these changes to a separate PR so we can evaluate what the minimum number of changes actually is.

umangachapagain

@aruniiird
This PR does a bug fix and refactors.
I suggest we separate refactor into a different PR.
Keep this PR limited to the fix and include changes from #1285 as a separate commit if it makes sense.

#1286 (comment)

…unction Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

Previously external cluster's monitoring endpoints were attached to Reconcile object's variables and passed to other parts of the code. Now, we are retrieving the Monitoring endpoint details, directly from the external cluster secret, while creating the CephCluster and do all the verifications. Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

When the JSON input changes, OCS-Operator should update the external cluster resources. Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

jarrpa

/lgtm

jarrpa · 2021-08-30T16:14:19Z

controllers/storagecluster/external_resources.go

+			// nothing to be done here,
+			// as all the validation will be done in CephCluster creation
+			if d.Name == "monitoring-endpoint" {
+				continue


jarrpa · 2021-08-30T16:16:53Z

/test ocs-operator-ci

jarrpa · 2021-08-30T16:22:59Z

/retest

openshift-bot · 2021-08-30T16:29:57Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-08-30T16:42:52Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

jarrpa · 2021-08-30T16:57:51Z

/retest-required cancel

Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>

jarrpa · 2021-08-30T17:41:31Z

The ocs-operator-ci was reliably failing on the KMS unit tests because of that one persistent flake with being unable to connect to localhost. I added some commits that hopefully resolve that once and for all.

Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>

jarrpa

/lgtm

openshift-ci · 2021-08-30T18:13:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jarrpa

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

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

aruniiird · 2021-08-30T18:29:42Z

/retest

aruniiird · 2021-08-30T19:09:29Z

Created a release 4.8 backport PR: #1319

jarrpa · 2021-08-30T20:49:31Z

/test ocs-operator-ci

openshift-bot · 2021-08-30T21:28:52Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

jarrpa · 2021-08-30T22:14:21Z

e2e tests passed, but retest-required is messing things up.... overriding the results.

/override ci/prow/ocs-operator-bundle-e2e-aws

openshift-ci · 2021-08-30T22:15:46Z

@jarrpa: Overrode contexts on behalf of jarrpa: ci/prow/ocs-operator-bundle-e2e-aws

In response to this:

e2e tests passed, but retest-required is messing things up.... overriding the results.

/override ci/prow/ocs-operator-bundle-e2e-aws

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/test-infra repository.

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 Jul 29, 2021

openshift-ci bot requested review from davidvossel and jarrpa July 29, 2021 09:35

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 29, 2021

aruniiird changed the title ~~[WIP] Fix external resources updation when json input change~~ Fix external resources updation when json input change Jul 30, 2021

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 Jul 30, 2021

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 3, 2021

umangachapagain reviewed Aug 23, 2021

View reviewed changes

umangachapagain mentioned this pull request Aug 23, 2021

Changing the way external endpoint IP/Port being propagated #1285

Closed

nbalacha reviewed Aug 24, 2021

View reviewed changes

umangachapagain modified the milestone: OCS 4.9 Aug 25, 2021

jarrpa added mvp Required for the next minimum viable product. priority/1-high labels Aug 26, 2021

jarrpa requested changes Aug 26, 2021

View reviewed changes

jarrpa self-assigned this Aug 26, 2021

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2021

umangachapagain requested changes Aug 27, 2021

View reviewed changes

aruniiird force-pushed the fix-external-resources-updation-when-JSON-input-change branch from a42cf4c to 13087d5 Compare August 30, 2021 10:51

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2021

aruniiird force-pushed the fix-external-resources-updation-when-JSON-input-change branch from 13087d5 to efc5181 Compare August 30, 2021 13:42

Moving the verification of the 'monitoring-endpoints' to a separate f…

733fa93

…unction Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

aruniiird force-pushed the fix-external-resources-updation-when-JSON-input-change branch from efc5181 to bc3d9bd Compare August 30, 2021 14:08

Fix the External cluster resources updation

d65b4a5

When the JSON input changes, OCS-Operator should update the external cluster resources. Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

aruniiird force-pushed the fix-external-resources-updation-when-JSON-input-change branch from bc3d9bd to d65b4a5 Compare August 30, 2021 14:55

jarrpa approved these changes Aug 30, 2021

View reviewed changes

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 30, 2021

unit-tests: use t.Run() in TestKMSConfigChanges()

e56643e

Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2021

unit-tests: receive on mock KMS server done channel

cc07d5e

Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>

jarrpa force-pushed the fix-external-resources-updation-when-JSON-input-change branch from 718ffc8 to cc07d5e Compare August 30, 2021 17:57

jarrpa approved these changes Aug 30, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2021

openshift-merge-robot merged commit 48352f5 into red-hat-storage:master Aug 30, 2021

Fix external resources updation when json input change #1286

Fix external resources updation when json input change #1286

Conversation

aruniiird commented Jul 29, 2021 • edited by jarrpa

openshift-ci bot commented Jul 29, 2021

umangachapagain commented Aug 3, 2021

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

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

nbalacha commented Aug 24, 2021

jarrpa 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

Choose a reason for hiding this comment

umangachapagain left a comment • edited

Choose a reason for hiding this comment

jarrpa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarrpa commented Aug 30, 2021

jarrpa commented Aug 30, 2021

openshift-bot commented Aug 30, 2021

openshift-bot commented Aug 30, 2021

jarrpa commented Aug 30, 2021

jarrpa commented Aug 30, 2021

jarrpa left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 30, 2021

aruniiird commented Aug 30, 2021

aruniiird commented Aug 30, 2021

jarrpa commented Aug 30, 2021

openshift-bot commented Aug 30, 2021

jarrpa commented Aug 30, 2021

openshift-ci bot commented Aug 30, 2021

aruniiird commented Jul 29, 2021 •

edited by jarrpa

umangachapagain left a comment •

edited