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.

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

cephfs: set Pool parameter to empty for Snapshot-backed volumes #4047

Merged
merged 1 commit into from Aug 17, 2023

Conversation

Rakshith-R
Copy link
Contributor

Describe what this PR does

Set VolumeOptions.Pool parameter to empty for Snapshot-backed volumes. This Pool parameter is optional and only used as 'pool_layout' parameter during subvolume and subvolume clone create request in cephcsi and not used for Snapshot-backed volume at all.
It is not saved anywhere for use in subsequent operations after create too. Therefore, We can set it to empty and not error out.

Refer: https://docs.ceph.com/en/latest/cephfs/fs-volumes/?highlight=pool_layout#fs-subvolumes

Resolves: #3820

@Rakshith-R Rakshith-R requested review from a team August 9, 2023 11:29
@mergify mergify bot added the component/cephfs Issues related to CephFS label Aug 9, 2023
Comment on lines 545 to 548
// Pool parameters is optional and only used to set 'pool_layout' arguement for
// subvolume and subvolume clone create commands.
// Setting this to emtpy since it is not used with Snapshot-backed volume.
vo.Pool = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add an E2E testing to ensure it works and we will not break it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@Rakshith-R Rakshith-R force-pushed the fix-cephfs-ro-pool branch 2 times, most recently from 4ec5e0a to c516125 Compare August 9, 2023 11:46
@Rakshith-R
Copy link
Contributor Author

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

// Pool parameters is optional and only used to set 'pool_layout' argument for
// subvolume and subvolume clone create commands.
// Setting this to empty since it is not used with Snapshot-backed volume.
vo.Pool = ""
Copy link
Member

Choose a reason for hiding this comment

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

Why should this not be the same as the pool of the original volume?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why should this not be the same as the pool of the original volume?

The original volume's pool is set only for create pvc/clone cmd as pool_layout (pool is retrieved directly from pv attributes) and is not saved anywhere and not used anywhere else.

The parentBackingSnapVolOpts here does not have its pool parameter set.

if vo.Pool != "" {
return errors.New("cannot set pool for snapshot-backed volume")
}
// Pool parameters is optional and only used to set 'pool_layout' argument for
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pool parameters to Pool parameter

@nixpanic
Copy link
Member

@Mergifyio rebase

Set VolumeOptions.Pool parameter to empty for Snapshot-backed volumes.
This Pool parameter is optional and  only used as 'pool-layout' parameter
during subvolume and subvolume clone create request in cephcsi
and not used for Snapshot-backed volume at all.
It is not saved anywhere for use in subsequent operations after create too.
Therefore, We can set it to empty and not error out.

Signed-off-by: rakshith-r <rar@redhat.com>
@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2023

rebase

✅ Branch has been successfully rebased

@nixpanic nixpanic added the ok-to-test Label to trigger E2E tests label Aug 17, 2023
@ceph-csi-bot
Copy link
Collaborator

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

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

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

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Aug 17, 2023
@mergify mergify bot merged commit f1e9d80 into ceph:devel Aug 17, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/cephfs Issues related to CephFS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cephfs: RO Restore fails when storageclass has pool paramter
4 participants