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: remove topologyConstrainedPools parameter #4499

Merged
merged 3 commits into from
Apr 5, 2024
Merged

rbd: remove topologyConstrainedPools parameter #4499

merged 3 commits into from
Apr 5, 2024

Conversation

iPraveenParihar
Copy link
Contributor

Describe what this PR does

This commit removes the topologyConstrainedPools parameter from PV volumeAttributes as it is not required.

Related issues

Fixes: #4497

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!)

@mergify mergify bot added the component/rbd Issues related to RBD label Mar 14, 2024
@iPraveenParihar
Copy link
Contributor Author

verified that topologyConstrainedPools has been removed.

[pm@dhcp53-176 examples]$ k get pv pvc-5aaf7aef-fda5-4a57-af7c-abbb737d77bb -o jsonpath='{.spec.csi.volumeAttributes}' | jq
{
  "clusterID": "rook-ceph",
  "encryptionKMSID": "azure-kms",
  "encryptionType": "block",
  "imageFeatures": "layering",
  "imageName": "csi-vol-1292393c-b86a-4ff7-abb7-52750879d83a",
  "journalPool": "replicapool",
  "pool": "rp",
  "storage.kubernetes.io/csiProvisionerIdentity": "1710418563048-5228-rook-ceph.rbd.csi.ceph.com"
}

@iPraveenParihar iPraveenParihar force-pushed the remove/topologyConstrainedPools-param branch from 6d70eb8 to f438135 Compare March 27, 2024 08:07
@iPraveenParihar iPraveenParihar marked this pull request as ready for review March 27, 2024 08:07
@iPraveenParihar iPraveenParihar force-pushed the remove/topologyConstrainedPools-param branch 3 times, most recently from 9aada71 to 21a71d3 Compare March 27, 2024 09:25
Madhu-1
Madhu-1 reviewed Mar 27, 2024
View reviewed changes
internal/rbd/rbd_util.go Outdated Show resolved Hide resolved
@iPraveenParihar iPraveenParihar force-pushed the remove/topologyConstrainedPools-param branch from 21a71d3 to b85269a Compare March 27, 2024 11:20
@mergify Mergify
Copy link
Contributor

mergify bot commented Mar 27, 2024

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@iPraveenParihar iPraveenParihar force-pushed the remove/topologyConstrainedPools-param branch from b85269a to 442ba67 Compare March 27, 2024 11:21
@iPraveenParihar iPraveenParihar requested a review from Madhu-1 March 27, 2024 12:17
@iPraveenParihar iPraveenParihar force-pushed the remove/topologyConstrainedPools-param branch 2 times, most recently from edad60f to e6a17c9 Compare March 28, 2024 05:24
nixpanic
nixpanic reviewed Mar 28, 2024
View reviewed changes
internal/util/topology.go Outdated Show resolved Hide resolved
@iPraveenParihar iPraveenParihar force-pushed the remove/topologyConstrainedPools-param branch 3 times, most recently from c8c1d71 to a4632ad Compare March 29, 2024 03:07
nixpanic
nixpanic reviewed Mar 29, 2024
View reviewed changes
internal/util/util.go Outdated Show resolved Hide resolved
@iPraveenParihar iPraveenParihar requested a review from nixpanic April 1, 2024 03:36
@iPraveenParihar iPraveenParihar force-pushed the remove/topologyConstrainedPools-param branch from a7ad618 to 2b72bf8 Compare April 1, 2024 03:41
@nixpanic nixpanic requested a review from a team April 3, 2024 08:19
@nixpanic
Copy link
Member

nixpanic commented Apr 4, 2024

@Mergifyio rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 4, 2024

rebase

✅ Branch has been successfully rebased

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Apr 4, 2024
@nixpanic
Copy link
Member

nixpanic commented Apr 5, 2024

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

@nixpanic
Copy link
Member

nixpanic commented Apr 5, 2024

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

spurious failure, the logs show that an image was not cleaned up (yet?):

[FAILED] backend images not matching kubernetes resource count,image count 1 kubernetes resource count 0

@nixpanic
Copy link
Member

nixpanic commented Apr 5, 2024

@Mergifyio requeue

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 5, 2024

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Apr 5, 2024

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

spurious failure, the logs show that an image was not cleaned up (yet?):

[FAILED] backend images not matching kubernetes resource count,image count 1 kubernetes resource count 0

i think we need to update the validation function to print rbd info and rbd status of each images to get more clarity what happened. it looks like the image is opened by someone but not closed? not sure who

@Madhu-1 Madhu-1 mentioned this pull request Apr 5, 2024
Open
@nixpanic
Copy link
Member

nixpanic commented Apr 5, 2024

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 5, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at c146724

@iPraveenParihar
rbd: remove topologyConstrainedPools parameter
921a302 
This commit removes the `topologyConstrainedPools` parameter
from PV volumeAttributes as it is not required.

Signed-off-by: Praveen M <m.praveen@ibm.com>
@iPraveenParihar
cephfs: refactor code for improved reusability
8d318d2 
Signed-off-by: Praveen M <m.praveen@ibm.com>
@iPraveenParihar
cleanup: use slices package
da9ef00 
This commit replaces the user implemented function
`CheckSliceContains()` with `slices.Contains()`
function introduced in Go 1.21.

Signed-off-by: Praveen M <m.praveen@ibm.com>
@iPraveenParihar iPraveenParihar force-pushed the remove/topologyConstrainedPools-param branch from 92bd938 to da9ef00 Compare April 5, 2024 10:30
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Apr 5, 2024
@ceph-csi-bot
Copy link
Collaborator

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

@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/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@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.27

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Apr 5, 2024
@nixpanic nixpanic mentioned this pull request Apr 5, 2024
Merged
@mergify mergify bot merged commit c146724 into ceph:devel Apr 5, 2024
35 checks passed
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

Assignees
No one assigned
Labels
component/rbd Issues related to RBD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't add topology details in the pv volumeAttributes
4 participants
@iPraveenParihar @nixpanic @ceph-csi-bot @Madhu-1