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

Disable uuid checks on XFS #913

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

jsafrane
Copy link
Contributor

@jsafrane jsafrane commented May 31, 2021

Is this a bug fix or adding new feature?
Bugfix

What is this PR about? / Why do we need it?
By default, XFS does not allow mounting two devices that have the same UUID of the XFS filesystem. As result, it's not possible to mount a volume + its restored snapshot.

Therefore disable UUID check in XFS using a mount option.

What testing is done?
Tested with kubernetes/kubernetes#102538

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 31, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2021
@coveralls
Copy link

coveralls commented May 31, 2021

Coverage Status

Coverage increased (+0.08%) to 79.575% when pulling 1ea2f79 on jsafrane:multivolume-test into 2a1c770 on kubernetes-sigs:master.

@jsafrane
Copy link
Contributor Author

jsafrane commented Jun 1, 2021

When tested without the second commit (the actual fix in the CSI driver), I can see the newly added test correctly fails when mounting volume + its snapshot on the same node:

[It] should concurrently access the volume and restored snapshot from pods on the same node [LinuxOnly]
  /home/prow/go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/vendor/k8s.io/kubernetes/test/e2e/storage/testsuites/multivolume.go:337
...
Jun  1 07:58:08.549: INFO: At 2021-06-01 07:53:04 +0000 UTC - event for pod-f465087a-3b4d-4a4a-923a-acac0e6e75e5: {kubelet ip-172-20-45-241.us-west-2.compute.internal} FailedMount: MountVolume.MountDevice failed for volume "pvc-14820d66-fee3-40d6-996e-14eed53ebb45" : rpc error: code = Internal desc = could not format "/dev/xvdbf" and mount it at "/var/lib/kubelet/plugins/kubernetes.io/csi/pv/pvc-14820d66-fee3-40d6-996e-14eed53ebb45/globalmount": mount failed: exit status 32
Mounting command: mount
Mounting arguments: -t xfs -o defaults /dev/xvdbf /var/lib/kubelet/plugins/kubernetes.io/csi/pv/pvc-14820d66-fee3-40d6-996e-14eed53ebb45/globalmount
Output: mount: /var/lib/kubelet/plugins/kubernetes.io/csi/pv/pvc-14820d66-fee3-40d6-996e-14eed53ebb45/globalmount: wrong fs type, bad option, bad superblock on /dev/xvdbf, missing codepage or helper program, or other error.

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-ebs-csi-driver/913/pull-aws-ebs-csi-driver-external-test/1399630187288596480

There is no system journal, still I think it failed to mount due to uuid conflict. With -o nouuid mount option (the second commit), it should succeed (tests are still running).

@sgorshkov85
Copy link

@jsafrane Hello, this ticket is critical for our team. Do you have any ETA for this PR? Sorry, to disturb you, but I need it to schedule my future work.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 9, 2021
@jsafrane jsafrane changed the title WIP: Test mounting restored snapshot + original volume to the same node Disable uuid checks on XFS Jun 9, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2021
@jsafrane
Copy link
Contributor Author

jsafrane commented Jun 9, 2021

Since the e2e test has been merged to Kubernetes (kubernetes/kubernetes#102538), I reworked this PR to update only the driver and add nouuid mount option on XFS.

@wongma7 @AndyXiangLi PTAL.

@wongma7
Copy link
Contributor

wongma7 commented Jun 9, 2021

/lgtm
/approve
thanks.
i created an issue so i remember to opt-in to the test when we bump dependencies to 1.22 #929

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, wongma7

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit ccc060a into kubernetes-sigs:master Jun 9, 2021
@vliggio-tdh
Copy link

Why not just change the UUID on the restored device?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants