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: remove extra check for restore size #3183

Merged
merged 1 commit into from Jul 18, 2022
Merged

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Jun 14, 2022

Looks like the cephfs snapshot size is buggy and it's getting removed in ceph fs. we cannot get the size of the snapshot during the CreateVolume call, so we cannot do any size check at CreateVolume to check if the restore size is smaller or not.

As we are removing this check it also fixes #3147 but we dont have any validation at the CSI level for smaller restore we need to depend on kubernetes external-provisioner for it.

fixes: #3147

ceph tracker https://tracker.ceph.com/issues/55822 to remove size from snapshot API.

Signed-off-by: Madhu Rajanna madhupr007@gmail.com

@Madhu-1 Madhu-1 requested review from nixpanic and a team June 14, 2022 06:51
@Madhu-1 Madhu-1 added the component/cephfs Issues related to CephFS label Jun 14, 2022
yati1998
yati1998 previously approved these changes Jun 14, 2022
@Madhu-1 Madhu-1 added the DNM DO NOT MERGE label Jun 14, 2022
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jun 14, 2022

Adding DNM and converting it to draft as the discussion is going on for snapshot-size

@Madhu-1 Madhu-1 marked this pull request as draft June 14, 2022 07:29
@nixpanic
Copy link
Collaborator

nixpanic commented Jun 14, 2022 via email

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jun 14, 2022

On Mon, Jun 13, 2022 at 11:51:55PM -0700, Madhu Rajanna wrote: Looks like the cephfs snapshot size is buggy and it's getting removed in ceph fs. we cannot get the size of the snapshot during the CreateVolume call, so we cannot do any size check at CreateVolume to check if the restore size is smaller or not.
If this is buggy, add a reference to the Ceph issue tracker please.

Done!

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jun 15, 2022

/retest ci/centos/mini-e2e-helm/k8s-1.23

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jun 15, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Jun 15, 2022

rebase

✅ Branch has been successfully rebased

@Madhu-1 Madhu-1 removed the DNM DO NOT MERGE label Jun 15, 2022
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jun 15, 2022

Removing DNM as discussed in the team meeting. we are good to consider this one.

@Madhu-1 Madhu-1 marked this pull request as ready for review June 15, 2022 14:10
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jun 15, 2022

@nixpanic PTAL

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jun 16, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Jun 16, 2022

rebase

✅ Branch has been successfully rebased

@mergify
Copy link
Contributor

mergify bot commented Jun 16, 2022

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

@mergify mergify bot dismissed yati1998’s stale review June 16, 2022 10:09

Pull request has been modified.

@Madhu-1 Madhu-1 requested a review from yati1998 June 16, 2022 10:09
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jun 16, 2022

Resolved merge conflicts and rebased

if vol.BackingSnapshot {
if vol.Size != parentVol.Size {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gman0 I am removing this check as it can cause a problem if the parent subvolume is resized after taking a snapshot, please check if am missing anything.

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jun 20, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Jun 20, 2022

rebase

✅ Branch has been successfully rebased

@Rakshith-R Rakshith-R requested review from gman0 and a team July 7, 2022 11:01
@Rakshith-R Rakshith-R added the bug Something isn't working label Jul 7, 2022
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jul 15, 2022

@humblec @nixpanic PTAL

@Madhu-1 Madhu-1 requested a review from humblec July 15, 2022 05:13
@Rakshith-R
Copy link
Contributor

@Mergifyio rebase

@Rakshith-R Rakshith-R added the ci/retry/e2e Label to retry e2e retesting on approved PR's label Jul 18, 2022
Looks like cephfs snapshot size is buggy and its
getting removed in ceph fs. we cannot get the size
of the snapshot during CreateVolume call, so we cannot
do any size check at CreateVolume to check if the
restore size is smaller or not.

As we are removing this check it also fixes ceph#3147
but we dont have any validation at CSI level for
smaller restore we need to depend on kubernetes
external-provisioner for it.

fixes: ceph#3147

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2022

rebase

✅ Branch has been successfully rebased

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/k8s-e2e-external-storage/1.21

@ceph-csi-bot
Copy link
Collaborator

@Madhu-1 "ci/centos/k8s-e2e-external-storage/1.21" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2022

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/k8s-e2e-external-storage/1.23

@ceph-csi-bot
Copy link
Collaborator

@Madhu-1 "ci/centos/k8s-e2e-external-storage/1.23" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/mini-e2e-helm/k8s-1.22

@ceph-csi-bot
Copy link
Collaborator

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.22" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2022

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/k8s-e2e-external-storage/1.23

@ceph-csi-bot
Copy link
Collaborator

@Madhu-1 "ci/centos/k8s-e2e-external-storage/1.23" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2022

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@mergify mergify bot merged commit ceb88d6 into ceph:devel Jul 18, 2022
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci/retry/e2e Label to retry e2e retesting on approved PR's component/cephfs Issues related to CephFS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cephfs: failed to create pvc from snapshot
6 participants