From 213cc99ead9a3fd03c3b382c6ac249fe1b9dbabd Mon Sep 17 00:00:00 2001 From: David Zhu Date: Thu, 25 Apr 2019 16:20:03 -0700 Subject: [PATCH 1/2] Operation generator migration metric fixes and test metrics retrieval code --- .../operationexecutor/operation_generator.go | 46 ++++- test/e2e/storage/drivers/in_tree.go | 14 ++ test/e2e/storage/testsuites/base.go | 169 ++++++++++++++++++ 3 files changed, 224 insertions(+), 5 deletions(-) diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 6fa0a684bbc..4599a487b4c 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -295,16 +295,18 @@ func (og *operationGenerator) GenerateBulkVolumeVerifyFunc( func (og *operationGenerator) GenerateAttachVolumeFunc( volumeToAttach VolumeToAttach, actualStateOfWorld ActualStateOfWorldAttacherUpdater) volumetypes.GeneratedOperations { + originalSpec := volumeToAttach.VolumeSpec attachVolumeFunc := func() (error, error) { var attachableVolumePlugin volume.AttachableVolumePlugin - originalSpec := volumeToAttach.VolumeSpec + nu, err := nodeUsingCSIPlugin(og, volumeToAttach.VolumeSpec, volumeToAttach.NodeName) if err != nil { return volumeToAttach.GenerateError("AttachVolume.NodeUsingCSIPlugin failed", err) } - // useCSIPlugin will check both CSIMigration and the plugin specific feature gate - if useCSIPlugin(og.volumePluginMgr, volumeToAttach.VolumeSpec) && nu { + // useCSIPlugin will check both CSIMigration and the plugin specific feature gates + ucp := useCSIPlugin(og.volumePluginMgr, volumeToAttach.VolumeSpec) + if ucp && nu { // The volume represented by this spec is CSI and thus should be migrated attachableVolumePlugin, err = og.volumePluginMgr.FindAttachablePluginByName(csi.CSIPluginName) if err != nil || attachableVolumePlugin == nil { @@ -382,8 +384,29 @@ func (og *operationGenerator) GenerateAttachVolumeFunc( } } - // Get attacher plugin attachableVolumePluginName := unknownAttachableVolumePlugin + // TODO(dyzz) Ignoring this error means that if the plugin is migrated and + // any transient error is encountered (API unavailable, driver not installed) + // the operation will have it's metric registered with the in-tree plugin instead + // of the CSI Driver we migrated to. Fixing this requires a larger refactor that + // involves determining the plugin_name for the metric generating "CompleteFunc" + // during the actual "OperationFunc" and not during this generation function + nu, _ := nodeUsingCSIPlugin(og, volumeToAttach.VolumeSpec, volumeToAttach.NodeName) + ucp := useCSIPlugin(og.volumePluginMgr, volumeToAttach.VolumeSpec) + + // Need to translate the spec here if the plugin is migrated so that the metrics + // emitted show the correct (migrated) plugin + if ucp && nu { + csiSpec, err := translateSpec(volumeToAttach.VolumeSpec) + if err == nil { + volumeToAttach.VolumeSpec = csiSpec + } + // If we have an error here we ignore it, the metric emitted will then be for the + // in-tree plugin. This error case(skipped one) will also trigger an error + // while the generated function is executed. And those errors will be handled during the execution of the generated + // function with a back off policy. + } + // Get attacher plugin attachableVolumePlugin, err := og.volumePluginMgr.FindAttachablePluginBySpec(volumeToAttach.VolumeSpec) // It's ok to ignore the error, returning error is not expected from this function. @@ -528,7 +551,20 @@ func (og *operationGenerator) GenerateMountVolumeFunc( actualStateOfWorld ActualStateOfWorldMounterUpdater, isRemount bool) volumetypes.GeneratedOperations { // Get mounter plugin + originalSpec := volumeToMount.VolumeSpec volumePluginName := unknownVolumePlugin + // Need to translate the spec here if the plugin is migrated so that the metrics + // emitted show the correct (migrated) plugin + if useCSIPlugin(og.volumePluginMgr, volumeToMount.VolumeSpec) { + csiSpec, err := translateSpec(volumeToMount.VolumeSpec) + if err == nil { + volumeToMount.VolumeSpec = csiSpec + } + // If we have an error here we ignore it, the metric emitted will then be for the + // in-tree plugin. This error case(skipped one) will also trigger an error + // while the generated function is executed. And those errors will be handled during the execution of the generated + // function with a back off policy. + } volumePlugin, err := og.volumePluginMgr.FindPluginBySpec(volumeToMount.VolumeSpec) if err == nil && volumePlugin != nil { @@ -536,7 +572,7 @@ func (og *operationGenerator) GenerateMountVolumeFunc( } mountVolumeFunc := func() (error, error) { - originalSpec := volumeToMount.VolumeSpec + // Get mounter plugin if useCSIPlugin(og.volumePluginMgr, volumeToMount.VolumeSpec) { csiSpec, err := translateSpec(volumeToMount.VolumeSpec) diff --git a/test/e2e/storage/drivers/in_tree.go b/test/e2e/storage/drivers/in_tree.go index e397921f0e4..68e813c83c3 100644 --- a/test/e2e/storage/drivers/in_tree.go +++ b/test/e2e/storage/drivers/in_tree.go @@ -88,6 +88,7 @@ func InitNFSDriver() testsuites.TestDriver { return &nfsDriver{ driverInfo: testsuites.DriverInfo{ Name: "nfs", + PluginName: "kubernetes.io/nfs", MaxFileSize: testpatterns.FileSizeLarge, SupportedFsType: sets.NewString( "", // Default fsType @@ -229,6 +230,7 @@ func InitGlusterFSDriver() testsuites.TestDriver { return &glusterFSDriver{ driverInfo: testsuites.DriverInfo{ Name: "gluster", + PluginName: "kubernetes.io/glusterfs", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType @@ -346,6 +348,7 @@ func InitISCSIDriver() testsuites.TestDriver { return &iSCSIDriver{ driverInfo: testsuites.DriverInfo{ Name: "iscsi", + PluginName: "kubernetes.io/iscsi", FeatureTag: "[Feature:Volumes]", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( @@ -458,6 +461,7 @@ func InitRbdDriver() testsuites.TestDriver { return &rbdDriver{ driverInfo: testsuites.DriverInfo{ Name: "rbd", + PluginName: "kubernetes.io/rbd", FeatureTag: "[Feature:Volumes]", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( @@ -585,6 +589,7 @@ func InitCephFSDriver() testsuites.TestDriver { return &cephFSDriver{ driverInfo: testsuites.DriverInfo{ Name: "ceph", + PluginName: "kubernetes.io/cephfs", FeatureTag: "[Feature:Volumes]", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( @@ -684,6 +689,7 @@ func InitHostPathDriver() testsuites.TestDriver { return &hostPathDriver{ driverInfo: testsuites.DriverInfo{ Name: "hostPath", + PluginName: "kubernetes.io/host-path", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType @@ -756,6 +762,7 @@ func InitHostPathSymlinkDriver() testsuites.TestDriver { return &hostPathSymlinkDriver{ driverInfo: testsuites.DriverInfo{ Name: "hostPathSymlink", + PluginName: "kubernetes.io/host-path", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType @@ -896,6 +903,7 @@ func InitEmptydirDriver() testsuites.TestDriver { return &emptydirDriver{ driverInfo: testsuites.DriverInfo{ Name: "emptydir", + PluginName: "kubernetes.io/empty-dir", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType @@ -961,6 +969,7 @@ func InitCinderDriver() testsuites.TestDriver { return &cinderDriver{ driverInfo: testsuites.DriverInfo{ Name: "cinder", + PluginName: "kubernetes.io/cinder", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType @@ -1129,6 +1138,7 @@ func InitGcePdDriver() testsuites.TestDriver { return &gcePdDriver{ driverInfo: testsuites.DriverInfo{ Name: "gcepd", + PluginName: "kubernetes.io/gce-pd", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: supportedTypes, SupportedMountOption: sets.NewString("debug", "nouid32"), @@ -1256,6 +1266,7 @@ func InitVSphereDriver() testsuites.TestDriver { return &vSphereDriver{ driverInfo: testsuites.DriverInfo{ Name: "vSphere", + PluginName: "kubernetes.io/vsphere-volume", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType @@ -1377,6 +1388,7 @@ func InitAzureDriver() testsuites.TestDriver { return &azureDriver{ driverInfo: testsuites.DriverInfo{ Name: "azure", + PluginName: "kubernetes.io/azure-file", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType @@ -1495,6 +1507,7 @@ func InitAwsDriver() testsuites.TestDriver { return &awsDriver{ driverInfo: testsuites.DriverInfo{ Name: "aws", + PluginName: "kubernetes.io/aws-ebs", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType @@ -1661,6 +1674,7 @@ func InitLocalDriverWithVolumeType(volumeType utils.LocalVolumeType) func() test return &localDriver{ driverInfo: testsuites.DriverInfo{ Name: "local", + PluginName: "kubernetes.io/local-volume", FeatureTag: featureTag, MaxFileSize: maxFileSize, SupportedFsType: supportedFsTypes, diff --git a/test/e2e/storage/testsuites/base.go b/test/e2e/storage/testsuites/base.go index 7cc57d28e6a..9be9a8944fb 100644 --- a/test/e2e/storage/testsuites/base.go +++ b/test/e2e/storage/testsuites/base.go @@ -18,8 +18,10 @@ package testsuites import ( "context" + "flag" "fmt" "regexp" + "strings" "time" . "github.com/onsi/ginkgo" @@ -31,13 +33,26 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" clientset "k8s.io/client-go/kubernetes" + csilib "k8s.io/csi-translation-lib" "k8s.io/kubernetes/test/e2e/framework" + "k8s.io/kubernetes/test/e2e/framework/metrics" "k8s.io/kubernetes/test/e2e/framework/podlogs" "k8s.io/kubernetes/test/e2e/framework/volume" "k8s.io/kubernetes/test/e2e/storage/testpatterns" ) +var ( + migratedPlugins *string +) + +func init() { + migratedPlugins = flag.String("storage.migratedPlugins", "", "comma seperated list of in-tree plugin names of form 'kubernetes.io/{pluginName}' migrated to CSI") +} + +type opCounts map[string]int64 + // TestSuite represents an interface for a set of tests which works with TestDriver type TestSuite interface { // getTestSuiteInfo returns the TestSuiteInfo for this TestSuite @@ -465,3 +480,157 @@ func StartPodLogs(f *framework.Framework) func() { return cancel } + +func getVolumeOpsFromMetricsForPlugin(ms metrics.ControllerManagerMetrics, pluginName string) opCounts { + totOps := opCounts{} + + for method, samples := range ms { + switch method { + case "storage_operation_status_count": + for _, sample := range samples { + plugin := string(sample.Metric["volume_plugin"]) + if pluginName != plugin { + continue + } + opName := string(sample.Metric["operation_name"]) + if opName == "verify_controller_attached_volume" { + // We ignore verify_controller_attached_volume because it does not call into + // the plugin. It only watches Node API and updates Actual State of World cache + continue + } + totOps[opName] = totOps[opName] + int64(sample.Value) + } + } + } + return totOps +} + +func getVolumeOpsFromKubeletMetricsForPlugin(ms metrics.KubeletMetrics, pluginName string) opCounts { + totOps := opCounts{} + + for method, samples := range ms { + switch method { + case "storage_operation_status_count": + for _, sample := range samples { + plugin := string(sample.Metric["volume_plugin"]) + if pluginName != plugin { + continue + } + opName := string(sample.Metric["operation_name"]) + if opName == "verify_controller_attached_volume" { + // We ignore verify_controller_attached_volume because it does not call into + // the plugin. It only watches Node API and updates Actual State of World cache + continue + } + totOps[opName] = totOps[opName] + int64(sample.Value) + } + } + } + return totOps +} + +func getVolumeOpCounts(c clientset.Interface, pluginName string) opCounts { + metricsGrabber, err := metrics.NewMetricsGrabber(c, nil, true, false, true, false, false) + + if err != nil { + framework.Failf("Error creating metrics grabber : %v", err) + } + + if !metricsGrabber.HasRegisteredMaster() { + framework.Skipf("Environment does not support getting controller-manager metrics - skipping") + } + + controllerMetrics, err := metricsGrabber.GrabFromControllerManager() + framework.ExpectNoError(err, "Error getting c-m metrics : %v", err) + totOps := getVolumeOpsFromMetricsForPlugin(controllerMetrics, pluginName) + + nodes, err := c.CoreV1().Nodes().List(metav1.ListOptions{}) + framework.ExpectNoError(err, "Error listing nodes: %v", err) + for _, node := range nodes.Items { + nodeMetrics, err := metricsGrabber.GrabFromKubelet(node.GetName()) + framework.ExpectNoError(err, "Error getting Kubelet %v metrics: %v", node.GetName(), err) + totOps = addOpCounts(totOps, getVolumeOpsFromKubeletMetricsForPlugin(nodeMetrics, pluginName)) + } + + return totOps +} + +func addOpCounts(o1 opCounts, o2 opCounts) opCounts { + totOps := opCounts{} + seen := sets.NewString() + for op, count := range o1 { + seen.Insert(op) + totOps[op] = totOps[op] + count + totOps[op] = totOps[op] + o2[op] + } + for op, count := range o2 { + if !seen.Has(op) { + totOps[op] = totOps[op] + count + } + } + return totOps +} + +func getMigrationVolumeOpCounts(cs clientset.Interface, pluginName string) (opCounts, opCounts) { + if len(pluginName) > 0 { + var migratedOps opCounts + csiName, err := csilib.GetCSINameFromInTreeName(pluginName) + if err != nil { + framework.Logf("Could not find CSI Name for in-tree plugin %v", pluginName) + migratedOps = opCounts{} + } else { + csiName = "kubernetes.io/csi:" + csiName + migratedOps = getVolumeOpCounts(cs, csiName) + } + return getVolumeOpCounts(cs, pluginName), migratedOps + } else { + // Not an in-tree driver + framework.Logf("Test running for native CSI Driver, not checking metrics") + return opCounts{}, opCounts{} + } +} + +func getTotOps(ops opCounts) int64 { + var tot int64 = 0 + for _, count := range ops { + tot += count + } + return tot +} + +func validateMigrationVolumeOpCounts(cs clientset.Interface, pluginName string, oldInTreeOps, oldMigratedOps opCounts) { + if len(pluginName) == 0 { + // This is a native CSI Driver and we don't check ops + return + } + + newInTreeOps, newMigratedOps := getMigrationVolumeOpCounts(cs, pluginName) + + if sets.NewString(strings.Split(*migratedPlugins, ",")...).Has(pluginName) { + // If this plugin is migrated based on the test flag storage.migratedPlugins + for op, count := range newInTreeOps { + if count != oldInTreeOps[op] { + framework.Failf("In-tree plugin %v migrated to CSI Driver, however found %v %v metrics for in-tree plugin", pluginName, count-oldInTreeOps[op], op) + } + } + totMigrated := getTotOps(newMigratedOps) + oldTotMigrated := getTotOps(oldMigratedOps) + if totMigrated-oldTotMigrated <= 0 { + framework.Failf("In-tree plugin %v migrated to CSI Driver, however found %v metrics for migrated plugin", pluginName, totMigrated-oldTotMigrated) + + } + } else { + // In-tree plugin is not migrated + totInTree := getTotOps(newInTreeOps) + oldTotInTree := getTotOps(oldInTreeOps) + if totInTree == oldTotInTree { + framework.Failf("In-tree plugin %v NOT migrated to CSI Driver, however found did not find any operation metrics for in-tree plugin", pluginName) + } + // We don't check counts for the Migrated version of the driver because if tests are running in parallel a test could be using + // the CSI Driver natively and increase the metrics count + + // TODO(dyzz): Add a dimension to OperationGenerator metrics for "migrated"->true/false so that we can disambiguate migrated metrics + // and native CSI Driver metrics. This way we can check the counts for migrated version of the driver for stronger negative test case + // guarantees (as well as more informative metrics). + } +} From 1271237d23bf828758a42eec531483d53cc006d5 Mon Sep 17 00:00:00 2001 From: David Zhu Date: Thu, 25 Apr 2019 16:20:19 -0700 Subject: [PATCH 2/2] Add migration metrics checking to all test suites --- .../operationexecutor/operation_generator.go | 12 ++- test/e2e/storage/drivers/in_tree.go | 92 +++++++++---------- test/e2e/storage/testsuites/BUILD | 2 + test/e2e/storage/testsuites/base.go | 85 +++++++---------- test/e2e/storage/testsuites/multivolume.go | 6 ++ test/e2e/storage/testsuites/provisioning.go | 6 ++ test/e2e/storage/testsuites/subpath.go | 6 ++ test/e2e/storage/testsuites/testdriver.go | 10 +- test/e2e/storage/testsuites/volume_io.go | 8 ++ test/e2e/storage/testsuites/volumemode.go | 6 ++ test/e2e/storage/testsuites/volumes.go | 7 ++ 11 files changed, 137 insertions(+), 103 deletions(-) diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 4599a487b4c..8c6340b4795 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -305,8 +305,7 @@ func (og *operationGenerator) GenerateAttachVolumeFunc( } // useCSIPlugin will check both CSIMigration and the plugin specific feature gates - ucp := useCSIPlugin(og.volumePluginMgr, volumeToAttach.VolumeSpec) - if ucp && nu { + if useCSIPlugin(og.volumePluginMgr, volumeToAttach.VolumeSpec) && nu { // The volume represented by this spec is CSI and thus should be migrated attachableVolumePlugin, err = og.volumePluginMgr.FindAttachablePluginByName(csi.CSIPluginName) if err != nil || attachableVolumePlugin == nil { @@ -391,12 +390,15 @@ func (og *operationGenerator) GenerateAttachVolumeFunc( // of the CSI Driver we migrated to. Fixing this requires a larger refactor that // involves determining the plugin_name for the metric generating "CompleteFunc" // during the actual "OperationFunc" and not during this generation function - nu, _ := nodeUsingCSIPlugin(og, volumeToAttach.VolumeSpec, volumeToAttach.NodeName) - ucp := useCSIPlugin(og.volumePluginMgr, volumeToAttach.VolumeSpec) + + nu, err := nodeUsingCSIPlugin(og, volumeToAttach.VolumeSpec, volumeToAttach.NodeName) + if err != nil { + klog.Errorf("GenerateAttachVolumeFunc failed to check if node is using CSI Plugin, metric for this operation may be inaccurate: %v", err) + } // Need to translate the spec here if the plugin is migrated so that the metrics // emitted show the correct (migrated) plugin - if ucp && nu { + if useCSIPlugin(og.volumePluginMgr, volumeToAttach.VolumeSpec) && nu { csiSpec, err := translateSpec(volumeToAttach.VolumeSpec) if err == nil { volumeToAttach.VolumeSpec = csiSpec diff --git a/test/e2e/storage/drivers/in_tree.go b/test/e2e/storage/drivers/in_tree.go index 68e813c83c3..3cc6ff707f3 100644 --- a/test/e2e/storage/drivers/in_tree.go +++ b/test/e2e/storage/drivers/in_tree.go @@ -87,9 +87,9 @@ var _ testsuites.DynamicPVTestDriver = &nfsDriver{} func InitNFSDriver() testsuites.TestDriver { return &nfsDriver{ driverInfo: testsuites.DriverInfo{ - Name: "nfs", - PluginName: "kubernetes.io/nfs", - MaxFileSize: testpatterns.FileSizeLarge, + Name: "nfs", + InTreePluginName: "kubernetes.io/nfs", + MaxFileSize: testpatterns.FileSizeLarge, SupportedFsType: sets.NewString( "", // Default fsType ), @@ -229,9 +229,9 @@ var _ testsuites.PreprovisionedPVTestDriver = &glusterFSDriver{} func InitGlusterFSDriver() testsuites.TestDriver { return &glusterFSDriver{ driverInfo: testsuites.DriverInfo{ - Name: "gluster", - PluginName: "kubernetes.io/glusterfs", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "gluster", + InTreePluginName: "kubernetes.io/glusterfs", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType ), @@ -347,10 +347,10 @@ var _ testsuites.PreprovisionedPVTestDriver = &iSCSIDriver{} func InitISCSIDriver() testsuites.TestDriver { return &iSCSIDriver{ driverInfo: testsuites.DriverInfo{ - Name: "iscsi", - PluginName: "kubernetes.io/iscsi", - FeatureTag: "[Feature:Volumes]", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "iscsi", + InTreePluginName: "kubernetes.io/iscsi", + FeatureTag: "[Feature:Volumes]", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType "ext2", @@ -460,10 +460,10 @@ var _ testsuites.PreprovisionedPVTestDriver = &rbdDriver{} func InitRbdDriver() testsuites.TestDriver { return &rbdDriver{ driverInfo: testsuites.DriverInfo{ - Name: "rbd", - PluginName: "kubernetes.io/rbd", - FeatureTag: "[Feature:Volumes]", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "rbd", + InTreePluginName: "kubernetes.io/rbd", + FeatureTag: "[Feature:Volumes]", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType "ext2", @@ -588,10 +588,10 @@ var _ testsuites.PreprovisionedPVTestDriver = &cephFSDriver{} func InitCephFSDriver() testsuites.TestDriver { return &cephFSDriver{ driverInfo: testsuites.DriverInfo{ - Name: "ceph", - PluginName: "kubernetes.io/cephfs", - FeatureTag: "[Feature:Volumes]", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "ceph", + InTreePluginName: "kubernetes.io/cephfs", + FeatureTag: "[Feature:Volumes]", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType ), @@ -688,9 +688,9 @@ var _ testsuites.InlineVolumeTestDriver = &hostPathDriver{} func InitHostPathDriver() testsuites.TestDriver { return &hostPathDriver{ driverInfo: testsuites.DriverInfo{ - Name: "hostPath", - PluginName: "kubernetes.io/host-path", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "hostPath", + InTreePluginName: "kubernetes.io/host-path", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType ), @@ -761,9 +761,9 @@ var _ testsuites.InlineVolumeTestDriver = &hostPathSymlinkDriver{} func InitHostPathSymlinkDriver() testsuites.TestDriver { return &hostPathSymlinkDriver{ driverInfo: testsuites.DriverInfo{ - Name: "hostPathSymlink", - PluginName: "kubernetes.io/host-path", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "hostPathSymlink", + InTreePluginName: "kubernetes.io/host-path", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType ), @@ -902,9 +902,9 @@ var _ testsuites.InlineVolumeTestDriver = &emptydirDriver{} func InitEmptydirDriver() testsuites.TestDriver { return &emptydirDriver{ driverInfo: testsuites.DriverInfo{ - Name: "emptydir", - PluginName: "kubernetes.io/empty-dir", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "emptydir", + InTreePluginName: "kubernetes.io/empty-dir", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType ), @@ -968,9 +968,9 @@ var _ testsuites.DynamicPVTestDriver = &cinderDriver{} func InitCinderDriver() testsuites.TestDriver { return &cinderDriver{ driverInfo: testsuites.DriverInfo{ - Name: "cinder", - PluginName: "kubernetes.io/cinder", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "cinder", + InTreePluginName: "kubernetes.io/cinder", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType "ext3", @@ -1138,7 +1138,7 @@ func InitGcePdDriver() testsuites.TestDriver { return &gcePdDriver{ driverInfo: testsuites.DriverInfo{ Name: "gcepd", - PluginName: "kubernetes.io/gce-pd", + InTreePluginName: "kubernetes.io/gce-pd", MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: supportedTypes, SupportedMountOption: sets.NewString("debug", "nouid32"), @@ -1265,9 +1265,9 @@ var _ testsuites.DynamicPVTestDriver = &vSphereDriver{} func InitVSphereDriver() testsuites.TestDriver { return &vSphereDriver{ driverInfo: testsuites.DriverInfo{ - Name: "vSphere", - PluginName: "kubernetes.io/vsphere-volume", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "vSphere", + InTreePluginName: "kubernetes.io/vsphere-volume", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType "ext4", @@ -1387,9 +1387,9 @@ var _ testsuites.DynamicPVTestDriver = &azureDriver{} func InitAzureDriver() testsuites.TestDriver { return &azureDriver{ driverInfo: testsuites.DriverInfo{ - Name: "azure", - PluginName: "kubernetes.io/azure-file", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "azure", + InTreePluginName: "kubernetes.io/azure-file", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType "ext4", @@ -1506,9 +1506,9 @@ var _ testsuites.DynamicPVTestDriver = &awsDriver{} func InitAwsDriver() testsuites.TestDriver { return &awsDriver{ driverInfo: testsuites.DriverInfo{ - Name: "aws", - PluginName: "kubernetes.io/aws-ebs", - MaxFileSize: testpatterns.FileSizeMedium, + Name: "aws", + InTreePluginName: "kubernetes.io/aws-ebs", + MaxFileSize: testpatterns.FileSizeMedium, SupportedFsType: sets.NewString( "", // Default fsType "ext3", @@ -1673,12 +1673,12 @@ func InitLocalDriverWithVolumeType(volumeType utils.LocalVolumeType) func() test } return &localDriver{ driverInfo: testsuites.DriverInfo{ - Name: "local", - PluginName: "kubernetes.io/local-volume", - FeatureTag: featureTag, - MaxFileSize: maxFileSize, - SupportedFsType: supportedFsTypes, - Capabilities: capabilities, + Name: "local", + InTreePluginName: "kubernetes.io/local-volume", + FeatureTag: featureTag, + MaxFileSize: maxFileSize, + SupportedFsType: supportedFsTypes, + Capabilities: capabilities, }, volumeType: volumeType, } diff --git a/test/e2e/storage/testsuites/BUILD b/test/e2e/storage/testsuites/BUILD index a04e92d9b12..005c744b142 100644 --- a/test/e2e/storage/testsuites/BUILD +++ b/test/e2e/storage/testsuites/BUILD @@ -32,7 +32,9 @@ go_library( "//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library", "//staging/src/k8s.io/client-go/dynamic:go_default_library", "//staging/src/k8s.io/client-go/kubernetes:go_default_library", + "//staging/src/k8s.io/csi-translation-lib:go_default_library", "//test/e2e/framework:go_default_library", + "//test/e2e/framework/metrics:go_default_library", "//test/e2e/framework/podlogs:go_default_library", "//test/e2e/framework/volume:go_default_library", "//test/e2e/storage/testpatterns:go_default_library", diff --git a/test/e2e/storage/testsuites/base.go b/test/e2e/storage/testsuites/base.go index 9be9a8944fb..e169a41879e 100644 --- a/test/e2e/storage/testsuites/base.go +++ b/test/e2e/storage/testsuites/base.go @@ -48,7 +48,7 @@ var ( ) func init() { - migratedPlugins = flag.String("storage.migratedPlugins", "", "comma seperated list of in-tree plugin names of form 'kubernetes.io/{pluginName}' migrated to CSI") + migratedPlugins = flag.String("storage.migratedPlugins", "", "comma separated list of in-tree plugin names of form 'kubernetes.io/{pluginName}' migrated to CSI") } type opCounts map[string]int64 @@ -481,31 +481,7 @@ func StartPodLogs(f *framework.Framework) func() { return cancel } -func getVolumeOpsFromMetricsForPlugin(ms metrics.ControllerManagerMetrics, pluginName string) opCounts { - totOps := opCounts{} - - for method, samples := range ms { - switch method { - case "storage_operation_status_count": - for _, sample := range samples { - plugin := string(sample.Metric["volume_plugin"]) - if pluginName != plugin { - continue - } - opName := string(sample.Metric["operation_name"]) - if opName == "verify_controller_attached_volume" { - // We ignore verify_controller_attached_volume because it does not call into - // the plugin. It only watches Node API and updates Actual State of World cache - continue - } - totOps[opName] = totOps[opName] + int64(sample.Value) - } - } - } - return totOps -} - -func getVolumeOpsFromKubeletMetricsForPlugin(ms metrics.KubeletMetrics, pluginName string) opCounts { +func getVolumeOpsFromMetricsForPlugin(ms metrics.Metrics, pluginName string) opCounts { totOps := opCounts{} for method, samples := range ms { @@ -530,6 +506,8 @@ func getVolumeOpsFromKubeletMetricsForPlugin(ms metrics.KubeletMetrics, pluginNa } func getVolumeOpCounts(c clientset.Interface, pluginName string) opCounts { + nodeLimit := 25 + metricsGrabber, err := metrics.NewMetricsGrabber(c, nil, true, false, true, false, false) if err != nil { @@ -542,14 +520,22 @@ func getVolumeOpCounts(c clientset.Interface, pluginName string) opCounts { controllerMetrics, err := metricsGrabber.GrabFromControllerManager() framework.ExpectNoError(err, "Error getting c-m metrics : %v", err) - totOps := getVolumeOpsFromMetricsForPlugin(controllerMetrics, pluginName) + totOps := getVolumeOpsFromMetricsForPlugin(metrics.Metrics(controllerMetrics), pluginName) + framework.Logf("Node name not specified for getVolumeOpCounts, falling back to listing nodes from API Server") nodes, err := c.CoreV1().Nodes().List(metav1.ListOptions{}) framework.ExpectNoError(err, "Error listing nodes: %v", err) - for _, node := range nodes.Items { - nodeMetrics, err := metricsGrabber.GrabFromKubelet(node.GetName()) - framework.ExpectNoError(err, "Error getting Kubelet %v metrics: %v", node.GetName(), err) - totOps = addOpCounts(totOps, getVolumeOpsFromKubeletMetricsForPlugin(nodeMetrics, pluginName)) + if len(nodes.Items) <= nodeLimit { + // For large clusters with > nodeLimit nodes it is too time consuming to + // gather metrics from all nodes. We just ignore the node metrics + // for those clusters + for _, node := range nodes.Items { + nodeMetrics, err := metricsGrabber.GrabFromKubelet(node.GetName()) + framework.ExpectNoError(err, "Error getting Kubelet %v metrics: %v", node.GetName(), err) + totOps = addOpCounts(totOps, getVolumeOpsFromMetricsForPlugin(metrics.Metrics(nodeMetrics), pluginName)) + } + } else { + framework.Logf("Skipping operation metrics gathering from nodes in getVolumeOpCounts, greater than %v nodes", nodeLimit) } return totOps @@ -560,8 +546,7 @@ func addOpCounts(o1 opCounts, o2 opCounts) opCounts { seen := sets.NewString() for op, count := range o1 { seen.Insert(op) - totOps[op] = totOps[op] + count - totOps[op] = totOps[op] + o2[op] + totOps[op] = totOps[op] + count + o2[op] } for op, count := range o2 { if !seen.Has(op) { @@ -604,33 +589,33 @@ func validateMigrationVolumeOpCounts(cs clientset.Interface, pluginName string, return } - newInTreeOps, newMigratedOps := getMigrationVolumeOpCounts(cs, pluginName) - if sets.NewString(strings.Split(*migratedPlugins, ",")...).Has(pluginName) { // If this plugin is migrated based on the test flag storage.migratedPlugins + newInTreeOps, _ := getMigrationVolumeOpCounts(cs, pluginName) + for op, count := range newInTreeOps { if count != oldInTreeOps[op] { framework.Failf("In-tree plugin %v migrated to CSI Driver, however found %v %v metrics for in-tree plugin", pluginName, count-oldInTreeOps[op], op) } } - totMigrated := getTotOps(newMigratedOps) - oldTotMigrated := getTotOps(oldMigratedOps) - if totMigrated-oldTotMigrated <= 0 { - framework.Failf("In-tree plugin %v migrated to CSI Driver, however found %v metrics for migrated plugin", pluginName, totMigrated-oldTotMigrated) - - } + // We don't check for migrated metrics because some negative test cases + // may not do any volume operations and therefore not emit any metrics } else { // In-tree plugin is not migrated - totInTree := getTotOps(newInTreeOps) - oldTotInTree := getTotOps(oldInTreeOps) - if totInTree == oldTotInTree { - framework.Failf("In-tree plugin %v NOT migrated to CSI Driver, however found did not find any operation metrics for in-tree plugin", pluginName) - } - // We don't check counts for the Migrated version of the driver because if tests are running in parallel a test could be using - // the CSI Driver natively and increase the metrics count + framework.Logf("In-tree plugin %v is not migrated, not validating any metrics", pluginName) - // TODO(dyzz): Add a dimension to OperationGenerator metrics for "migrated"->true/false so that we can disambiguate migrated metrics - // and native CSI Driver metrics. This way we can check the counts for migrated version of the driver for stronger negative test case + // We don't check in-tree plugin metrics because some negative test + // cases may not do any volume operations and therefore not emit any + // metrics + + // We don't check counts for the Migrated version of the driver because + // if tests are running in parallel a test could be using the CSI Driver + // natively and increase the metrics count + + // TODO(dyzz): Add a dimension to OperationGenerator metrics for + // "migrated"->true/false so that we can disambiguate migrated metrics + // and native CSI Driver metrics. This way we can check the counts for + // migrated version of the driver for stronger negative test case // guarantees (as well as more informative metrics). } } diff --git a/test/e2e/storage/testsuites/multivolume.go b/test/e2e/storage/testsuites/multivolume.go index d7c74dc2f06..a4446417dff 100644 --- a/test/e2e/storage/testsuites/multivolume.go +++ b/test/e2e/storage/testsuites/multivolume.go @@ -64,6 +64,9 @@ func (t *multiVolumeTestSuite) defineTests(driver TestDriver, pattern testpatter ns *v1.Namespace driver TestDriver resources []*genericVolumeTestResource + + intreeOps opCounts + migratedOps opCounts } var ( dInfo = driver.GetDriverInfo() @@ -91,6 +94,7 @@ func (t *multiVolumeTestSuite) defineTests(driver TestDriver, pattern testpatter // Now do the more expensive test initialization. l.config, l.testCleanup = driver.PrepareTest(f) + l.intreeOps, l.migratedOps = getMigrationVolumeOpCounts(f.ClientSet, dInfo.InTreePluginName) } cleanup := func() { @@ -102,6 +106,8 @@ func (t *multiVolumeTestSuite) defineTests(driver TestDriver, pattern testpatter l.testCleanup() l.testCleanup = nil } + + validateMigrationVolumeOpCounts(f.ClientSet, dInfo.InTreePluginName, l.intreeOps, l.migratedOps) } // This tests below configuration: diff --git a/test/e2e/storage/testsuites/provisioning.go b/test/e2e/storage/testsuites/provisioning.go index f6267283997..e96a1ddf568 100644 --- a/test/e2e/storage/testsuites/provisioning.go +++ b/test/e2e/storage/testsuites/provisioning.go @@ -89,6 +89,9 @@ func (p *provisioningTestSuite) defineTests(driver TestDriver, pattern testpatte cs clientset.Interface pvc *v1.PersistentVolumeClaim sc *storage.StorageClass + + intreeOps opCounts + migratedOps opCounts } var ( dInfo = driver.GetDriverInfo() @@ -119,6 +122,7 @@ func (p *provisioningTestSuite) defineTests(driver TestDriver, pattern testpatte // Now do the more expensive test initialization. l.config, l.testCleanup = driver.PrepareTest(f) + l.intreeOps, l.migratedOps = getMigrationVolumeOpCounts(f.ClientSet, dInfo.InTreePluginName) l.cs = l.config.Framework.ClientSet claimSize := dDriver.GetClaimSize() l.sc = dDriver.GetDynamicProvisionStorageClass(l.config, pattern.FsType) @@ -142,6 +146,8 @@ func (p *provisioningTestSuite) defineTests(driver TestDriver, pattern testpatte l.testCleanup() l.testCleanup = nil } + + validateMigrationVolumeOpCounts(f.ClientSet, dInfo.InTreePluginName, l.intreeOps, l.migratedOps) } It("should provision storage with defaults", func() { diff --git a/test/e2e/storage/testsuites/subpath.go b/test/e2e/storage/testsuites/subpath.go index 121822dde05..3072e423c30 100644 --- a/test/e2e/storage/testsuites/subpath.go +++ b/test/e2e/storage/testsuites/subpath.go @@ -83,6 +83,9 @@ func (s *subPathTestSuite) defineTests(driver TestDriver, pattern testpatterns.T subPathDir string filePathInSubpath string filePathInVolume string + + intreeOps opCounts + migratedOps opCounts } var l local @@ -99,6 +102,7 @@ func (s *subPathTestSuite) defineTests(driver TestDriver, pattern testpatterns.T // Now do the more expensive test initialization. l.config, l.testCleanup = driver.PrepareTest(f) + l.intreeOps, l.migratedOps = getMigrationVolumeOpCounts(f.ClientSet, driver.GetDriverInfo().InTreePluginName) l.resource = createGenericVolumeTestResource(driver, l.config, pattern) // Setup subPath test dependent resource @@ -157,6 +161,8 @@ func (s *subPathTestSuite) defineTests(driver TestDriver, pattern testpatterns.T l.testCleanup() l.testCleanup = nil } + + validateMigrationVolumeOpCounts(f.ClientSet, driver.GetDriverInfo().InTreePluginName, l.intreeOps, l.migratedOps) } It("should support non-existent path", func() { diff --git a/test/e2e/storage/testsuites/testdriver.go b/test/e2e/storage/testsuites/testdriver.go index 5923db5dbc8..3736390b747 100644 --- a/test/e2e/storage/testsuites/testdriver.go +++ b/test/e2e/storage/testsuites/testdriver.go @@ -128,8 +128,14 @@ const ( // DriverInfo represents static information about a TestDriver. type DriverInfo struct { - Name string // Name of the driver, aka the provisioner name. - FeatureTag string // FeatureTag for the driver + // Internal name of the driver, this is used as a display name in the test + // case and test objects + Name string + // Fully qualified plugin name as registered in Kubernetes of the in-tree + // plugin if it exists and is empty if this DriverInfo represents a CSI + // Driver + InTreePluginName string + FeatureTag string // FeatureTag for the driver MaxFileSize int64 // Max file size to be tested for this driver SupportedFsType sets.String // Map of string for supported fs type diff --git a/test/e2e/storage/testsuites/volume_io.go b/test/e2e/storage/testsuites/volume_io.go index aef892450ee..e13232bc34e 100644 --- a/test/e2e/storage/testsuites/volume_io.go +++ b/test/e2e/storage/testsuites/volume_io.go @@ -80,6 +80,9 @@ func (t *volumeIOTestSuite) defineTests(driver TestDriver, pattern testpatterns. testCleanup func() resource *genericVolumeTestResource + + intreeOps opCounts + migratedOps opCounts } var ( dInfo = driver.GetDriverInfo() @@ -99,10 +102,13 @@ func (t *volumeIOTestSuite) defineTests(driver TestDriver, pattern testpatterns. // Now do the more expensive test initialization. l.config, l.testCleanup = driver.PrepareTest(f) + l.intreeOps, l.migratedOps = getMigrationVolumeOpCounts(f.ClientSet, dInfo.InTreePluginName) + l.resource = createGenericVolumeTestResource(driver, l.config, pattern) if l.resource.volSource == nil { framework.Skipf("Driver %q does not define volumeSource - skipping", dInfo.Name) } + } cleanup := func() { @@ -115,6 +121,8 @@ func (t *volumeIOTestSuite) defineTests(driver TestDriver, pattern testpatterns. l.testCleanup() l.testCleanup = nil } + + validateMigrationVolumeOpCounts(f.ClientSet, dInfo.InTreePluginName, l.intreeOps, l.migratedOps) } It("should write files of various sizes, verify size, validate content [Slow]", func() { diff --git a/test/e2e/storage/testsuites/volumemode.go b/test/e2e/storage/testsuites/volumemode.go index 0e54fb57d35..4d0b89af299 100644 --- a/test/e2e/storage/testsuites/volumemode.go +++ b/test/e2e/storage/testsuites/volumemode.go @@ -70,6 +70,9 @@ func (t *volumeModeTestSuite) defineTests(driver TestDriver, pattern testpattern ns *v1.Namespace // genericVolumeTestResource contains pv, pvc, sc, etc., owns cleaning that up genericVolumeTestResource + + intreeOps opCounts + migratedOps opCounts } var ( dInfo = driver.GetDriverInfo() @@ -91,6 +94,7 @@ func (t *volumeModeTestSuite) defineTests(driver TestDriver, pattern testpattern // Now do the more expensive test initialization. l.config, l.testCleanup = driver.PrepareTest(f) + l.intreeOps, l.migratedOps = getMigrationVolumeOpCounts(f.ClientSet, dInfo.InTreePluginName) fsType := pattern.FsType volBindMode := storagev1.VolumeBindingImmediate @@ -153,6 +157,8 @@ func (t *volumeModeTestSuite) defineTests(driver TestDriver, pattern testpattern l.testCleanup() l.testCleanup = nil } + + validateMigrationVolumeOpCounts(f.ClientSet, dInfo.InTreePluginName, l.intreeOps, l.migratedOps) } // We register different tests depending on the drive diff --git a/test/e2e/storage/testsuites/volumes.go b/test/e2e/storage/testsuites/volumes.go index 7b42f9f3ce0..53870860a22 100644 --- a/test/e2e/storage/testsuites/volumes.go +++ b/test/e2e/storage/testsuites/volumes.go @@ -98,6 +98,9 @@ func (t *volumesTestSuite) defineTests(driver TestDriver, pattern testpatterns.T testCleanup func() resource *genericVolumeTestResource + + intreeOps opCounts + migratedOps opCounts } var dInfo = driver.GetDriverInfo() var l local @@ -119,6 +122,8 @@ func (t *volumesTestSuite) defineTests(driver TestDriver, pattern testpatterns.T if l.resource.volSource == nil { framework.Skipf("Driver %q does not define volumeSource - skipping", dInfo.Name) } + + l.intreeOps, l.migratedOps = getMigrationVolumeOpCounts(f.ClientSet, dInfo.InTreePluginName) } cleanup := func() { @@ -131,6 +136,8 @@ func (t *volumesTestSuite) defineTests(driver TestDriver, pattern testpatterns.T l.testCleanup() l.testCleanup = nil } + + validateMigrationVolumeOpCounts(f.ClientSet, dInfo.InTreePluginName, l.intreeOps, l.migratedOps) } It("should be mountable", func() {