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

rbd: make pool optional in rbd sc if topologyconstraints are present #4459

Merged
merged 1 commit into from
Mar 22, 2024
Merged

rbd: make pool optional in rbd sc if topologyconstraints are present #4459

merged 1 commit into from
Mar 22, 2024

Conversation

parth-gr
Copy link
Contributor

@parth-gr parth-gr commented Feb 26, 2024 •
edited
Loading

Describe what this PR does

if the rbd storage class is created with topologyconstraintspools, replicated pool is still mandatory, making the pool optional if the topologyconstraintspools are requested

Fixes: #4380

Future concerns

List items that are not part of the PR and do not impact it's
functionality, but are work items that can be taken up subsequently.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes
    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)

@parth-gr parth-gr marked this pull request as draft February 26, 2024 14:04
@mergify mergify bot added the component/rbd Issues related to RBD label Feb 26, 2024
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 5, 2024
@parth-gr
external: add replicated 3 pool for the rbd sc
4a6f017 
till the csi pr get fix
ceph/ceph-csi#4459
we can still make
use of topology by adding the replicated 3 pool
once the pr gets merged will revert this commit

Signed-off-by: parth-gr <[email protected]>
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 5, 2024
@parth-gr
external: add replicated 3 pool for the rbd sc
7182018 
till the csi pr get fix
ceph/ceph-csi#4459
we can still make
use of topology by adding the replicated 3 pool
once the pr gets merged will revert this commit

Signed-off-by: parth-gr <[email protected]>
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 5, 2024
@parth-gr
external: add replicated 3 pool for the rbd sc
12b300d 
till the csi pr get fix
ceph/ceph-csi#4459
we can still make
use of topology by adding the replicated 3 pool
once the pr gets merged will revert this commit

Signed-off-by: parth-gr <[email protected]>
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 6, 2024
@parth-gr
external: add replicated 3 pool for the rbd sc
49beae4 
till the csi pr get fix
ceph/ceph-csi#4459
we can still make
use of topology by adding the replicated 3 pool
once the pr gets merged will revert this commit

Signed-off-by: parth-gr <[email protected]>
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 7, 2024
@parth-gr
external: add replicated 3 pool for the rbd sc
8c93519 
till the csi pr get fix
ceph/ceph-csi#4459
we can still make
use of topology by adding the replicated 3 pool
once the pr gets merged will revert this commit

Signed-off-by: parth-gr <[email protected]>
nixpanic
nixpanic requested changes Mar 7, 2024
View reviewed changes
internal/rbd/controllerserver.go Outdated Show resolved Hide resolved
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 7, 2024
@parth-gr
external: add replicated 3 pool for the rbd sc
42fa6d6 
till the csi pr get fix
ceph/ceph-csi#4459
we can still make
use of topology by adding the replicated 3 pool
once the pr gets merged will revert this commit

Signed-off-by: parth-gr <[email protected]>
@nixpanic nixpanic added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Mar 7, 2024
@parth-gr parth-gr marked this pull request as ready for review March 7, 2024 14:49
@parth-gr parth-gr requested a review from nixpanic March 7, 2024 14:50
@parth-gr
Copy link
Contributor Author

parth-gr commented Mar 7, 2024

And I believe for input validation I have to change this https://github.com/ceph/ceph-csi/blob/devel/internal/rbd/controllerserver.go#L81 to add only if topology not present

@parth-gr parth-gr mentioned this pull request Mar 7, 2024
Merged
6 tasks
@nixpanic
Copy link
Member

nixpanic commented Mar 7, 2024

And I believe for input validation I have to change this https://github.com/ceph/ceph-csi/blob/devel/internal/rbd/controllerserver.go#L81 to add only if topology not present

Yes, I think that needs to depend on the topology details as well.

parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 8, 2024
@parth-gr
external: add replicated 3 pool for the rbd sc
f525edf 
till the csi pr get fix
ceph/ceph-csi#4459
we can still make
use of topology by adding the replicated 3 pool
once the pr gets merged will revert this commit

Signed-off-by: parth-gr <[email protected]>
@parth-gr parth-gr changed the title rbd: make pool optional in rbd sc if topologyconstraints are present [WIP] rbd: make pool optional in rbd sc if topologyconstraints are present Mar 8, 2024
@parth-gr parth-gr force-pushed the tcpools branch 2 times, most recently from 3f06ab0 to 495b6af Compare March 8, 2024 14:14
@parth-gr parth-gr requested a review from nixpanic March 8, 2024 14:14
@parth-gr parth-gr force-pushed the tcpools branch 2 times, most recently from ac2db3f to 31ae182 Compare March 8, 2024 15:03
@parth-gr parth-gr changed the title [WIP] rbd: make pool optional in rbd sc if topologyconstraints are present rbd: make pool optional in rbd sc if topologyconstraints are present Mar 8, 2024
@nixpanic
Copy link
Member

nixpanic commented Mar 8, 2024

/test ci/centos/mini-e2e-helm/k8s-1.28

parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 11, 2024
@parth-gr
external: add replicated 3 pool for the rbd sc
812a9c0 
till the csi pr get fix
ceph/ceph-csi#4459
we can still make
use of topology by adding the replicated 3 pool
once the pr gets merged will revert this commit

Signed-off-by: parth-gr <[email protected]>
@parth-gr
Copy link
Contributor Author

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Mar 22, 2024
@mergify mergify bot merged commit 063319f into ceph:devel Mar 22, 2024
34 checks passed
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 26, 2024
@parth-gr
external: remove replicated 3 pool for the rbd sc
0ea6527 
csi pr get merged ceph/ceph-csi#4459
so, This reverts commit 812a9c0.

Signed-off-by: parth-gr <[email protected]>
@parth-gr parth-gr mentioned this pull request Mar 26, 2024
Merged
6 tasks
parth-gr added a commit to parth-gr/ocs-operator that referenced this pull request Mar 26, 2024
@parth-gr
rbd: update rbd sc for topology provisioning
0783a56 
with new fix at csi ceph/ceph-csi#4459 we dont need
the pool parameter dependency from the storageclass
So removing it

Signed-off-by: parth-gr <[email protected]>
@parth-gr parth-gr mentioned this pull request Mar 26, 2024
Merged
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 26, 2024
@parth-gr
external: remove replicated 3 pool for the rbd sc
1d7ceaa 
csi pr get merged ceph/ceph-csi#4459
so, This reverts commit 812a9c0.
The csi PR removes the dependency of the pool parameter from the
rbd storage class if the topology pools are passed,
so removing them in the examples and making it optional

Signed-off-by: parth-gr <[email protected]>
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 27, 2024
@parth-gr
external: remove replicated 3 pool for the rbd sc
647ccc7 
csi pr get merged ceph/ceph-csi#4459
so, This reverts commit 812a9c0.
The csi PR removes the dependency of the pool parameter from the
rbd storage class if the topology pools are passed,
so removing them in the examples and making it optional

Signed-off-by: parth-gr <[email protected]>
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 27, 2024
@parth-gr
external: remove replicated pool from json data
b729de0 
csi pr get merged ceph/ceph-csi#4459
The csi PR removes the dependency of the pool parameter from the
rbd storage class if the topology pools are passed,
so removing them in the examples and making it optional

Signed-off-by: parth-gr <[email protected]>
@parth-gr parth-gr mentioned this pull request Mar 27, 2024
Closed
6 tasks
parth-gr added a commit to parth-gr/ocs-operator that referenced this pull request Mar 27, 2024
@parth-gr
rbd: update rbd sc for topology provisioning
7f019f2 
with new fix at csi ceph/ceph-csi#4459 we dont need
the pool parameter dependency from the storageclass
So removing it

Signed-off-by: parth-gr <[email protected]>
parth-gr added a commit to parth-gr/ocs-operator that referenced this pull request Mar 27, 2024
@parth-gr
rbd: update rbd sc for topology provisioning
6c980c3 
with new fix at csi ceph/ceph-csi#4459 we dont need
the pool parameter dependency from the storageclass
So removing it

and currently the topology storageclass also checks for
the replicated cephblockpool which is not needed
so adding a check if it is topology sc then do not
check for replicated pool

Signed-off-by: parth-gr <[email protected]>
parth-gr added a commit to parth-gr/rook that referenced this pull request Mar 27, 2024
@parth-gr
external: remove replicated 3 pool for the rbd sc
58f96b6 
csi pr get merged ceph/ceph-csi#4459
so, This reverts commit 812a9c0.
The csi PR removes the dependency of the pool parameter from the
rbd storage class if the topology pools are passed,
so removing them in the examples and making it optional

Signed-off-by: parth-gr <[email protected]>
@parth-gr
Copy link
Contributor Author

parth-gr commented Mar 27, 2024 •
edited
Loading

@nixpanic just want understand how the upgrade will go with this? So old pvc will still be fine and use the journal pool as replica pool, but what if after upgrade there is a pvc expansion request for old pvc(storage class parameter will change)? Will there be any impact..
I can check the above scenario, do I need to see anything specific too objects

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Mar 28, 2024

@nixpanic just want understand how the upgrade will go with this? So old pvc will still be fine and use the journal pool as replica pool, but what if after upgrade there is a pvc expansion request for old pvc(storage class parameter will change)? Will there be any impact.. I can check the above scenario, do I need to see anything specific too objects

These parameters will not be set for any operation other than PVC creation.

Nikhil-Ladha pushed a commit to red-hat-storage/rook that referenced this pull request Mar 28, 2024
@parth-gr @Nikhil-Ladha
external: remove replicated 3 pool for the rbd sc
0611d19 
csi pr get merged ceph/ceph-csi#4459
so, This reverts commit 812a9c0.
The csi PR removes the dependency of the pool parameter from the
rbd storage class if the topology pools are passed,
so removing them in the examples and making it optional

Signed-off-by: parth-gr <[email protected]>
@parth-gr
Copy link
Contributor Author

@nixpanic just want understand how the upgrade will go with this? So old pvc will still be fine and use the journal pool as replica pool, but what if after upgrade there is a pvc expansion request for old pvc(storage class parameter will change)? Will there be any impact.. I can check the above scenario, do I need to see anything specific too objects

These parameters will not be set for any operation other than PVC creation.

OKay thanks so we are good with upgrades

Nikhil-Ladha pushed a commit to Nikhil-Ladha/rook that referenced this pull request Mar 28, 2024
@parth-gr @Nikhil-Ladha
external: remove replicated 3 pool for the rbd sc
f04eca8 
csi pr get merged ceph/ceph-csi#4459
so, This reverts commit 812a9c0.
The csi PR removes the dependency of the pool parameter from the
rbd storage class if the topology pools are passed,
so removing them in the examples and making it optional

Signed-off-by: parth-gr <[email protected]>
Nikhil-Ladha pushed a commit to Nikhil-Ladha/rook that referenced this pull request Mar 28, 2024
@parth-gr @Nikhil-Ladha
external: remove replicated 3 pool for the rbd sc
8d6b3eb 
csi pr get merged ceph/ceph-csi#4459
so, This reverts commit 812a9c0.
The csi PR removes the dependency of the pool parameter from the
rbd storage class if the topology pools are passed,
so removing them in the examples and making it optional

Signed-off-by: parth-gr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Madhu-1 Madhu-1 Madhu-1 approved these changes

@nixpanic nixpanic nixpanic approved these changes

@Rakshith-R Rakshith-R Awaiting requested review from Rakshith-R

@iPraveenParihar iPraveenParihar Awaiting requested review from iPraveenParihar

Assignees
No one assigned
Labels
component/deployment Helm chart, kubernetes templates and configuration Issues/PRs component/rbd Issues related to RBD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rbd storageclass should make pool optional if topologyConstrainedPools parameter is used
5 participants
@parth-gr @nixpanic @iPraveenParihar @Madhu-1 @ceph-csi-bot