-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 support for subpath in play kube for named volumes #16803
Conversation
2b098ca
to
eade91e
Compare
@vrothberg @umohnani8 @mheon PTAL |
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
@umohnani8 @ygalblum PTAL
@@ -254,6 +254,8 @@ type ContainerNamedVolume struct { | |||
// IsAnonymous sets the named volume as anonymous even if it has a name | |||
// This is used for emptyDir volumes from a kube yaml | |||
IsAnonymous bool `json:"setAnonymous,omitempty"` | |||
// SubPath determines which part of the Source will be mounted in the container | |||
SubPath string |
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.
Does this need to be stored? Or is this only in effect for the length of the kube play
? Not sure this needs to be plumbed this deeply?
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 think it does since in generateSpec (which play kube eventually calls) we use ContainerNamedVolume
to generate the mountpoint and we need to subpath to get the source properly.
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.
Shouldn't we just put the full path (path + subpath) into the named volume as Path?
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.
Oh, wait, I see the problem there, different containers will want to index into the volume at different places.
subpath allows for only a subdirecty of a volumes data to be mounted in the container add support for the named volume type sub path with others to follow. resolves containers#12929 Signed-off-by: Charlie Doern <[email protected]>
- sleep | ||
- inf | ||
volumeMounts: | ||
- mountPath: /var |
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.
Does this work with other types of volume mount? HostPath for example?
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.
Not in this PR, probably won't be too hard though. I think I just need to edit all of the appropriate specgen structs.
Should we expose Subpath via |
I agree, but should it be a separate PR? |
OK, separate PR is fine. Maybe make a GH issue so we don't forget about it? Code LGTM |
/hold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoern, mheon 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 |
LGTM |
subpath allows for only a subdirecty of a volumes data to be mounted in the container add support for the named volume type sub path with others to follow.
resolves #12929
Signed-off-by: Charlie Doern [email protected]
Does this PR introduce a user-facing change?