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

CSI - Implementation for Inline Volume Support #68232

Closed
wants to merge 2 commits into
base: master
from

Conversation

@vladimirvivien
Copy link
Member

vladimirvivien commented Sep 4, 2018

What this PR does / why we need it:
This PR is the implementation of the API changes and CSI code needed to support CSI inline volume.

See also

Release note:

Adds support for CSI inline volumes embedded in pod specs.

/SIG storage

⚠️ Closing this PR ⚠️
The new design contains substantial changes. Closing this PR to avoid carrying around massive amount of old irrelevant and unrelated comments. PR for new API changes is #74086.

@k8s-ci-robot k8s-ci-robot requested review from deads2k and ericchiang Sep 4, 2018

@vladimirvivien vladimirvivien changed the title CSI Inline Volume in-tree Implementation CSI - Implementation for Inline Volume Support Sep 4, 2018

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

vladimirvivien commented Sep 4, 2018

/assign

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

vladimirvivien commented Sep 4, 2018

/status approved-for-milestone

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 4, 2018

You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to add status labels.

Show resolved Hide resolved pkg/apis/core/types.go Outdated
@kfox1111

This comment has been minimized.

Copy link

kfox1111 commented Sep 5, 2018

still gonna make it for 1.12?

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-volume-impl branch from 304da26 to ee4cfc4 Sep 5, 2018

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-volume-impl branch from ee4cfc4 to b694ca1 Sep 7, 2018

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-volume-impl branch 2 times, most recently from 9771bb6 to b37de8c Sep 9, 2018

@kfox1111

This comment has been minimized.

Copy link

kfox1111 commented Oct 8, 2018

Is this possible for 1.13?

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-volume-impl branch from b37de8c to d7d0407 Oct 9, 2018

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-volume-impl branch from 3bfc9eb to ce827da Nov 13, 2018

@davidz627
Copy link
Contributor

davidz627 left a comment

just reviewed the non-test implementation commit

driverName = pvSource.Driver
skip, err := c.plugin.skipAttach(driverName)
if err != nil {
klog.Error(log("attacher.Attach failed to find if driver is attachable: %v", err))

This comment has been minimized.

@davidz627

davidz627 Nov 13, 2018

Contributor

nit: I don't think we should be logging errors everytime right before we return an error. If we return the error the caller should be logging it somewhere

if c.plugin.inlineFeatureEnabled && volSource != nil {
driverName = volSource.Driver
if volSource.VolumeHandle != nil {
volumeHandle = *volSource.VolumeHandle

This comment has been minimized.

@davidz627

davidz627 Nov 13, 2018

Contributor

why is this one a pointer that needs a nil check whereas the volumeHandle in pvSource is just a string

return "", nil

// post attachment object and wait for attachment to be created
createdAttchID, err := c.postVolumeAttachment(driverName, volumeHandle, attachment, csiDefaultTimeout)

This comment has been minimized.

@davidz627

davidz627 Nov 13, 2018

Contributor

nit: spelling createdAttachID

return "", nil

// post attachment object and wait for attachment to be created
createdAttchID, err := c.postVolumeAttachment(driverName, volumeHandle, attachment, csiDefaultTimeout)

This comment has been minimized.

@davidz627

davidz627 Nov 13, 2018

Contributor

post is an overloaded word and means something fairly specific. Is that what we're doing? Or is this more of a CreateVolumeAttachment type deal

This comment has been minimized.

@kfox1111

kfox1111 Nov 14, 2018

Yeah, I think this is a create volume attachment thing. post as in http method=post.

klog.V(4).Info(log("attacher.WaitForAttach called [attachment.ID=%v]", attachID))
if spec == nil {
klog.Error(log("attacher.WaitForAttach missing volume.Spec"))
return "", errors.New("missing spec")

This comment has been minimized.

@davidz627

davidz627 Nov 13, 2018

Contributor

I would prefer we don't log errors here again, and please put the better error message into the error that is returned, this will then get logged by the caller

}

func (p *csiPlugin) CanSupport(spec *volume.Spec) bool {
// TODO (vladimirvivien) CanSupport should also take into account
// the availability/registration of specified Driver in the volume source
return spec.PersistentVolume != nil && spec.PersistentVolume.Spec.CSI != nil
if p.inlineFeatureEnabled {
return (spec.PersistentVolume != nil && spec.PersistentVolume.Spec.CSI != nil) ||

This comment has been minimized.

@davidz627

davidz627 Nov 13, 2018

Contributor

if we're doing inline why are we checking whether PersistentVolume is not nil either? We specifically want Persistent volume to be nil actually?

sourceKind string
)

if volSource != nil && p.inlineFeatureEnabled {

This comment has been minimized.

@davidz627

davidz627 Nov 13, 2018

Contributor

also could use that common function

Show resolved Hide resolved pkg/volume/csi/csi_plugin.go Outdated
Show resolved Hide resolved pkg/volume/csi/csi_plugin.go Outdated
var spec *volume.Spec

if p.inlineFeatureEnabled && volData[volDataKey.sourceKind] == volumeKind {
handle := volData[volDataKey.volHandle]

This comment has been minimized.

@davidz627

davidz627 Nov 13, 2018

Contributor

unclear to me why we're using this volData map instead of passing variables around. This seems much more brittle, especially since nowhere in this function do we check for the values in the map already existing.

If we MUST use the map, we should do nil checks and fail fast if the values are not there

@davidz627
Copy link
Contributor

davidz627 left a comment

I am not as familiar with API's so someone else should review this, but I have added some simple comments

@liggitt liggitt modified the milestones: v1.13, v1.14 Nov 16, 2018

@kfox1111

This comment has been minimized.

Copy link

kfox1111 commented Dec 11, 2018

@rthallisey pr that I think will interest kubevirt for container images.

@vladimirvivien vladimirvivien referenced this pull request Feb 11, 2019

Open

CSI Inline Volume Implementation Umbrella Issue #73929

4 of 12 tasks complete

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-volume-impl branch from ce827da to da710bc Feb 12, 2019

Show resolved Hide resolved pkg/api/pod/util.go Outdated

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-volume-impl branch from da710bc to 25abe18 Feb 14, 2019

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/XXL labels Feb 14, 2019

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-volume-impl branch from 25abe18 to 372f871 Feb 14, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 14, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vladimirvivien
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp

If they are not already assigned, you can assign the PR to them by writing /assign @lavalamp in a comment when ready.

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

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-volume-impl branch from 372f871 to f5597a3 Feb 14, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 14, 2019

@vladimirvivien: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-bazel-test f5597a3 link /test pull-kubernetes-bazel-test
pull-kubernetes-verify f5597a3 link /test pull-kubernetes-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

vladimirvivien commented Feb 15, 2019

⚠️ Closing this PR ⚠️

Apologies for confusion. Closing this PR because of a lot of outdated comments and due to changes in implementation. Moving API review here #74086

⚠️ Closing this PR ⚠️

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

vladimirvivien commented Feb 15, 2019

@msau42 I will address all of your comments #74086

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

vladimirvivien commented Feb 15, 2019

⚠️ Closing this PR ⚠️

Closing this PR because of a lot of old outdated comments due to changes in implementation. Moving API changes here #74086

⚠️ Closing this PR ⚠️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment