-
Notifications
You must be signed in to change notification settings - Fork 141
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
filesystem is not resized when restoring from snapshot/cloning to larger size than origin #972
filesystem is not resized when restoring from snapshot/cloning to larger size than origin #972
Conversation
Welcome @RomanBednar! |
Hi @RomanBednar. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@mattcary Hi, I've noticed there are tests for NodeExpandVolume that are commented out with a note about fakeexec being too brittle:
But, by adding mount-utils resizer to NodeStageVolume I broke the existing unit tests for this function because there are actual command executions in the resize flow and so I used the fakeexec - just to have some working example. Better approach might be using a mock interface instead. What do you think? |
/ok-to-test |
I dunno, creating a mock that would actually catch errors or regressions is very hard. Maybe just stick with fakeexec for now and we'll see how it does. This should be exercised in e2e tests as it's part of the csi spec (but probably isn't?) |
We've investigated the tests with @jsafrane and we currently see two issues:
I'll look into it and update the CSI tests accordingly once we resolve the sanity test issue. Another catch is that adding csi e2e tests will take quite some time before we can use it (until next Kubernetes release at best). @mattcary What would you suggest to fix the sanity tests? And do we want to wait with this patch until e2e tests are available? |
Thanks for laying out the state of things so clearly! Would it be easy to add a gcp-persistent-disk-csi-driver/test/e2e test? (confusingly this is not the k/k e2e test) It should be about as straightfoward as adding a sanity check, except it uses a real cloud provider etc so no faking is necessary. |
@mattcary, while it would be possible to add a new test to test/e2e, it's not easy to fix e2e/sanity - all Out of curiosity, is there any reason why don't you run pull-gcp-compute-persistent-disk-csi-driver-sanity tests with a real cloud + mounter + exec? |
I agree. Sorry for not making that more clear---I don't think we should try to fix sanity, I think we should just put it in an e2e test. That would be skipping resize tests in sanity (the other tests will continue to work, or have those been broken too?) I'm not actually familiar with the sanity tests, I've never had a reason to look at them. If we do run them in a cloud context, would they substantially duplicate the e2e tests? I'll have to look into them. I suppose our e2e tests hit GCE specific features that aren't covered by sanity, and sanity does more thorough generic CSI testing. If my supposition is true I think it would probably be a good idea to run them with real resources. That's been reliable in the e2e tests, and I've never found fakes a very convincing test strategy.
test/k8s-integration runs the k/k tests. There's real value in splitting those out from e2e tests as they take a lot longer to run and are way way flakier. Just bringing up a cluster consistently across k8s minor versions, different versions of kubetest, etc, is a lot of toil. |
Unfortunately, all sanity tests that call NodeStage will fail, because NodeStage is not able to read device size with the fake exec.
There definitely is some overlap in your custom e2e tests, kubernetes-integration and sanity test. The sanity tests add some checks that cannot be easily tested in Kubernetes, like that the driver returns the right error code when a volume does not exist, staging path does not exist and so on. If you want to use sanity tests, then you should run them against a real CSI driver in a real cloud. On the other hand, you don't need Kubernetes to run the tests (unless the driver itself needs to talk to Kubernetes). You need a VM in GCE, run the CSI driver in the VM (either in a container or directly on the host, it does not really matter) and poke the driver's CSI socket with the sanity tests. It will provision real volumes, attach them to the VM and mount them. |
See #991 and kubernetes/test-infra#26354, we're making the sanity tests optional while we sort this out. I think that unblocks this PR? (as soon as the test-infra change goes in) |
/refresh |
c244d1f
to
8ffb6e1
Compare
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
b925ef8
to
0cc341d
Compare
NodeStageVolume() which is called few lines above in NodeStageExt4Volume() can now resize filesystem if it's needed so this check is no longer valid.
dc2d805
to
41b3dfd
Compare
Thanks for the review @mauriciopoppe ! /lgtm (and of course thanks for the PR @RomanBednar :-) |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattcary, RomanBednar 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 |
This test was also disabled upstream: https://github.com/kubernetes/test-infra/pull/26354/files The problem is that sanity tests currently use command faker and only one driver instance. That makes any command faking impossible as the same instance of the driver is re-used in many tests. Relevant discussion upstream is here: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#972 (comment)
This test was also disabled upstream: https://github.com/kubernetes/test-infra/pull/26354/files The problem is that sanity tests currently use command faker and only one driver instance. That makes any command faking impossible as the same instance of the driver is re-used in many tests. Relevant discussion upstream is here: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#972 (comment)
This test was also disabled upstream: https://github.com/kubernetes/test-infra/pull/26354/files The problem is that sanity tests currently use command faker and only one driver instance. That makes any command faking impossible as the same instance of the driver is re-used in many tests. Relevant discussion upstream is here: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#972 (comment)
… is not backported to 1.8-. The fix is only in 1.9+.
… is not backported to 1.8-. The fix is only in 1.9+.
… is not backported to 1.8-. The fix is only in 1.9+.
… is not backported to 1.8-. The fix is only in 1.9+.
… is not backported to 1.8-. The fix is only in 1.9+.
… is not backported to 1.8-. The fix is only in 1.9+.
… is not backported to 1.8-. The fix is only in 1.9+.
… is not backported to 1.8-. The fix is only in 1.9+.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Volume filesystem size is not expanded during clone if pvc data source is larger than original volume.
Similarly when restoring a snapshot to a pvc with larger size the filesystem is not expanded either.
This patch uses mount-utils to handle volume size check and resizing.
Which issue(s) this PR fixes:
Fixes #784
Special notes for your reviewer:
Here is how I verified the patch manually.
BEFORE PATCH:
AFTER PATCH:
Does this PR introduce a user-facing change?: