block creation of StorageClass until prerequisites are met #1224

umangachapagain · 2021-06-14T08:31:14Z

CephBlockPool waits for CephCluster to be ready.
Similarly, StorageClass needs to wait for CephBlockPool to be ready.

Same for CephFilesystem.

jarrpa

Unless I'm missing something, this doesn't seem to take into account that either of CephBlockPool or CephFilesystem could be disabled. Would it make more sense to extend StorageClusterConfiguration to include a "required kind" or something so that we can just do the check on a per-SC basis here? https://github.com/openshift/ocs-operator/blob/master/controllers/storagecluster/storageclasses.go#L88-L93

Also, CI failures look valid. Check make ocs-operator-ci output.

umangachapagain · 2021-06-23T04:39:01Z

Unless I'm missing something, this doesn't seem to take into account that either of CephBlockPool or CephFilesystem could be disabled. Would it make more sense to extend StorageClusterConfiguration to include a "required kind" or something so that we can just do the check on a per-SC basis here? https://github.com/openshift/ocs-operator/blob/master/controllers/storagecluster/storageclasses.go#L88-L93

Don't we always create both the StorageClasses?

I don't think a "required kind" is necessary since we know what is required.

Also, CI failures look valid. Check make ocs-operator-ci output.

Yes, they are valid and I give up on fixing it. It requires re-factoring entire unit test framework (which is actually running integration test internally).
Unless someone has a better way to fix it.

jarrpa · 2021-06-23T15:02:17Z

Unless I'm missing something, this doesn't seem to take into account that either of CephBlockPool or CephFilesystem could be disabled. Would it make more sense to extend StorageClusterConfiguration to include a "required kind" or something so that we can just do the check on a per-SC basis here? https://github.com/openshift/ocs-operator/blob/master/controllers/storagecluster/storageclasses.go#L88-L93

Don't we always create both the StorageClasses?

No. If there's no CephBlockPool created it makes no sense to create a StorageClass pointing to something that doesn't exist.

I don't think a "required kind" is necessary since we know what is required.

It would be a cleaner implementation. Rather than having an if/else reading the StorageClass name and checking for the appropriate Kind based on that, We could just read the same field for all configs. But... eh, you're right, it's not strictly necessary, so I won't block on it.

Also, CI failures look valid. Check make ocs-operator-ci output.

Yes, they are valid and I give up on fixing it. It requires re-factoring entire unit test framework (which is actually running integration test internally).
Unless someone has a better way to fix it.

The ocs-operator-ci fixes are straightforward linting problems, so please at least deal with that. :P

The e2e failures look like CI infra issues, so we just need to retest. That can wait until LGTM.

umangachapagain · 2021-06-28T13:42:09Z

Unless I'm missing something, this doesn't seem to take into account that either of CephBlockPool or CephFilesystem could be disabled. Would it make more sense to extend StorageClusterConfiguration to include a "required kind" or something so that we can just do the check on a per-SC basis here? https://github.com/openshift/ocs-operator/blob/master/controllers/storagecluster/storageclasses.go#L88-L93

Updated to check if particular StorageClass is enabled before trying to wait. But, it seems like StorageClass creation doesn't care about the setting anyway and that's a different PR.

Also, CI failures look valid. Check make ocs-operator-ci output.

Fixed the annoying uppercase/lowercase issue.

umangachapagain · 2021-06-29T05:37:06Z

/retest

jarrpa

Unless I'm missing something, this doesn't seem to take into account that either of CephBlockPool or CephFilesystem could be disabled. Would it make more sense to extend StorageClusterConfiguration to include a "required kind" or something so that we can just do the check on a per-SC basis here? https://github.com/openshift/ocs-operator/blob/master/controllers/storagecluster/storageclasses.go#L88-L93

Updated to check if particular StorageClass is enabled before trying to wait. But, it seems like StorageClass creation doesn't care about the setting anyway and that's a different PR.

Oh it absolutely does!! It's embedded into the StorageClassConfiguration: https://github.com/openshift/ocs-operator/blob/master/controllers/storagecluster/storageclasses.go#L81-L83 That said, I understand it may be difficult (and a bad idea) to try and generalize the logic of checking for valid resources. As such I recommend just moving the existing logic you have to either the new*StorageClassConfiguration() functions, or newStorageClassConfigurations(), or maybe even in createStorageClasses() itself. Don't block the creation of all StorageClasses if one fails, but do return err so we block further reconciliation of dependent resources.

Also, CI failures look valid. Check make ocs-operator-ci output.

Fixed the annoying uppercase/lowercase issue.

You know I'm like this. 😛 Apologies nonetheless. 😂

controllers/storagecluster/storageclasses.go

controllers/storagecluster/cephblockpools.go

jarrpa

/lgtm

jarrpa · 2021-08-26T19:32:07Z

Gonna run this one more time just to be sure....

/test ocs-operator-bundle-e2e-aws

jarrpa · 2021-08-26T19:33:38Z

/test ocs-operator-bundle-e2e-aws

jarrpa · 2021-08-26T21:15:27Z

controllers/storagecluster/storageclasses.go

+		if cephFilesystem.Status == nil {
+			return fmt.Errorf("cephFilesystem %q is not reporting status", key)
+		}
+		r.Log.Info("CephFilesystem %q is in phase %q", key, cephFilesystem.Status.Phase)


This line is causing a panic: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_ocs-operator/1224/pull-ci-openshift-ocs-operator-master-ocs-operator-bundle-e2e-aws/1430977603430256640/artifacts/ocs-operator-bundle-e2e-aws/ocs-must-gather/artifacts/ocs-must-gather/registry-build02-ci-openshift-org-ci-op-x5bhg4hh-stable-sha256-809f3f89c362cf2be3425fde0e63d24bcc8a3b9f9de6d5ec0dbdf031296f3bcf/namespaces/openshift-storage/pods/ocs-operator-5b59fc94cf-n2j4h/ocs-operator/ocs-operator/logs/current.log

Specifically:

"msg":"non-string key argument passed to logging, ignoring all later arguments", "invalid key":{"namespace":"openshift-storage","name":"test-storagecluster-cephfilesystem"}

2 months of updates and these tests still won't pass. I'm outta here.

You go with my blessing. 🙏 🙏 🙏

jarrpa · 2021-08-26T21:15:53Z

/hold

jarrpa · 2021-08-26T21:16:14Z

/lgtm cancel

jarrpa · 2021-08-27T17:12:12Z

...test failed on RPC timeout. BECAUSE OF COURSE IT DID. 😠 Anyway, code looks solid, TYVM. 🙇

/lgtm
/hold cancel

jarrpa · 2021-08-27T17:12:52Z

/test ocs-operator-bundle-e2e-aws

jarrpa · 2021-08-27T17:13:20Z

/retest-required

jarrpa · 2021-08-27T19:45:24Z

controllers/storagecluster/storageclasses.go

+			if err != nil || cephFilesystem.Status == nil || cephFilesystem.Status.Phase != cephv1.ConditionType(util.PhaseReady) {
+				r.Log.Info("Waiting for CephFilesystem to be Ready. Skip reconciling StorageClass",
+					"CephFilesystem", klog.KRef(key.Name, key.Namespace),
+					"StorageClass", klog.KRef("", sc.Name),
+				)


StorageCluster Conditions show the following:

Last Heartbeat Time: 2021-08-27T18:33:40Z Last Transition Time: 2021-08-27T18:12:08Z Message: Error while reconciling: some StorageClasses [test-storagecluster-cephfs] were skipped while waiting for pre-requisites to be met Reason: ReconcileFailed Status: False Type: ReconcileComplete

ocs-operator logs show the following:

2021-08-27T18:33:40.740590363Z {"level":"info","ts":1630089220.7405756,"logger":"controllers.StorageCluster","msg":"Waiting for CephFilesystem to be Ready. Skip reconciling StorageClass","Request.Namespace":"openshift-storage","Request.Name":"test-storagecluster","CephFilesystem":"test-storagecluster-cephfilesystem/openshift-storage","StorageClass":"test-storagecluster-cephfs"} 2021-08-27T18:33:40.756182902Z {"level":"error","ts":1630089220.7561336,"logger":"controller-runtime.manager.controller.storagecluster","msg":"Reconciler error","reconciler group":"ocs.openshift.io","reconciler kind":"StorageCluster","name":"test-storagecluster","namespace":"openshift-storage","error":"some StorageClasses [test-storagecluster-cephfs] were skipped while waiting for pre-requisites to be met","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/src/github.com/openshift/ocs-operator/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:253\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/go/src/github.com/openshift/ocs-operator/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:214"}

Looking at the if conditions you have, I checked the CephFilesystem and indeed it has a nil Status: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_ocs-operator/1224/pull-ci-openshift-ocs-operator-master-ocs-operator-bundle-e2e-aws/1431303805407858688/artifacts/ocs-operator-bundle-e2e-aws/ocs-must-gather/artifacts/ocs-must-gather/registry-build02-ci-openshift-org-ci-op-s10lq9fp-stable-sha256-2cabcf61a08025e75eba8b82626357ddb48a44bf609f7d1483242ad5d0b1f5f0/ceph/namespaces/openshift-storage/ceph.rook.io/cephfilesystems/test-storagecluster-cephfilesystem.yaml

The rook-ceph-operator logs don't show anything obvious to me, but maybe I'm missing something: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_ocs-operator/1224/pull-ci-openshift-ocs-operator-master-ocs-operator-bundle-e2e-aws/1431303805407858688/artifacts/ocs-operator-bundle-e2e-aws/ocs-must-gather/artifacts/ocs-must-gather/registry-build02-ci-openshift-org-ci-op-s10lq9fp-stable-sha256-2cabcf61a08025e75eba8b82626357ddb48a44bf609f7d1483242ad5d0b1f5f0/namespaces/openshift-storage/pods/rook-ceph-operator-8c8468d9d-mwwvh/rook-ceph-operator/rook-ceph-operator/logs/current.log

All Pods show Ready and Running: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_ocs-operator/1224/pull-ci-openshift-ocs-operator-master-ocs-operator-bundle-e2e-aws/1431303805407858688/artifacts/ocs-operator-bundle-e2e-aws/ocs-must-gather/artifacts/ocs-must-gather/registry-build02-ci-openshift-org-ci-op-s10lq9fp-stable-sha256-2cabcf61a08025e75eba8b82626357ddb48a44bf609f7d1483242ad5d0b1f5f0/namespaces/openshift-storage/oc_output/pods_-owide

So... offhand, I'm stuck. @travisn any ideas?

In a test cluster I don't see any status ever updated on the cephfilesystem either. You're just now relying on it for the first time, or perhaps there was a regression? Still looking in the code...

Ok, there was a bug such that the filesystem status was not being updated if mirroring was not enabled on the filesystem. See rook/rook#8609

openshift-bot · 2021-08-28T20:13:49Z

/retest-required

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

Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>

rearranging resourveManager instances to preserve the order in which they need to be processed. This helps in providing a visual cue about the sequence of tasks. It also reduces chances of deadlock due to tasks waiting for each other to complete. Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>

CephBlockPool and CephFilesystem updates were incorrectly handled. This commit fixes it by updating only the spec and not resetting the Status. This also helps in unit testing. Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>

StorageClass should only be created after the CephBlockPool and CephFilesystem are Ready. So, added a prereq check to skip reconciling if prereq is not met. Also, added a field to flag external storage clusters. This is required to skip prereq checks on external mode clusters. Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>

Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>

jarrpa

ONE. MORE. TIME.

/lgtm

jarrpa · 2021-08-30T18:00:32Z

/override ci/prow/ci-index-dev-master-dependencies

openshift-ci · 2021-08-30T18:00:33Z

[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

openshift-ci · 2021-08-30T18:05:59Z

@jarrpa: Overrode contexts on behalf of jarrpa: ci/prow/ci-index-dev-master-dependencies

In response to this:

/override ci/prow/ci-index-dev-master-dependencies

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.

Issue: Due to PR red-hat-storage/ocs-operator#1224, creation of ocs-storagecluster-ceph-rbd storage class is taking moretime Signed-off-by: vavuthu <vavuthu@redhat.com>

umangachapagain requested a review from jarrpa June 14, 2021 08:31

openshift-ci bot requested a review from agarwal-mudit June 14, 2021 08:31

umangachapagain requested review from travisn and leseb and removed request for agarwal-mudit June 14, 2021 08:31

umangachapagain force-pushed the storageclass-wait branch from 3222fa3 to 9a791e7 Compare June 21, 2021 14:24

jarrpa requested changes Jun 21, 2021

View reviewed changes

umangachapagain force-pushed the storageclass-wait branch 2 times, most recently from da259e7 to 6fb50fc Compare June 28, 2021 13:38

umangachapagain requested a review from jarrpa June 28, 2021 13:42

iamniting mentioned this pull request Aug 19, 2021

storagecluster: move cephCluster deletion at the end #1293

Merged

umangachapagain force-pushed the storageclass-wait branch from 6fb50fc to d8b4e05 Compare August 24, 2021 11:21

umangachapagain added this to the OCS 4.9 milestone Aug 25, 2021

jarrpa requested changes Aug 25, 2021

View reviewed changes

controllers/storagecluster/storageclasses.go Outdated Show resolved Hide resolved

controllers/storagecluster/cephblockpools.go Show resolved Hide resolved

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

jarrpa approved these changes Aug 26, 2021

View reviewed changes

openshift-ci bot assigned jarrpa Aug 26, 2021

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2021

jarrpa requested changes Aug 26, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 26, 2021

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

umangachapagain force-pushed the storageclass-wait branch from d8b4e05 to 5e41656 Compare August 27, 2021 08:25

jarrpa added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 27, 2021

jarrpa reviewed Aug 27, 2021

View reviewed changes

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

umangachapagain added 5 commits August 30, 2021 22:53

update rook to v1.7.2

8eb66c3

Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>

add vendor changes for rook v1.7.2 latest

a84b7f8

Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>

controllers: fix update logic for CBP & CFS

7525010

CephBlockPool and CephFilesystem updates were incorrectly handled. This commit fixes it by updating only the spec and not resetting the Status. This also helps in unit testing. Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>

umangachapagain force-pushed the storageclass-wait branch from 5e41656 to a5d5f7c Compare August 30, 2021 17:23

openshift-ci bot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm Indicates that a PR is ready to be merged. labels Aug 30, 2021

update CSV to latest

1775401

Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>

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 45b1cd6 into red-hat-storage:master Aug 30, 2021

umangachapagain deleted the storageclass-wait branch August 30, 2021 19:08

vavuthu mentioned this pull request Aug 31, 2021

creation of ceph rbd storageclass is taking more time red-hat-storage/ocs-ci#4787

Closed

block creation of StorageClass until prerequisites are met #1224

block creation of StorageClass until prerequisites are met #1224

umangachapagain commented Jun 14, 2021

jarrpa left a comment •

edited

umangachapagain commented Jun 23, 2021

jarrpa commented Jun 23, 2021

umangachapagain commented Jun 28, 2021

umangachapagain commented Jun 29, 2021

jarrpa left a comment •

edited

jarrpa left a comment

jarrpa commented Aug 26, 2021

jarrpa commented Aug 26, 2021

jarrpa Aug 26, 2021

umangachapagain Aug 27, 2021

jarrpa Aug 27, 2021

jarrpa commented Aug 26, 2021

jarrpa commented Aug 26, 2021

jarrpa commented Aug 27, 2021

jarrpa commented Aug 27, 2021

jarrpa commented Aug 27, 2021

jarrpa Aug 27, 2021

travisn Aug 27, 2021

travisn Aug 27, 2021

openshift-bot commented Aug 28, 2021

jarrpa left a comment

jarrpa commented Aug 30, 2021

openshift-ci bot commented Aug 30, 2021

openshift-ci bot commented Aug 30, 2021

block creation of StorageClass until prerequisites are met #1224

block creation of StorageClass until prerequisites are met #1224

Conversation

umangachapagain commented Jun 14, 2021

jarrpa left a comment • edited

Choose a reason for hiding this comment

umangachapagain commented Jun 23, 2021

jarrpa commented Jun 23, 2021

umangachapagain commented Jun 28, 2021

umangachapagain commented Jun 29, 2021

jarrpa left a comment • edited

Choose a reason for hiding this comment

jarrpa left a comment

Choose a reason for hiding this comment

jarrpa commented Aug 26, 2021

jarrpa commented Aug 26, 2021

jarrpa Aug 26, 2021

Choose a reason for hiding this comment

umangachapagain Aug 27, 2021

Choose a reason for hiding this comment

jarrpa Aug 27, 2021

Choose a reason for hiding this comment

jarrpa commented Aug 26, 2021

jarrpa commented Aug 26, 2021

jarrpa commented Aug 27, 2021

jarrpa commented Aug 27, 2021

jarrpa commented Aug 27, 2021

jarrpa Aug 27, 2021

Choose a reason for hiding this comment

travisn Aug 27, 2021

Choose a reason for hiding this comment

travisn Aug 27, 2021

Choose a reason for hiding this comment

openshift-bot commented Aug 28, 2021

jarrpa left a comment

Choose a reason for hiding this comment

jarrpa commented Aug 30, 2021

openshift-ci bot commented Aug 30, 2021

openshift-ci bot commented Aug 30, 2021

jarrpa left a comment •

edited

jarrpa left a comment •

edited