Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

storageclassrequest: reconcile RADOS Namespace for blockpool request #2478

Merged
merged 4 commits into from
Mar 20, 2024
Merged

storageclassrequest: reconcile RADOS Namespace for blockpool request #2478

merged 4 commits into from
Mar 20, 2024

Conversation

jarrpa
Copy link
Member

@jarrpa jarrpa commented Feb 23, 2024

Signed-off-by: Jose A. Rivera [email protected]

@openshift-ci 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 Feb 23, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 23, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2024
@jarrpa jarrpa changed the title storageclassrequest: reconcile RADOS Namespace for blockpool request [WIP] storageclassrequest: reconcile RADOS Namespace for blockpool request Feb 23, 2024
@jarrpa jarrpa force-pushed the jarrpa/rns-provider branch 7 times, most recently from 9b2f572 to 33f5168 Compare February 26, 2024 19:21
@leelavg leelavg mentioned this pull request Mar 4, 2024
Merged
nb-ohad
nb-ohad requested changes Mar 12, 2024
View reviewed changes
controllers/storageclassrequest/storageclassrequest_controller.go Show resolved Hide resolved
controllers/storageclassrequest/storageclassrequest_controller.go Outdated Show resolved Hide resolved
controllers/storageclassrequest/storageclassrequest_controller.go Outdated Show resolved Hide resolved
controllers/storageclassrequest/storageclassrequest_controller.go Outdated Show resolved Hide resolved
controllers/storageclassrequest/storageclassrequest_controller.go Outdated Show resolved Hide resolved
controllers/storageclassrequest/storageclassrequest_controller.go Outdated Show resolved Hide resolved
controllers/storageclassrequest/storageclassrequest_controller.go Outdated Show resolved Hide resolved
controllers/storageclassrequest/storageclassrequest_controller.go Outdated Show resolved Hide resolved
controllers/storageclassrequest/storageclassrequest_controller.go Show resolved Hide resolved
controllers/storageclassrequest/storageclassrequest_controller.go Outdated Show resolved Hide resolved
@nb-ohad nb-ohad removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2024
@jarrpa jarrpa force-pushed the jarrpa/rns-provider branch from 33f5168 to e321c97 Compare March 12, 2024 19:53
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2024
@jarrpa
Copy link
Member Author

jarrpa commented Mar 12, 2024

Address most of the review comments, going to step away for a bit for the afternoon. Still not done with removing storage profiles (getting there...) but pushing what I have thus far just in case.

@jarrpa jarrpa force-pushed the jarrpa/rns-provider branch 3 times, most recently from bd9870b to 6aa7135 Compare March 13, 2024 21:22
@jarrpa jarrpa changed the title [WIP] storageclassrequest: reconcile RADOS Namespace for blockpool request storageclassrequest: reconcile RADOS Namespace for blockpool request Mar 13, 2024
@jarrpa jarrpa marked this pull request as ready for review March 13, 2024 21:24
@openshift-ci 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 Mar 13, 2024
@jarrpa
Copy link
Member Author

jarrpa commented Mar 13, 2024

Aaaand finally I can consider this ready for review. I still need to do another round of rigorous testing based on all the recent review changes, but for now this is close enough.

NOTE: StorageProfiles still remain, but only for SubvolumeGroups. Apart from being out of scope for this PR, I'll want to have some discussion with folks more familiar with subvolume groups as to how to best determine/set/match device classes.

@jarrpa
Copy link
Member Author

jarrpa commented Mar 14, 2024

/test ocs-operator-bundle-e2e-aws

1 similar comment
@jarrpa
Copy link
Member Author

jarrpa commented Mar 14, 2024

/test ocs-operator-bundle-e2e-aws

@jarrpa
Copy link
Member Author

jarrpa commented Mar 15, 2024

I don't know what's causing the e2e-test to fail.... nothing in the operator Pod logs suggests an error of any kind, the tests just time out waiting for the StorageCluster... which it seems to do by watching for the OperatorCondition to be "Upgradable", I think? But the ocs-must-gather doesn't look like it ran, either, so I can't inspect any of the CRs. Any ideas?

@jarrpa jarrpa force-pushed the jarrpa/rns-provider branch 2 times, most recently from 667cdd4 to 765203e Compare March 19, 2024 03:53
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2024
@jarrpa jarrpa force-pushed the jarrpa/rns-provider branch from 765203e to 661ae59 Compare March 19, 2024 14:43
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2024
@jarrpa jarrpa force-pushed the jarrpa/rns-provider branch 2 times, most recently from b4ad713 to 80ace6d Compare March 19, 2024 15:06
jarrpa added 3 commits March 19, 2024 11:46
@jarrpa
provider: slight refactor to GetStorageClassClaimConfig()
eb84948 
Since a StorageClass resource *must* always be returned as one of the
external resources in the StorageClassClaimConfig, we might as well
start defining it outside the loop over the StorageClassRequest's
CephResources. This enables the upcoming enhancement to add RADOS
namespace information.

Signed-off-by: Jose A. Rivera <[email protected]>
@jarrpa
provider: check for RADOS namespace in GetStorageClassClaimConfig()
220c8bf 
Signed-off-by: Jose A. Rivera <[email protected]>
@jarrpa
controllers: define OwnersIndexFieldFunc()
b522666 
Signed-off-by: Jose A. Rivera <[email protected]>
@jarrpa jarrpa force-pushed the jarrpa/rns-provider branch from 80ace6d to 5b39995 Compare March 19, 2024 16:50
@jarrpa jarrpa force-pushed the jarrpa/rns-provider branch from 5b39995 to baacc8c Compare March 19, 2024 17:34
@jarrpa
storageclassrequest: fulfill requests with RADOS namespaces
f8c4eed 
This set of changes achieves two things:

* Reconciles a given CephBlockPool to serve a single storage profile,
  regardless of consumer. As part of this, also changes the
  CephBlockPool name generation to remove the UUID portion.
* Reconciles a CephBlockPoolRadosNamespace to provide isolation of data
  for consumers on shared block pools.

Signed-off-by: Jose A. Rivera <[email protected]>
@jarrpa jarrpa force-pushed the jarrpa/rns-provider branch from baacc8c to f8c4eed Compare March 19, 2024 17:36
@nb-ohad
Copy link
Contributor

nb-ohad commented Mar 19, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jarrpa, nb-ohad

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:

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

@agarwal-mudit
Copy link
Member

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit cdbfe1d into red-hat-storage:main Mar 20, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leelavg leelavg leelavg left review comments

@rewantsoni rewantsoni rewantsoni left review comments

@nb-ohad nb-ohad nb-ohad requested changes

Assignees

@nb-ohad nb-ohad

Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jarrpa @nb-ohad @agarwal-mudit @rewantsoni @leelavg @openshift-merge-robot