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

update aws plugin for block support #58625

Merged
merged 1 commit into from Feb 27, 2018

Conversation

@screeley44
Contributor

screeley44 commented Jan 22, 2018

Update aws plugin to support block and volumeMode.

cc @jsafrane @mtanino @msau42

AWS EBS volume plugin got block volume support
@screeley44

This comment has been minimized.

Contributor

screeley44 commented Jan 22, 2018

I still need to add some unit tests and probably a feature gate check, but wanted to start to get some eyes on this...

@mtanino

This comment has been minimized.

Member

mtanino commented Jan 22, 2018

@screeley44

This comment has been minimized.

Contributor

screeley44 commented Jan 23, 2018

/sig storage

@screeley44

This comment has been minimized.

Contributor

screeley44 commented Jan 23, 2018

/test pull-kubernetes-e2e-kops-aws

@jsafrane

Nothing obviously wrong, it seems it goes the right direction.

return path.Join(host.GetVolumeDevicePluginDir(awsElasticBlockStorePluginName), vID)
}
func getVolumeIDFromGlobalVDPVPath(globalPath string) (string, error) {

This comment has been minimized.

@jsafrane

jsafrane Jan 25, 2018

Member

Reading the code, it seems that filepath.Base() does something very similar.

This comment has been minimized.

@screeley44

screeley44 Jan 25, 2018

Contributor

ack

}
// make a directory like /var/lib/kubelet/plugins/kubernetes.io/aws-ebs/volumeDevices/volumeID
func makeVDPDNameInternal(host volume.VolumeHost, vID string) string {

This comment has been minimized.

@jsafrane

jsafrane Jan 25, 2018

Member

This function is called only once, should it be directly in ebsGlobalMapPath or even GetGlobalMapPath?

This comment has been minimized.

@screeley44

screeley44 Jan 25, 2018

Contributor

ack - cleaned up extra calls and consolidated stepped functions

@screeley44 screeley44 changed the title from WIP: update aws plugin for block support to update aws plugin for block support Jan 25, 2018

@jsafrane

This comment has been minimized.

Member

jsafrane commented Jan 29, 2018

/assign @mtanino
/approve no-issue

volName: volName,
manager: manager,
plugin: plugin,
}}, nil

This comment has been minimized.

@mtanino

mtanino Jan 30, 2018

Member

Unmapper must have volumeID because this value will be used at GetGlobalMapPath().

This comment has been minimized.

@screeley44

screeley44 Jan 30, 2018

Contributor

@mtanino - ack, I implemented a way to get volumeID, take a look when you get a chance

func findVolumeIDFromPath(podUID string, pluginDir string) string {
files, err := ioutil.ReadDir(pluginDir)
if err != nil {
return ""

This comment has been minimized.

@mtanino

mtanino Jan 30, 2018

Member

This method should return error if something is wrong instead of returning empty string, otherwise you'll hit another error when you use volumeID like removing globalMapPath.

// GetGlobalMapPath returns global map path and error
// path: plugins/kubernetes.io/{PluginName}/volumeDevices/volumeID/volName/
// plugins/kubernetes.io/aws-ebs/volumeDevices/vol-XXXXXX/myebsvolume/
func (ebs *awsElasticBlockStore) GetGlobalMapPath(spec *volume.Spec) (string, error) {

This comment has been minimized.

@mtanino

mtanino Jan 31, 2018

Member

@screeley44

From my debug result, global mount path(filesystem volume) is corresponding to provisioning type.

  • Static provisioning
/var/lib/kubelet/plugins/kubernetes.io/aws-ebs/mounts/vol-079033653d3945a51

In this case, volumeID is vol-0b2360c4480ad10f4

  • Dynamic provisioning
/var/lib/kubelet/plugins/kubernetes.io/aws-ebs/mounts/aws/us-east-2c/vol-0b2360c4480ad10f4

In this case, volumeID is aws/us-east-2c/vol-0b2360c4480ad10f4.
Provisioner add aws and volume region into volumeID.

But either case, the global mount path is /var/lib/kubelet/plugins/kubernetes.io/aws-ebs/mounts/{volumeID}.

Therefore, we should use same format for Block volume case.
Supposed global map path for Block volume is;

  • /var/lib/kubelet/plugins/kubernetes.io/aws-ebs/volumeDevices/{volumeID}

And then under {volumeID} dir we will have symbolic links whose name is pod's UUID.
So this method should be like this;

 func (ebs *awsElasticBlockStore) GetGlobalMapPath(spec *volume.Spec) (string, error) {
       volumeSource, _, err := getVolumeSource(spec)
       if err != nil {
               return "", err
       }
       return path.Join(ebs.plugin.host.GetVolumeDevicePluginDir(awsElasticBlockStorePluginName), string(volumeSource.VolumeID)), nil
 }

This comment has been minimized.

@screeley44

screeley44 Feb 1, 2018

Contributor

ack - backed out all the volName changes and fixed GetGlobalMapMapth based on spec volumeID.

if err != nil {
return nil, err
}
glog.V(5).Infof("globalMapPathUUID: %s, err: %v", globalMapPathUUID, err)

This comment has been minimized.

@mtanino

mtanino Feb 1, 2018

Member

err is always nil. We needn't log the err here.

manager: manager,
mounter: mounter,
plugin: plugin,
MetricsProvider: volume.NewMetricsStatFS(getPath(podUID, spec.Name(), plugin.host)),

This comment has been minimized.

@gnufied

gnufied Feb 2, 2018

Member

Does statfs syscall work on raw block devices?

@gnufied

This comment has been minimized.

Member

gnufied commented Feb 2, 2018

Have we started adding some e2e tests for block devices? if yes - lets add some feature gated(i.e [Feature:BlockVolume] e2e tests for EBS.

@@ -241,6 +242,150 @@ func (plugin *awsElasticBlockStorePlugin) ConstructVolumeSpec(volName, mountPath
return volume.NewSpecFromVolume(awsVolume), nil
}

This comment has been minimized.

@gnufied

gnufied Feb 2, 2018

Member

It will be nice to have all the block volume support for EBS functions/interfaces - moved to a separate file called ebs_block_device.go or something like that

@screeley44

This comment has been minimized.

Contributor

screeley44 commented Feb 2, 2018

blocked for now until #58177 is implemented.

devicePath is empty for block volumes during reonstruction Map() and Unmap() and should be fixed in above PR

@screeley44

This comment has been minimized.

Contributor

screeley44 commented Feb 13, 2018

/test pull-kubernetes-e2e-kops-aws

@screeley44

This comment has been minimized.

Contributor

screeley44 commented Feb 13, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 20, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, screeley44

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

@screeley44

This comment has been minimized.

Contributor

screeley44 commented Feb 23, 2018

/test pull-kubernetes-e2e-gce

@screeley44

This comment has been minimized.

Contributor

screeley44 commented Feb 23, 2018

/test pull-kubernetes-unit

@jsafrane

This comment has been minimized.

Member

jsafrane commented Feb 26, 2018

/lgtm

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 27, 2018

Automatic merge from submit-queue (batch tested with PRs 60427, 60361, 60364, 58625, 60187). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 6eabef4 into kubernetes:master Feb 27, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation screeley44 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment