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

cephfs: remove subvolumegroup creation #4195

Merged
merged 3 commits into from
Nov 10, 2023
Merged

cephfs: remove subvolumegroup creation #4195

merged 3 commits into from
Nov 10, 2023

Conversation

iPraveenParihar
Copy link
Contributor

Describe what this PR does

This commit remove the creation of subvolumegroup.

Is there anything that requires special attention

Is the change backward compatible? Yes

Are there concerns around backward compatibility? No

Fixes: #4185

@mergify mergify bot added the component/cephfs Issues related to CephFS label Oct 16, 2023
@Rakshith-R
Copy link
Contributor

@iPraveenParihar You also need to modify docs asking users to create subvolumegroup before hand.

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-subvolumegroup-creation branch from e50088f to cd580d8 Compare October 16, 2023 10:15
Madhu-1
Madhu-1 reviewed Oct 16, 2023
View reviewed changes
internal/cephfs/core/volume.go
@@ -241,24 +233,6 @@ func (s *subVolumeClient) CreateVolume(ctx context.Context) error {
return err
}

// create subvolumegroup if not already created for the cluster.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart of documentation update, You might also need below changes/testing

  • Upgrade testing to ensure existing PVC works without any problem
  • E2E tests to create the subvolumegroup
  • Remove extra in-memory check for subvolumegroup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upgrade testing to ensure existing PVC works without any problem

@Madhu-1 existing PVC are already in a subvolumegroup. So, I guess there won't be any problem.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes thats correct just need to test and ensure it works fine, This is already part of upgrade testing i believe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested it, It works fine

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-subvolumegroup-creation branch from cd580d8 to eade799 Compare October 17, 2023 14:45
@iPraveenParihar
Copy link
Contributor Author

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

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-subvolumegroup-creation branch from eade799 to 5d08144 Compare October 18, 2023 05:00
@iPraveenParihar
Copy link
Contributor Author

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

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-subvolumegroup-creation branch from 5d08144 to d75d0fa Compare October 18, 2023 07:27
@iPraveenParihar
Copy link
Contributor Author

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

@iPraveenParihar iPraveenParihar marked this pull request as ready for review October 23, 2023 09:47
@iPraveenParihar
Copy link
Contributor Author

iPraveenParihar commented Oct 23, 2023 •
edited
Loading

@iPraveenParihar You also need to modify docs asking users to create subvolumegroup before hand.

@Rakshith-R, will add a Note section here - docs/deploy-cephfs.md ?

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-subvolumegroup-creation branch 2 times, most recently from 78eb7a9 to 9d21a67 Compare November 2, 2023 08:11
@iPraveenParihar
Copy link
Contributor Author

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

@iPraveenParihar
Copy link
Contributor Author

/test ci/centos/upgrade-tests-cephfs

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-subvolumegroup-creation branch 3 times, most recently from 238a88e to 106b745 Compare November 2, 2023 10:19
Madhu-1
Madhu-1 reviewed Nov 2, 2023
View reviewed changes
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nits.

deploy/csi-config-map-sample.yaml Outdated
@@ -20,6 +20,7 @@ kind: ConfigMap
# NOTE: Make sure you don't add radosNamespace option to a currently in use
# configuration as it will cause issues.
# The field "cephFS.subvolumeGroup" is optional and defaults to "csi".
# NOTE: The given subvolumeGroup must already exists in the pool.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# NOTE: The given subvolumeGroup must already exists in the pool.
# NOTE: The given subvolumeGroup must already exist in the filesystem.

docs/deploy-cephfs.md Outdated
@@ -241,3 +241,10 @@ However, not all KMS are supported in order to be compatible with
[fscrypt](https://github.com/google/fscrypt). In general KMS that
either store secrets to use directly (Vault), or allow access to the
plain password (Kubernetes Secrets) work.

## CephFS Volume Provisioning
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## CephFS Volume Provisioning
## CephFS PVC Provisioning

docs/deploy-cephfs.md Outdated Show resolved Hide resolved
@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-subvolumegroup-creation branch from 106b745 to d85a26e Compare November 2, 2023 11:09
Madhu-1
Madhu-1 reviewed Nov 2, 2023
View reviewed changes
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, lets wait and get this added to #4222?

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-subvolumegroup-creation branch from d85a26e to ff8a4eb Compare November 6, 2023 06:35
@iPraveenParihar
Copy link
Contributor Author

LGTM, lets wait and get this added to #4222?

Added to PendingReleaseNotes.md

Madhu-1
Madhu-1 reviewed Nov 6, 2023
View reviewed changes
PendingReleaseNotes.md Outdated Show resolved Hide resolved
@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-subvolumegroup-creation branch from ff8a4eb to cc0a8f1 Compare November 6, 2023 07:09
Madhu-1
Madhu-1 previously approved these changes Nov 6, 2023
View reviewed changes
Rakshith-R
Rakshith-R previously approved these changes Nov 6, 2023
View reviewed changes
@iPraveenParihar
Copy link
Contributor Author

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

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-subvolumegroup-creation branch from c20158d to 1bd8aed Compare November 10, 2023 07:28
@mergify mergify bot dismissed Madhu-1’s stale review November 10, 2023 07:29

Pull request has been modified.

@Rakshith-R
Copy link
Contributor

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 10, 2023 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

The pull request can't be updated.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@nixpanic
Copy link
Member

@Mergifyio rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 10, 2023

rebase

✅ Branch has been successfully rebased

@nixpanic nixpanic force-pushed the cephfs/remove-subvolumegroup-creation branch from 1bd8aed to c5c7709 Compare November 10, 2023 09:38
@nixpanic
Copy link
Member

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 10, 2023 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at cbc8210

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Nov 10, 2023
@ceph-csi-bot
Copy link
Collaborator

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

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

@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-cephfs

@ceph-csi-bot
Copy link
Collaborator

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

@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-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@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/k8s-1.27

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Nov 10, 2023
@mergify mergify bot merged commit cbc8210 into ceph:devel Nov 10, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rakshith-R Rakshith-R Rakshith-R approved these changes

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

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

Successfully merging this pull request may close these issues.

Remove subvolumegroup creation from cephcsi
6 participants
@iPraveenParihar @Rakshith-R @riya-singhal31 @ceph-csi-bot @Madhu-1 @nixpanic