Support persistent volume usage for kubernetes running on Photon Controller platform #36133

luomiao · 2016-11-03T05:35:36Z

What this PR does / why we need it:
Enable the persistent volume usage for kubernetes running on Photon platform.
Photon Controller: https://vmware.github.io/photon-controller/

Only the first commit include the real code change.
The following commits are for third-party vendor dependency and auto-generated code/docs updating.

Two components are added:
pkg/cloudprovider/providers/photon: support Photon Controller as cloud provider
pkg/volume/photon_pd: support Photon persistent disk as volume source for persistent volume

Usage introduction:
a. Photon Controller is supported as cloud provider.
When choosing to use photon controller as a cloud provider, "--cloud-provider=photon --cloud-config=[path_to_config_file]" is required for kubelet/kube-controller-manager/kube-apiserver. The config file of Photon Controller should follow the following usage:

[Global]
target = http://[photon_controller_endpoint_IP]
ignoreCertificate = true
tenant = [tenant_name]
project = [project_name]
overrideIP = true

b. Photon persistent disk is supported as volume source/persistent volume source.
yaml usage:

volumes:
  - name: photon-storage-1
    photonPersistentDisk:
        pdID: "643ed4e2-3fcc-482b-96d0-12ff6cab2a69"

pdID is the persistent disk ID from Photon Controller.

c. Enable Photon Controller as volume provisioner.
yaml usage:

kind: StorageClass
apiVersion: storage.k8s.io/v1beta1
metadata:
  name: gold_sc
provisioner: kubernetes.io/photon-pd
parameters:
  flavor: persistent-disk-gold

The flavor "persistent-disk-gold" needs to be created by Photon platform admin before hand.

This change is

luomiao · 2016-11-03T07:03:46Z

@pdhamdhere @abrarshivani @AlainRoy @kerneltime
This change has been reviewed by the above contributors within vmware/kubernetes fork.

k8s-ci-robot · 2016-11-03T19:28:25Z

Jenkins Kubemark GCE e2e failed for commit 2688fed3f4a8475596b437bdd5c40caa74110478. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-03T20:36:27Z

Jenkins GCE e2e failed for commit 2688fed3f4a8475596b437bdd5c40caa74110478. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

kerneltime · 2016-11-03T21:50:36Z

cc @saad-ali we have reviewed the changes internally, let us know how we can help. Ref: vmware-archive#21

k8s-ci-robot · 2016-11-03T21:55:28Z

Jenkins GCI GCE e2e failed for commit 3326b6b7a8c5af592ca7c173d9391398ab9c90d0. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

brendandburns · 2016-11-03T23:11:53Z

pkg/api/validation/validation.go

@@ -1159,6 +1175,14 @@ func ValidatePersistentVolume(pv *api.PersistentVolume) field.ErrorList {
 			allErrs = append(allErrs, validateVsphereVolumeSource(pv.Spec.VsphereVolume, specPath.Child("vsphereVolume"))...)
 		}
 	}
+	if pv.Spec.PhotonPersistentDisk != nil {


This code is duplicated, perhaps extract out as a single function?

It's not 100% duplicated though... one is using fldPath and the other is using specPath. And I am trying to be consistent with the context code.
Maybe a separate commit just for code-cleanup to shorten all the duplicated code in this block would be a better solution?

brendandburns · 2016-11-03T23:13:51Z

pkg/cloudprovider/providers/photon/photon.go

+func getVMIDbyNodename(project string, nodeName string) (string, error) {
+	vmList, err := photonClient.Projects.GetVMs(project, nil)
+	if err != nil {
+		return "", fmt.Errorf("Failed to GetVMs from project %s with nodeName %s, error: [%v]", project, nodeName, err)


I'd prefer not wrapping errors, just return it. If you feel the need, log it as well.

This and all the following wrapping errors are addressed.

brendandburns

Overall structure seems ok, though there are a bunch of cleanups/refactors.

Plus it needs a rebase.

brendandburns · 2016-11-03T23:14:49Z

pkg/cloudprovider/providers/photon/photon.go

+
+	for _, vm := range vmList.Items {
+		if vm.Name == nodeName {
+			if vm.State == "STARTED" {


This is a magic string. Extract as a const and add a comment to explain what it means. (is this string not in the API SDK somewhere?)

brendandburns · 2016-11-03T23:15:20Z

pkg/cloudprovider/providers/photon/photon.go

+func getVMIDbyIP(project string, IPAddress string) (string, error) {
+	vmList, err := photonClient.Projects.GetVMs(project, nil)
+	if err != nil {
+		return "", fmt.Errorf("Failed to GetVMs for project %s, error [%v]", project, err)


as above wrt to wrapping errors.

brendandburns · 2016-11-03T23:16:12Z

pkg/cloudprovider/providers/photon/photon.go

+func getProjIDbyName(tenantName, projName string) (string, error) {
+	tenants, err := photonClient.Tenants.GetAll()
+	if err != nil {
+		return "", fmt.Errorf("GetAll tenants failed with error [%v]", err)


brendandburns · 2016-11-03T23:16:37Z

pkg/cloudprovider/providers/photon/photon.go

+		if tenant.Name == tenantName {
+			projects, err := photonClient.Tenants.GetProjects(tenant.ID, nil)
+			if err != nil {
+				return "", fmt.Errorf("Failed to GetProjects for tenant %s, error [%v]", tenantName, err)


And here, I'm going to stop commenting, but please just return err everywhere.

brendandburns · 2016-11-03T23:17:13Z

pkg/cloudprovider/providers/photon/photon.go

+	}
+
+	//TODO: add handling of certification enabled situation
+	options := &photon.ClientOptions{


Does this mean it is insecure? Please comment on what this means (and make sure it's in the docs too)

brendandburns · 2016-11-03T23:22:56Z

pkg/cloudprovider/providers/photon/photon.go

+func logError(msg string, err error) error {
+	s := "Photon Cloud Provider: " + msg + ". Error [" + err.Error() + "]"
+	glog.Errorf(s)
+	return fmt.Errorf(s)


please don't wrap the error, just return the error.

brendandburns · 2016-11-03T23:24:20Z

pkg/volume/photon_pd/attacher.go

+var _ volume.AttachableVolumePlugin = &photonPersistentDiskPlugin{}
+
+// Singleton key mutex for keeping attach operations for the same host atomic
+var attachdetachMutex = keymutex.NewKeyMutex()


Rather than use a global variable, why not just always return a singleton from NewAttacher and have that singleton own the lock?

Have double-checked with Photon Controller team and the attach/detach mutex is taken care inside Photon Controller, so we are safe to remove them here.

brendandburns · 2016-11-03T23:32:56Z

pkg/volume/photon_pd/attacher.go

+}
+
+func (detacher *photonPersistentDiskDetacher) WaitForDetach(devicePath string, timeout time.Duration) error {
+	ticker := time.NewTicker(checkSleepDuration)


You should really just use util.Poll here.

https://github.com/kubernetes/kubernetes/blob/master/pkg/util/wait/wait.go#L161

brendandburns · 2016-11-03T23:33:24Z

pkg/volume/photon_pd/attacher.go

+	// scan scsi path to discover the new disk
+	scsiHostScan()
+
+	ticker := time.NewTicker(checkSleepDuration)


you should use util.Poll here

https://github.com/kubernetes/kubernetes/blob/master/pkg/util/wait/wait.go#L161

Thank you! this looks a better solution. This code again is trying to keep consistent with other volume plugins. I will work on to simplify this part of code in a separate PR for all the volume plugin.

brendandburns · 2016-11-03T23:36:09Z

pkg/volume/photon_pd/photon_util.go

+)
+
+var ErrProbeVolume = errors.New("Error scanning attached volumes")
+var volNameToDeviceName = make(map[string]string)


Why is this a global? That seems like a bad idea.

This is because the detacher doesn't have information about the disk ID. It only has information of volName. So we have to keep a global mapping in order to find out which device to be removed from scsi path. This will be gone when Photon start to use pvscsi controller instead of scsi controller.

luomiao · 2016-11-04T02:35:45Z

@brendandburns
Hi Brendan,
I have incorporated most of your comments and please see otherwise reply inline.
Please let me know if you have further suggestions. Thank you!

brendandburns · 2016-11-04T23:14:54Z

LGTM. The risk is low, this is largely factored outside of the main code-paths.

k8s-ci-robot · 2016-11-06T04:05:44Z

Jenkins verification failed for commit be20162540b61ee87ef32bdbad67f08f2443373d. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-07T07:55:27Z

Jenkins unit/integration failed for commit e3082e8adc45937eed8fee9f7e65e88ca321f690. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

saad-ali · 2016-11-08T06:06:05Z

Release-czar approved post-code freeze merge--This was LGTMed and in the merge-queue at code freeze time for 1.5. Adding 1.5 milestone to let it gets merged after code freeze.

1. Enable Photon Controller as cloud provider 2. Support Photon persistent disk as volume source/persistent volume source

k8s-ci-robot · 2016-11-08T18:36:37Z

Jenkins GCI GKE smoke e2e failed for commit 20b9fc6. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

luomiao · 2016-11-08T18:47:23Z

@k8s-bot gci gke e2e test this

k8s-github-robot · 2016-11-09T07:27:05Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-11-09T08:10:22Z

Automatic merge from submit-queue

mengqiy · 2016-12-08T19:13:44Z

pkg/api/v1/types.go

@@ -405,6 +407,8 @@ type PersistentVolumeSource struct {
 	// AzureDisk represents an Azure Data Disk mount on the host and bind mount to the pod.
 	// +optional
 	AzureDisk *AzureDiskVolumeSource `json:"azureDisk,omitempty" protobuf:"bytes,16,opt,name=azureDisk"`
+	// PhotonPersistentDisk represents a PhotonController persistent disk attached and mounted on kubelets host machine
+	PhotonPersistentDisk *PhotonPersistentDiskVolumeSource `json:"photonPersistentDisk,omitempty" protobuf:"bytes,17,opt,name=photonPersistentDisk"`


@mbohlool Should we have // +optional here?

Same question for pkg/api/types.go

googlebot added the cla: yes label Nov 3, 2016

k8s-github-robot assigned brendandburns Nov 3, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/old-docs size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 3, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2016

luomiao force-pushed the photon-support-PR-v2 branch from eda6ed7 to b212091 Compare November 3, 2016 15:46

k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/old-docs labels Nov 3, 2016

luomiao force-pushed the photon-support-PR-v2 branch from 658392d to 2688fed Compare November 3, 2016 19:00

luomiao force-pushed the photon-support-PR-v2 branch from 2688fed to 3326b6b Compare November 3, 2016 20:36

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2016

brendandburns reviewed Nov 3, 2016

View reviewed changes

brendandburns suggested changes Nov 3, 2016

View reviewed changes

luomiao force-pushed the photon-support-PR-v2 branch from 3326b6b to 861e974 Compare November 4, 2016 01:45

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2016

luomiao force-pushed the photon-support-PR-v2 branch from 861e974 to e61ea39 Compare November 4, 2016 03:40

brendandburns added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Nov 4, 2016

brendandburns approved these changes Nov 4, 2016

View reviewed changes

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2016

luomiao force-pushed the photon-support-PR-v2 branch from be20162 to a3625f5 Compare November 6, 2016 03:53

brendandburns added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2016

luomiao force-pushed the photon-support-PR-v2 branch from a3625f5 to e3082e8 Compare November 7, 2016 07:38

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 7, 2016

luomiao force-pushed the photon-support-PR-v2 branch from e3082e8 to 5ac6fdd Compare November 7, 2016 08:10

luomiao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2016

saad-ali added this to the v1.5 milestone Nov 8, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2016

Miao Luo added 5 commits November 8, 2016 09:36

Support persistent volume on Photon Controller platform

b22ccc6

1. Enable Photon Controller as cloud provider 2. Support Photon persistent disk as volume source/persistent volume source

Photon Controller platform vendor code

a7fc0e4

Autogenerated files update due to new API changes for photon volume

96f88f6

Fix verification error due to API version update.

496d229

Photon Controller support: Address github code review comments.

20b9fc6

luomiao force-pushed the photon-support-PR-v2 branch from 5ac6fdd to 20b9fc6 Compare November 8, 2016 17:58

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 8, 2016

saad-ali added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2016

k8s-github-robot merged commit b3e4083 into kubernetes:master Nov 9, 2016

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

mengqiy reviewed Dec 8, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support persistent volume usage for kubernetes running on Photon Controller platform #36133

Support persistent volume usage for kubernetes running on Photon Controller platform #36133

luomiao commented Nov 3, 2016 •

edited

Loading

luomiao commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

kerneltime commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

brendandburns Nov 3, 2016

luomiao Nov 3, 2016

brendandburns Nov 3, 2016

luomiao Nov 4, 2016

brendandburns left a comment

brendandburns Nov 3, 2016

brendandburns Nov 3, 2016

brendandburns Nov 3, 2016

brendandburns Nov 3, 2016

brendandburns Nov 3, 2016

brendandburns Nov 3, 2016

brendandburns Nov 3, 2016

luomiao Nov 4, 2016

brendandburns Nov 3, 2016

brendandburns Nov 3, 2016

luomiao Nov 4, 2016

brendandburns Nov 3, 2016

luomiao Nov 4, 2016

luomiao commented Nov 4, 2016 •

edited

Loading

brendandburns commented Nov 4, 2016

k8s-ci-robot commented Nov 6, 2016

k8s-ci-robot commented Nov 7, 2016

saad-ali commented Nov 8, 2016

k8s-ci-robot commented Nov 8, 2016

luomiao commented Nov 8, 2016

k8s-github-robot commented Nov 9, 2016

k8s-github-robot commented Nov 9, 2016

mengqiy Dec 8, 2016

mengqiy Dec 8, 2016

Support persistent volume usage for kubernetes running on Photon Controller platform #36133

Support persistent volume usage for kubernetes running on Photon Controller platform #36133

Conversation

luomiao commented Nov 3, 2016 • edited Loading

luomiao commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

kerneltime commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luomiao commented Nov 4, 2016 • edited Loading

brendandburns commented Nov 4, 2016

k8s-ci-robot commented Nov 6, 2016

k8s-ci-robot commented Nov 7, 2016

saad-ali commented Nov 8, 2016

k8s-ci-robot commented Nov 8, 2016

luomiao commented Nov 8, 2016

k8s-github-robot commented Nov 9, 2016

k8s-github-robot commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luomiao commented Nov 3, 2016 •

edited

Loading

luomiao commented Nov 4, 2016 •

edited

Loading