-
Notifications
You must be signed in to change notification settings - Fork 40.1k
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
Add volume spec to mountedPod in actual state of world #61549
Add volume spec to mountedPod in actual state of world #61549
Conversation
@jingxu97: Adding do-not-merge/release-note-label-needed because the release note process has not been followed. One of the following labels is required "release-note", "release-note-action-required", or "release-note-none". 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. |
// In particular, the Unmount method uses spec.Name() as the volumeSpecName | ||
// in the mount path: | ||
// /var/lib/kubelet/pods/{podUID}/volumes/{escapeQualifiedPluginName}/{volumeSpecName}/ | ||
spec *volume.Spec |
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.
Could we name this volumeSpec? Not to be confused with pod spec
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 agree. To improve readability we should name this volumeSpec
. Currently the code reads - mountedPod.Spec
which almost sounds like it is pod spec.
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 followed the name from desired state data structure. If preferred, I can change in both places?
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 am happy to have just this one renamed to volumeSpec
but your call.
62a2ae3
to
5da43cc
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.
Mostly looks good. But one potential problem is - we have Spec
stored in both mountedPod
and attachedVolume
field and it is not clear which one should be used when. Obviously if you read the code carefully, one can see that mountedPod.Spec
is not available after volume is unmounted (but device is still mounted).
We should file a follow up github issue to clean it up.
@@ -713,7 +724,7 @@ func getMountedVolume( | |||
MountedVolume: operationexecutor.MountedVolume{ | |||
PodName: mountedPod.podName, | |||
VolumeName: attachedVolume.volumeName, | |||
InnerVolumeSpecName: attachedVolume.spec.Name(), | |||
InnerVolumeSpecName: mountedPod.spec.Name(), |
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 struct contains volume.Spec
too derived from attachedVolume
field. So potentially we are carrying around invalid spec all the time?
Can we replace Spec
in MountedVolume
to mountedPod.Spec
?
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.
yes, I think we should use mountedPod.Spec
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.
Are you planning to replace this or as part of follow up PR?
/retest |
/test pull-kubernetes-e2e-gce |
@gnufied @saad-ali I also manually tested this PR. To reproduce the issue, I add a delay during volume unmount so the first volume will be still in actual state when the second volume is added to cause the volume spec conflict. Without this fix, the test will always fail. While with this PR, the test can pass. |
@jingxu97 the commit still has "WIP:xxx" in it btw - can you reword it? |
5da43cc
to
8fd9bfd
Compare
@gnufied I removed the WIP. PTAL. Thanks! |
OuterVolumeSpecName: mountedPod.outerVolumeSpecName, | ||
PluginName: attachedVolume.pluginName, | ||
PodUID: mountedPod.podUID, | ||
Mounter: mountedPod.mounter, | ||
BlockVolumeMapper: mountedPod.blockVolumeMapper, | ||
VolumeGidValue: mountedPod.volumeGidValue, | ||
VolumeSpec: attachedVolume.spec, | ||
VolumeSpec: mountedPod.volumeSpec, |
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.
Should we cover this with some simple unit tests? Such that - when someone adds 2 pods with same PD-name then returned mounted volumes should have correct values?
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 added unit test for this. PTAL. Thanks!
8fd9bfd
to
03d1d55
Compare
/lgtm |
03d1d55
to
6089c45
Compare
6089c45
to
bdf7040
Compare
@Random-Liu Could you please help approve this PR? Thanks! |
[MILESTONENOTIFIER] Milestone Removed From Pull Request Important: This pull request was missing labels required for the v1.10 milestone for more than 3 days: kind: Must specify exactly one of |
/test pull-kubernetes-e2e-gce |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied, jingxu97, Random-Liu, saad-ali 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 |
/retest Review the full test history for this PR. Silence the bot with an |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
will this fix be pciked over into release-1.10? |
It seems that block mode is unusable due to this, because devicePath will never appear in the container |
Other minor fixes included. This is predicated on: kubernetes/kubernetes#61549
Thanks @gnufied!! And yes, this needs to be cherry picked back to 1.10 |
@fabiand could you please describe in more details about what you mean "It seems that block mode is unusable due to this, because devicePath will never appear in the container" Thanks! |
@jingxu97 I might have been to quick it could actually be that we tested a version which did not have iSCSI support (openshift 3.9) - so let me check that before providing more evidence for my claim above. |
Other minor fixes included. This is predicated on: kubernetes/kubernetes#61549
…49-upstream-release-1.10 Add volume spec to mountedPod in actual state of world
Add volume spec into mountedPod data struct in the actual state of the
world.
Fixes issue #61248
Release note: