External: configure sc properties for external cluster EC RBD pool #1440

subhamkrai · 2021-12-16T08:25:20Z

Adding extra field in storageClass "dataPool" when pool is
erasure coded pool in case of RBD.

Links:

storageClass https://github.com/rook/rook/blob/master/deploy/examples/csi/rbd/storageclass-ec.yaml#L45
Json Blob in case of EC RBD pool pool: add rbd ec pool support in external cluster rook/rook#9276 (comment)

Signed-off-by: subhamkrai srai@redhat.com

subhamkrai · 2021-12-16T08:26:57Z

@leseb ^^

subhamkrai · 2021-12-16T08:28:38Z

marked as draft as I need to check how to test external cluster changes.

aruniiird · 2021-12-16T11:20:10Z

controllers/storagecluster/storageclasses.go

@@ -228,6 +228,12 @@ func newCephBlockPoolStorageClassConfiguration(initData *ocsv1.StorageCluster) S
 		disable:           managementSpec.DisableStorageClass,
 		isClusterExternal: initData.Spec.ExternalStorage.Enable,
 	}
+	if dataPool != "" {


We are checking whether the dataPool argument is empty or not, but where are we using it?
In the line#233, we are directly creating a value (using the function generateNameForEcCephBlockPool()) for the dataPool key.

~~That's a good point. Where is dataPool used? With EC, the SC config looks like this:~~

~~https://github.com/rook/rook/blob/fffb86295600196389de81905c3402f9aec6e751/deploy/examples/csi/rbd/storageclass-ec.yaml#L45-L46~~

EDIT: actually the code is a bit confusing but this is where parameters are set:

ocs-operator/controllers/storagecluster/external_resources.go

Lines 332 to 334 in d64079d

for k, v := range d.Data {

scc.storageClass.Parameters[k] = v

}

So dataPool and pool will be populated according to the JSON blob content.

agarwal-mudit · 2021-12-16T11:22:54Z

/cc @aruniiird

openshift-ci · 2021-12-16T11:22:55Z

@agarwal-mudit: GitHub didn't allow me to request PR reviews from the following users: aruniiird.

Note that only red-hat-storage members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @aruniiird

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.

controllers/storagecluster/external_resources_test.go

controllers/storagecluster/external_resources.go

leseb · 2021-12-21T09:14:46Z

controllers/storagecluster/storageclasses.go

@@ -228,6 +228,12 @@ func newCephBlockPoolStorageClassConfiguration(initData *ocsv1.StorageCluster) S
 		disable:           managementSpec.DisableStorageClass,
 		isClusterExternal: initData.Spec.ExternalStorage.Enable,
 	}
+	if dataPool != "" {


~~That's a good point. Where is dataPool used? With EC, the SC config looks like this:~~

~~https://github.com/rook/rook/blob/fffb86295600196389de81905c3402f9aec6e751/deploy/examples/csi/rbd/storageclass-ec.yaml#L45-L46~~

EDIT: actually the code is a bit confusing but this is where parameters are set:

ocs-operator/controllers/storagecluster/external_resources.go

Lines 332 to 334 in d64079d

for k, v := range d.Data {

scc.storageClass.Parameters[k] = v

}

So dataPool and pool will be populated according to the JSON blob content.

controllers/storagecluster/generate.go

controllers/storagecluster/storageclasses.go

controllers/storagecluster/external_resources_test.go

leseb · 2021-12-21T15:13:35Z

controllers/storagecluster/storageclasses.go

+	if data["dataPool"] != "" {
+		sc.storageClass.Parameters["pool"] = generateNameForErasureCodedCephBlockPool(initData)
+		sc.storageClass.Parameters["dataPool"] = generateNameForCephBlockPool(initData)
+	}
+	return sc


I wonder if this block is actually needed since the cluster is external and we don't support EC in converged AFAIK.
@aruniiird to confirm?

You are right, we don't require dataPool value to be set here.
IMO, we don't even have to touch the function newCephBlockPoolStorageClassConfiguration() at all because, as you mentioned in the above comment, that for loop will provide appropriate values for both pool and dataPool parameters.

So, basically we don't need any change just adding unit test to verify working.

Adding unit test to verify erasure coded pool in case of RBD in external cluster. Links: 1) storageClass https://github.com/rook/rook/blob/master/deploy/examples/csi/rbd/storageclass-ec.yaml#L45 2) Json Blob in case of EC RBD pool rook/rook#9276 (comment) Signed-off-by: subhamkrai <srai@redhat.com>

openshift-ci · 2021-12-22T07:46:14Z

@subhamkrai: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/red-hat-storage-ocs-ci-e2e-aws	`951032a`	link	false	`/test red-hat-storage-ocs-ci-e2e-aws`

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/test-infra repository. I understand the commands that are listed here.

leseb · 2021-12-22T11:08:00Z

controllers/storagecluster/external_resources_test.go

+				actualSC.storageClass.Parameters[k] = v
+			}
+			assert.NotEmpty(t, actualSC.storageClass.Parameters["clusterID"])
+			assert.Equal(t, extR.Data["dataPool"], actualSC.storageClass.Parameters["dataPool"])


Where do you test that the dataPool name is ec-data-pool?

"dataPool": "ec-data-pool", this what I'm passing as JSON blob

Yes and it should be in the SC parameters, so the unit test should validate this.

maybe I'm missing something but I'm passing "dataPool": "ec-data-pool" in the JSON blob and checking it with storage class parameters actualSC.storageClass.Parameters["dataPool"]) if provided is equal to expected.

aruniiird

LGTM

openshift-ci · 2021-12-23T02:28:21Z

@aruniiird: changing LGTM is restricted to collaborators

In response to this:

LGTM

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.

agarwal-mudit

/lgtm

openshift-ci · 2021-12-23T03:17:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agarwal-mudit, aruniiird, leseb, subhamkrai

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~~ [agarwal-mudit]

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

subhamkrai force-pushed the external-ec-rbd branch from b0f71c6 to 40069ff Compare December 16, 2021 08:26

subhamkrai marked this pull request as draft December 16, 2021 08:28

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 Dec 16, 2021

subhamkrai marked this pull request as ready for review December 16, 2021 10:30

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 Dec 16, 2021

aruniiird reviewed Dec 16, 2021

View reviewed changes

agarwal-mudit requested a review from jarrpa December 16, 2021 11:23

leseb suggested changes Dec 21, 2021

View reviewed changes

openshift-ci bot assigned leseb Dec 21, 2021

subhamkrai force-pushed the external-ec-rbd branch from 40069ff to d8409a3 Compare December 21, 2021 13:46

leseb suggested changes Dec 21, 2021

View reviewed changes

subhamkrai force-pushed the external-ec-rbd branch from d8409a3 to 99357d3 Compare December 21, 2021 15:01

leseb suggested changes Dec 21, 2021

View reviewed changes

subhamkrai force-pushed the external-ec-rbd branch from 99357d3 to 976eebf Compare December 21, 2021 16:01

subhamkrai force-pushed the external-ec-rbd branch from 976eebf to 951032a Compare December 22, 2021 06:27

subhamkrai requested a review from leseb December 22, 2021 06:34

leseb reviewed Dec 22, 2021

View reviewed changes

leseb approved these changes Dec 22, 2021

View reviewed changes

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

aruniiird approved these changes Dec 23, 2021

View reviewed changes

agarwal-mudit approved these changes Dec 23, 2021

View reviewed changes

openshift-ci bot assigned agarwal-mudit Dec 23, 2021

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

openshift-merge-robot merged commit b1e4ef1 into red-hat-storage:main Dec 23, 2021

subhamkrai deleted the external-ec-rbd branch December 23, 2021 04:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External: configure sc properties for external cluster EC RBD pool #1440

External: configure sc properties for external cluster EC RBD pool #1440

subhamkrai commented Dec 16, 2021 •

edited

subhamkrai commented Dec 16, 2021

subhamkrai commented Dec 16, 2021

aruniiird Dec 16, 2021

leseb Dec 21, 2021 •

edited

agarwal-mudit commented Dec 16, 2021

openshift-ci bot commented Dec 16, 2021

leseb Dec 21, 2021 •

edited

leseb Dec 21, 2021

aruniiird Dec 22, 2021

subhamkrai Dec 22, 2021

openshift-ci bot commented Dec 22, 2021

leseb Dec 22, 2021

subhamkrai Dec 22, 2021

leseb Dec 22, 2021

subhamkrai Dec 22, 2021

aruniiird left a comment

openshift-ci bot commented Dec 23, 2021

agarwal-mudit left a comment

openshift-ci bot commented Dec 23, 2021

	for k, v := range d.Data {
	scc.storageClass.Parameters[k] = v
	}

External: configure sc properties for external cluster EC RBD pool #1440

External: configure sc properties for external cluster EC RBD pool #1440

Conversation

subhamkrai commented Dec 16, 2021 • edited

subhamkrai commented Dec 16, 2021

subhamkrai commented Dec 16, 2021

Choose a reason for hiding this comment

leseb Dec 21, 2021 • edited

Choose a reason for hiding this comment

agarwal-mudit commented Dec 16, 2021

openshift-ci bot commented Dec 16, 2021

leseb Dec 21, 2021 • edited

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 Dec 22, 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

aruniiird left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 23, 2021

agarwal-mudit left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 23, 2021

subhamkrai commented Dec 16, 2021 •

edited

leseb Dec 21, 2021 •

edited

leseb Dec 21, 2021 •

edited