-
Notifications
You must be signed in to change notification settings - Fork 108
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
Raw block volume mode support #249
Raw block volume mode support #249
Conversation
b25af5a
to
f517dc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good overall; left a few suggestions and questions.
Could I ask you to also amend the change log and extend the feature list in our README?
@@ -147,7 +148,31 @@ func (m *mounter) Mount(source, target, fsType string, opts ...string) error { | |||
return errors.New("target is not specified for mounting the volume") | |||
} | |||
|
|||
mountArgs = append(mountArgs, "-t", fsType) | |||
// This is a raw block device mount. Create the mount point as a file | |||
// since bind mount device node requires it to be a file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I didn't know that you have to use a file target. Curious, how/where did you find out about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trial and error and then found the corresponding man page (https://linux.die.net/man/8/mount) information about it which was extremely vague:
mount --bind olddir newdir
or shortoption
mount -B olddir newdir
or fstab entry is:
/olddir /newdir none bind
After this call the same contents is accessible in two places. One can also remount a single file (on a single file).
Specifically:
One can also remount a single file (on a single file).
Given that the absolute device path is a block device file, we correspondingly have to bind mount that onto another file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍Thanks (and good job discovering this)
I forgot to mention that I haven't reviewed the extended integration tests yet; will do that in a second round. @nicktate btw thanks for the excellent PR description -- appreciated the quick overview I was able to get thanks to that. 👏 |
Noticed another thing... the spec describes how
This raises two questions for me:
|
test/kubernetes/integration_test.go
Outdated
|
||
tt := []struct { | ||
pod func() *v1.Pod | ||
pvc func() *v1.PersistentVolumeClaim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember we discussed that the integration tests will only be useful for testing Kubernetes < 1.14 going forward because newer releases support running the upstream end-to-end tests (that ship with an entire suite of access mode-related tests). What we likely want at some point is introduce a flag to run all integration tests for older releases (1.13 while still supported by us, potentially even older ones) and none for newer releases.
I understand the function returning a PVC is to support testing for filesystem and block mode, respectively, here and for most (all?) test we have build so far. Unfortunately, this would make it difficult to run the tests on 1.13 or below because block mode turned beta in Kubernetes 1.14 only; meaning that the block mode-related tests would all fail on older releases.
Thinking about how we can resolve this, I can see two possibilities:
- Test the access mode in a single, dedicated test only and revert the pre-existing tests to their prior state. That way, we can easily turn tests on and off as needed per tested Kubernetes release.
- Do not test the access mode in our integration tests at all but rely on the upstream tests only. Merging the feature without any tests is not ideal, however, so for this scenario we'd probably want to wait for Add support for running upstream storage end-to-end tests #248 first so that we can enable the block testing capability in the upstream tests.
The first option allows us to move forward without depending on #248, at the price of a bit more tech debt (as we eventually will be able to remove our access mode integration tests entirely in favor of the upstream ones; at least, I assume so). #248 seems close to completion, but you never know, so maybe option 1. seems slightly preferable.
Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the version support 👍
I think #248 is definitely the direction we will move towards but I think 1
or a variation of it would be better in the short term to reduce the risk of this PR getting blocked.
I'm fine either creating a separate test as you suggested in number 1
or simply calculating the kubernetes_version
of the cluster in test during setup
and then having a minimum version gate for the table driven test configuration. I think either way it would be smart to create a version gate for the block tests.
I'm personally leaning towards adding the gate into the updated table driven tests for the enhanced test cleanup functionality and naming. I think if they were staying long term it would be the right decision, but since these tests also have a relatively short EOL I would be fine with reverting them to prior state as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gating on top of your improvements sounds fine for me as well. 👍
Left a general comment/question regarding the direction of our integration tests, which I think we should answer first before going into details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm on a quick read, modulo Timo's notes.
Regarding the resize case, my opinion is that we should not resize filesystems for raw block devices. We didn't create the filesystem, so it feels wrong to touch it later; if a user is using raw block, they're signing up to take care of such things themselves. Interested in others' thoughts on this.
@adamwg / @timoreimann I agree with you both on the https://github.com/kubernetes/kubernetes/blob/4c50ee993c82c6852eb3b3aa8dfa8ecc4bcfe330/pkg/util/resizefs/resizefs_linux.go#L49 ignores resizing of the device if it is not formatted. I hadn't considered the case of the user requesting block and formatting after the fact. I will |
b307610
to
f102259
Compare
That sounds good to me. 👍 FWIW, the |
f102259
to
553c03e
Compare
@timoreimann Most comments have been addressed with exception of test updates & readme/changelog. I will be following up once I have re-ran integration / e2e tests. |
be86518
to
2e40914
Compare
@timoreimann / @adamwg This should be good for a final review. I believe all comments have been addressed and I've re-ran e2e and upstream tests and posted the new results in the description. I do have some interesting findings regarding the expand volume. I updated the logic per the spec and check volume capabilities if it exists, and in the case of block volume attempt to In testing the
I posed the question in the community CSI channel, so you can follow any updates here: https://kubernetes.slack.com/archives/C8EJ01Z46/p1578414177000600. |
I've also tested against #209 and it seems to be working as expected:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 for validating the PR with Rook as well.
LGTM 👍 Can you use Github to squash-merge the change (or squash manually and force-push) so that we end up with a single clean commit in master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
* Refactor integration tests for parallelization and raw block volume mode * Add kubernetes version compatibility check for integration tests * Add changelog and readme updates for raw block mode
2e40914
to
0e22e5c
Compare
Description
What does this pull request accomplish
VolumeMode: Block
supporttest/integration_test.go
Block
volume tests to integration testsAdditional Info
Here is a high level summary of the changes to the driver:
controller.go/validateCapabilities
- Needs to acceptBlock
access mode in the capabilities checknode.go/NodeStageVolume
- Updated to anoop
for block volumes because we bind mount the absolute device path directlynode.go/NodePublishVolume
- Checks theAccessMode
and triggersnodePublishVolumeForFileSystem
ornodePublishVolumeForBlock
respectively.nodePublishVolumeForBlock
is the absolute path of the device on diskmounter.go/Mount
- Updated to handle an emptyfsType
being passed in the case ofBlock
node.go/NodeGetVolumeStats
- Checks to determine if it is a block device. If so, it usesblockdev
to determine thetotalCapacity
of the device (no other information can be collected)Here is a high level summary of the changes to the integration tests:
Filesystem
andBlock
Test Results
Fixes #192