vSphere Cloud Provider code refactoring

pkg/cloudprovider/providers/vsphere/BUILD

@@ -12,32 +12,24 @@ go_library(
     name = "go_default_library",
     srcs = [
         "vsphere.go",
-        "vsphere_metrics.go",
         "vsphere_util.go",
     ],
     tags = ["automanaged"],
     deps = [
         "//pkg/api/v1/helper:go_default_library",
         "//pkg/cloudprovider:go_default_library",
+        "//pkg/cloudprovider/providers/vsphere/vclib:go_default_library",
+        "//pkg/cloudprovider/providers/vsphere/vclib/diskmanagers:go_default_library",
         "//pkg/controller:go_default_library",
         "//vendor/github.com/golang/glog:go_default_library",
-        "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
         "//vendor/github.com/vmware/govmomi:go_default_library",
-        "//vendor/github.com/vmware/govmomi/find:go_default_library",
         "//vendor/github.com/vmware/govmomi/object:go_default_library",
-        "//vendor/github.com/vmware/govmomi/pbm:go_default_library",
-        "//vendor/github.com/vmware/govmomi/pbm/types:go_default_library",
-        "//vendor/github.com/vmware/govmomi/property:go_default_library",
-        "//vendor/github.com/vmware/govmomi/session:go_default_library",
         "//vendor/github.com/vmware/govmomi/vim25:go_default_library",
         "//vendor/github.com/vmware/govmomi/vim25/mo:go_default_library",
-        "//vendor/github.com/vmware/govmomi/vim25/soap:go_default_library",
-        "//vendor/github.com/vmware/govmomi/vim25/types:go_default_library",
         "//vendor/golang.org/x/net/context:go_default_library",
         "//vendor/gopkg.in/gcfg.v1:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
     ],
 )
 
@@ -48,6 +40,7 @@ go_test(
     tags = ["automanaged"],
     deps = [
         "//pkg/cloudprovider:go_default_library",
+        "//pkg/cloudprovider/providers/vsphere/vclib:go_default_library",
         "//vendor/golang.org/x/net/context:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/rand:go_default_library",
@@ -63,6 +56,9 @@ filegroup(
 
 filegroup(
     name = "all-srcs",
-    srcs = [":package-srcs"],
+    srcs = [
+        ":package-srcs",
+        "//pkg/cloudprovider/providers/vsphere/vclib:all-srcs",
+    ],
     tags = ["automanaged"],
 )

pkg/cloudprovider/providers/vsphere/vsphere_metrics.go

-/*
-Copyright 2017 The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package vsphere
-
-import (
-	"github.com/prometheus/client_golang/prometheus"
-	"time"
-)
-
-const (
-	api_createvolume = "CreateVolume"
-	api_deletevolume = "DeleteVolume"
-	api_attachvolume = "AttachVolume"
-	api_detachvolume = "DetachVolume"
-)
-
-const (
-	operation_deletevolume                      = "DeleteVolumeOperation"
-	operation_attachvolume                      = "AttachVolumeOperation"
-	operation_detachvolume                      = "DetachVolumeOperation"
-	operation_diskIsAttached                    = "DiskIsAttachedOperation"
-	operation_disksAreAttached                  = "DisksAreAttachedOperation"
-	operation_createvolume                      = "CreateVolumeOperation"
-	operation_createvolume_with_policy          = "CreateVolumeWithPolicyOperation"
-	operation_createvolume_with_raw_vsan_policy = "CreateVolumeWithRawVSANPolicyOperation"
-)
-
-// vsphereApiMetric is for recording latency of Single API Call.
-var vsphereApiMetric = prometheus.NewHistogramVec(
-	prometheus.HistogramOpts{
-		Name: "cloudprovider_vsphere_api_request_duration_seconds",
-		Help: "Latency of vsphere api call",
-	},
-	[]string{"request"},
-)
-
-var vsphereApiErrorMetric = prometheus.NewCounterVec(
-	prometheus.CounterOpts{
-		Name: "cloudprovider_vsphere_api_request_errors",
-		Help: "vsphere Api errors",
-	},
-	[]string{"request"},
-)
-
-// vsphereOperationMetric is for recording latency of vSphere Operation which invokes multiple APIs to get the task done.
-var vsphereOperationMetric = prometheus.NewHistogramVec(
-	prometheus.HistogramOpts{
-		Name: "cloudprovider_vsphere_operation_duration_seconds",
-		Help: "Latency of vsphere operation call",
-	},
-	[]string{"operation"},
-)
-
-var vsphereOperationErrorMetric = prometheus.NewCounterVec(
-	prometheus.CounterOpts{
-		Name: "cloudprovider_vsphere_operation_errors",
-		Help: "vsphere operation errors",
-	},
-	[]string{"operation"},
-)
-
-func registerMetrics() {
-	prometheus.MustRegister(vsphereApiMetric)
-	prometheus.MustRegister(vsphereApiErrorMetric)
-	prometheus.MustRegister(vsphereOperationMetric)
-	prometheus.MustRegister(vsphereOperationErrorMetric)
-}
-
-func recordvSphereMetric(actionName string, requestTime time.Time, err error) {
-	switch actionName {
-	case api_createvolume, api_deletevolume, api_attachvolume, api_detachvolume:
-		recordvSphereAPIMetric(actionName, requestTime, err)
-	default:
-		recordvSphereOperationMetric(actionName, requestTime, err)
-	}
-}
-
-func recordvSphereAPIMetric(actionName string, requestTime time.Time, err error) {
-	if err != nil {
-		vsphereApiErrorMetric.With(prometheus.Labels{"request": actionName}).Inc()
-	} else {
-		vsphereApiMetric.With(prometheus.Labels{"request": actionName}).Observe(calculateTimeTaken(requestTime))
-	}
-}
-
-func recordvSphereOperationMetric(actionName string, requestTime time.Time, err error) {
-	if err != nil {
-		vsphereOperationErrorMetric.With(prometheus.Labels{"operation": actionName}).Inc()
-	} else {
-		vsphereOperationMetric.With(prometheus.Labels{"operation": actionName}).Observe(calculateTimeTaken(requestTime))
-	}
-}
-
-func recordCreateVolumeMetric(volumeOptions *VolumeOptions, requestTime time.Time, err error) {
-	var actionName string
-	if volumeOptions.StoragePolicyName != "" {
-		actionName = operation_createvolume_with_policy
-	} else if volumeOptions.VSANStorageProfileData != "" {
-		actionName = operation_createvolume_with_raw_vsan_policy
-	} else {
-		actionName = operation_createvolume
-	}
-	recordvSphereMetric(actionName, requestTime, err)
-}
-
-func calculateTimeTaken(requestBeginTime time.Time) (timeTaken float64) {
-	if !requestBeginTime.IsZero() {
-		timeTaken = time.Since(requestBeginTime).Seconds()
-	} else {
-		timeTaken = 0
-	}
-	return timeTaken
-}

pkg/cloudprovider/providers/vsphere/vsphere_test.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/kubernetes/pkg/cloudprovider"
+	"k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere/vclib"
 )
 
 func configFromEnv() (cfg VSphereConfig, ok bool) {
@@ -125,11 +126,11 @@ func TestVSphereLogin(t *testing.T) {
 	defer cancel()
 
 	// Create vSphere client
-	err = vSphereLogin(ctx, vs)
+	err = vs.conn.Connect(ctx)
 	if err != nil {
-		t.Errorf("Failed to create vSpere client: %s", err)
+		t.Errorf("Failed to connect to vSphere: %s", err)
 	}
-	defer vs.client.Logout(ctx)
+	defer vs.conn.GoVmomiClient.Logout(ctx)
 }
 
 func TestZones(t *testing.T) {
@@ -168,14 +169,14 @@ func TestInstances(t *testing.T) {
 		t.Fatalf("CurrentNodeName() failed: %s", err)
 	}
 
-	externalId, err := i.ExternalID(nodeName)
+	externalID, err := i.ExternalID(nodeName)
 	if err != nil {
 		t.Fatalf("Instances.ExternalID(%s) failed: %s", nodeName, err)
 	}
-	t.Logf("Found ExternalID(%s) = %s\n", nodeName, externalId)
+	t.Logf("Found ExternalID(%s) = %s\n", nodeName, externalID)
 
 	nonExistingVM := types.NodeName(rand.String(15))
-	externalId, err = i.ExternalID(nonExistingVM)
+	externalID, err = i.ExternalID(nonExistingVM)
 	if err == cloudprovider.InstanceNotFound {
 		t.Logf("VM %s was not found as expected\n", nonExistingVM)
 	} else if err == nil {
@@ -184,13 +185,13 @@ func TestInstances(t *testing.T) {
 		t.Fatalf("Instances.ExternalID did not fail as expected, err: %v", err)
 	}
 
-	instanceId, err := i.InstanceID(nodeName)
+	instanceID, err := i.InstanceID(nodeName)
 	if err != nil {
 		t.Fatalf("Instances.InstanceID(%s) failed: %s", nodeName, err)
 	}
-	t.Logf("Found InstanceID(%s) = %s\n", nodeName, instanceId)
+	t.Logf("Found InstanceID(%s) = %s\n", nodeName, instanceID)
 
-	instanceId, err = i.InstanceID(nonExistingVM)
+	instanceID, err = i.InstanceID(nonExistingVM)
 	if err == cloudprovider.InstanceNotFound {
 		t.Logf("VM %s was not found as expected\n", nonExistingVM)
 	} else if err == nil {
@@ -222,7 +223,7 @@ func TestVolumes(t *testing.T) {
 		t.Fatalf("CurrentNodeName() failed: %s", err)
 	}
 
-	volumeOptions := &VolumeOptions{
+	volumeOptions := &vclib.VolumeOptions{
 		CapacityKB: 1 * 1024 * 1024,
 		Tags:       nil,
 		Name:       "kubernetes-test-volume-" + rand.String(10),
@@ -233,7 +234,7 @@ func TestVolumes(t *testing.T) {
 		t.Fatalf("Cannot create a new VMDK volume: %v", err)
 	}
 
-	_, _, err = vs.AttachDisk(volPath, "", "")
+	_, err = vs.AttachDisk(volPath, "", "")
 	if err != nil {
 		t.Fatalf("Cannot attach volume(%s) to VM(%s): %v", volPath, nodeName, err)
 	}
@@ -249,36 +250,3 @@ func TestVolumes(t *testing.T) {
 	// 	t.Fatalf("Cannot delete VMDK volume %s: %v", volPath, err)
 	// }
 }
-
-func TestGetVMName(t *testing.T) {
-	cfg, ok := configFromEnv()
-	if !ok {
-		t.Skipf("No config found in environment")
-	}
-
-	// Create vSphere configuration object
-	vs, err := newVSphere(cfg)
-	if err != nil {
-		t.Fatalf("Failed to construct/authenticate vSphere: %s", err)
-	}
-
-	// Create context
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	// Create vSphere client
-	err = vSphereLogin(ctx, vs)
-	if err != nil {
-		t.Errorf("Failed to create vSpere client: %s", err)
-	}
-	defer vs.client.Logout(ctx)
-
-	// Get VM name
-	vmName, err := getVMName(vs.client, &cfg)
-	if err != nil {
-		t.Fatalf("Failed to get VM name: %s", err)
-	}
-	if vmName != "vmname" {
-		t.Errorf("Expect VM name 'vmname', got: %s", vmName)
-	}
-}

pkg/volume/vsphere_volume/BUILD

@@ -19,6 +19,7 @@ go_library(
     deps = [
         "//pkg/cloudprovider:go_default_library",
         "//pkg/cloudprovider/providers/vsphere:go_default_library",
+        "//pkg/cloudprovider/providers/vsphere/vclib:go_default_library",
         "//pkg/util/keymutex:go_default_library",
         "//pkg/util/mount:go_default_library",
         "//pkg/util/strings:go_default_library",
@@ -43,7 +44,7 @@ go_test(
     library = ":go_default_library",
     tags = ["automanaged"],
     deps = [
-        "//pkg/cloudprovider/providers/vsphere:go_default_library",
+        "//pkg/cloudprovider/providers/vsphere/vclib:go_default_library",
         "//pkg/util/mount:go_default_library",
         "//pkg/volume:go_default_library",
         "//pkg/volume/testing:go_default_library",

pkg/volume/vsphere_volume/attacher.go

@@ -75,7 +75,7 @@ func (attacher *vsphereVMDKAttacher) Attach(spec *volume.Spec, nodeName types.No
 
 	// vsphereCloud.AttachDisk checks if disk is already attached to host and
 	// succeeds in that case, so no need to do that separately.
-	_, diskUUID, err := attacher.vsphereVolumes.AttachDisk(volumeSource.VolumePath, volumeSource.StoragePolicyID, nodeName)
+	diskUUID, err := attacher.vsphereVolumes.AttachDisk(volumeSource.VolumePath, volumeSource.StoragePolicyID, nodeName)
 	if err != nil {
 		glog.Errorf("Error attaching volume %q to node %q: %+v", volumeSource.VolumePath, nodeName, err)
 		return "", err

pkg/volume/vsphere_volume/attacher_test.go

@@ -21,7 +21,7 @@ import (
 	"testing"
 
 	"k8s.io/api/core/v1"
-	"k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere"
+	"k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere/vclib"
 	"k8s.io/kubernetes/pkg/volume"
 	volumetest "k8s.io/kubernetes/pkg/volume/testing"
 
@@ -233,29 +233,29 @@ type diskIsAttachedCall struct {
 	ret        error
 }
 
-func (testcase *testcase) AttachDisk(diskName string, storagePolicyName string, nodeName types.NodeName) (string, string, error) {
+func (testcase *testcase) AttachDisk(diskName string, storagePolicyName string, nodeName types.NodeName) (string, error) {
 	expected := &testcase.attach
 
 	if expected.diskName == "" && expected.nodeName == "" {
 		// testcase.attach looks uninitialized, test did not expect to call
 		// AttachDisk
 		testcase.t.Errorf("Unexpected AttachDisk call!")
-		return "", "", errors.New("Unexpected AttachDisk call!")
+		return "", errors.New("Unexpected AttachDisk call!")
 	}
 
 	if expected.diskName != diskName {
 		testcase.t.Errorf("Unexpected AttachDisk call: expected diskName %s, got %s", expected.diskName, diskName)
-		return "", "", errors.New("Unexpected AttachDisk call: wrong diskName")
+		return "", errors.New("Unexpected AttachDisk call: wrong diskName")
 	}
 
 	if expected.nodeName != nodeName {
 		testcase.t.Errorf("Unexpected AttachDisk call: expected nodeName %s, got %s", expected.nodeName, nodeName)
-		return "", "", errors.New("Unexpected AttachDisk call: wrong nodeName")
+		return "", errors.New("Unexpected AttachDisk call: wrong nodeName")
 	}
 
 	glog.V(4).Infof("AttachDisk call: %s, %s, returning %q, %v", diskName, nodeName, expected.retDeviceUUID, expected.ret)
 
-	return "", expected.retDeviceUUID, expected.ret
+	return expected.retDeviceUUID, expected.ret
 }
 
 func (testcase *testcase) DetachDisk(diskName string, nodeName types.NodeName) error {
@@ -312,7 +312,7 @@ func (testcase *testcase) DisksAreAttached(diskNames []string, nodeName types.No
 	return nil, errors.New("Not implemented")
 }
 
-func (testcase *testcase) CreateVolume(volumeOptions *vsphere.VolumeOptions) (volumePath string, err error) {
+func (testcase *testcase) CreateVolume(volumeOptions *vclib.VolumeOptions) (volumePath string, err error) {
 	return "", errors.New("Not implemented")
 }
 

pkg/volume/vsphere_volume/vsphere_volume_util.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/api/core/v1"
 	"k8s.io/kubernetes/pkg/cloudprovider"
 	"k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere"
+	"k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere/vclib"
 	"k8s.io/kubernetes/pkg/volume"
 	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 )
@@ -94,7 +95,7 @@ func (util *VsphereDiskUtil) CreateVolume(v *vsphereVolumeProvisioner) (volSpec
 	// vSphere works with kilobytes, convert to KiB with rounding up
 	volSizeKB := int(volume.RoundUpSize(volSizeBytes, 1024))
 	name := volume.GenerateVolumeName(v.options.ClusterName, v.options.PVName, 255)
-	volumeOptions := &vsphere.VolumeOptions{
+	volumeOptions := &vclib.VolumeOptions{
 		CapacityKB: volSizeKB,
 		Tags:       *v.options.CloudTags,
 		Name:       name,
@@ -129,7 +130,7 @@ func (util *VsphereDiskUtil) CreateVolume(v *vsphereVolumeProvisioner) (volSpec
 
 	if volumeOptions.VSANStorageProfileData != "" {
 		if volumeOptions.StoragePolicyName != "" {
-			return nil, fmt.Errorf("Cannot specify storage policy capabilities along with storage policy name. Please specify only one.")
+			return nil, fmt.Errorf("Cannot specify storage policy capabilities along with storage policy name. Please specify only one")
 		}
 		volumeOptions.VSANStorageProfileData = "(" + volumeOptions.VSANStorageProfileData + ")"
 	}
@@ -141,7 +142,6 @@ func (util *VsphereDiskUtil) CreateVolume(v *vsphereVolumeProvisioner) (volSpec
 
 	vmDiskPath, err := cloud.CreateVolume(volumeOptions)
 	if err != nil {
-		glog.V(2).Infof("Error creating vsphere volume: %v", err)
 		return nil, err
 	}
 	volSpec = &VolumeSpec{