From 561685f2e088684597b13441b219df030ce158b7 Mon Sep 17 00:00:00 2001 From: Anshuman Date: Fri, 10 Jan 2025 11:09:51 +0530 Subject: [PATCH 1/2] Improved linting and fixed linting issues in code --- .golangci.yml | 202 ++++++++++++++++++ .../controllers/nodeupdate_controller.go | 4 +- adhoc-controllers/main.go | 11 +- cmd/main.go | 6 +- cmd/options.go | 16 +- cmd/options/controller_options.go | 38 ++-- cmd/options/node_options.go | 2 +- cmd/options/server_options.go | 2 +- cmd/options_test.go | 9 +- pkg/cloud/cloud.go | 12 +- pkg/cloud/metadata.go | 14 +- pkg/cloud/metadata_k8s.go | 4 +- pkg/cloud/powervs.go | 4 +- pkg/cloud/powervs_node.go | 2 +- pkg/device/device.go | 13 +- pkg/device/multipath.go | 16 +- pkg/device/utils.go | 8 +- pkg/driver/constants.go | 10 +- pkg/driver/controller.go | 33 +-- pkg/driver/controller_test.go | 7 - pkg/driver/driver.go | 4 +- pkg/driver/identity.go | 1 + pkg/driver/mount.go | 2 +- pkg/driver/mount_test.go | 3 - pkg/driver/node.go | 54 ++--- pkg/driver/node_test.go | 29 ++- pkg/driver/sanity_test.go | 17 +- pkg/driver/stats.go | 13 +- pkg/driver/version.go | 2 +- pkg/util/util.go | 14 +- pkg/util/util_test.go | 2 - tests/e2e/driver/driver.go | 4 +- tests/e2e/driver/powervs_csi_driver.go | 9 +- tests/e2e/dynamic_provisioning.go | 1 + tests/e2e/pre_provisioning.go | 6 +- ...namically_provisioned_cmd_volume_tester.go | 8 +- ...cally_provisioned_collocated_pod_tester.go | 9 +- ...namically_provisioned_delete_pod_tester.go | 8 +- ...lly_provisioned_read_only_volume_tester.go | 8 +- ...cally_provisioned_reclaim_policy_tester.go | 8 +- ...ically_provisioned_resize_volume_tester.go | 10 +- ...rovisioned_topology_aware_volume_tester.go | 9 +- ...pre_provisioned_read_only_volume_tester.go | 3 +- .../pre_provisioned_reclaim_policy_tester.go | 6 +- .../pre_provisioned_volume_tester.go | 7 +- tests/e2e/testsuites/specs.go | 1 + tests/e2e/testsuites/testsuites.go | 28 +-- tests/it/utils.go | 19 +- tests/remote/pi_resources.go | 19 +- tests/remote/setup.go | 11 +- tests/remote/util.go | 1 + 51 files changed, 464 insertions(+), 265 deletions(-) create mode 100644 .golangci.yml diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 000000000..834baad20 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,202 @@ +linters: + disable-all: true + enable: + - asasalint + - asciicheck + - bodyclose + - containedctx + - copyloopvar + - decorder + - dogsled + - errcheck + - errchkjson + - execinquery + # - gci + - goconst + # - gocritic + - gocyclo + - godot + - gofmt + - goimports + - goprintffuncname + # - gosec + - gosimple + - govet + # - importas + - ineffassign + - misspell + - nakedret + - nilerr + - noctx + - nolintlint + - nosprintfhostport + - prealloc + - predeclared + - reassign + # - revive + - rowserrcheck + - staticcheck + # - stylecheck + # - thelper + - typecheck + - unconvert + - unparam + - unused + - usestdlibvars + - whitespace + +linters-settings: + gocyclo: + min-complexity: 20 + godot: + # declarations - for top level declaration comments (default); + # toplevel - for top level comments; + # all - for all comments. + scope: toplevel + exclude: + - '^ \+.*' + - '^ ANCHOR.*' + gci: + sections: + - standard + - default + - prefix(github.com/IBM) + - prefix(k8s.io) + - prefix(sigs.k8s.io) + - blank + - dot + importas: + no-unaliased: true + alias: + # Kubernetes + - pkg: k8s.io/api/core/v1 + alias: corev1 + - pkg: k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1 + alias: apiextensionsv1 + - pkg: k8s.io/apimachinery/pkg/apis/meta/v1 + alias: metav1 + - pkg: k8s.io/apimachinery/pkg/api/errors + alias: apierrors + - pkg: k8s.io/apimachinery/pkg/util/errors + alias: kerrors + # Controller Runtime + - pkg: sigs.k8s.io/controller-runtime + alias: ctrl + nolintlint: + allow-unused: false + # allow-leading-space: false + require-specific: true + gosec: + excludes: + - G307 # Deferring unsafe method "Close" on type "\*os.File" + - G108 # Profiling endpoint is automatically exposed on /debug/pprof + gocritic: + enabled-tags: + - experimental + disabled-checks: + - appendAssign + - dupImport # https://github.com/go-critic/go-critic/issues/845 + - evalOrder + - ifElseChain + - octalLiteral + - regexpSimplify + - sloppyReassign + - truncateCmp + - typeDefFirst + - unnamedResult + - unnecessaryDefer + - whyNoLint + - wrapperFunc + unused: + go: "1.22" +issues: + exclude-files: + - "zz_generated.*\\.go$" + max-same-issues: 0 + max-issues-per-linter: 0 + # We are disabling default golangci exclusions because we want to help reviewers to focus on reviewing the most relevant + # changes in PRs and avoid nitpicking. + exclude-use-default: false + exclude-rules: + - linters: + - gci + path: _test\.go + - linters: + - revive + text: "exported: exported method .*\\.(Reconcile|SetupWithManager|SetupWebhookWithManager) should have comment or be unexported" + - linters: + - errcheck + text: Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked + # Exclude some packages or code to require comments, for example test code, or fake clients. + - linters: + - revive + text: exported (method|function|type|const) (.+) should have comment or be unexported + source: (func|type).*Fake.* + - linters: + - revive + text: exported (method|function|type|const) (.+) should have comment or be unexported + path: fake_\.go + - linters: + - revive + text: exported (method|function|type|const) (.+) should have comment or be unexported + path: "(framework|e2e)/.*.go" + # Disable unparam "always receives" which might not be really + # useful when building libraries. + - linters: + - unparam + text: always receives + # Dot imports for gomega or ginkgo are allowed + # within test files. + - path: _test\.go + text: should not use dot imports + - path: (framework|e2e)/.*.go + text: should not use dot imports + - path: _test\.go + text: cyclomatic complexity + # Append should be able to assign to a different var/slice. + - linters: + - gocritic + text: "appendAssign: append result not assigned to the same slice" + # Disable linters for conversion + - linters: + - staticcheck + text: "SA1019: in.(.+) is deprecated" + path: .*(api|types)\/.*\/.*conversion.*\.go$ + - linters: + - revive + text: exported (method|function|type|const) (.+) should have comment or be unexported + path: .*(api|types)\/.*\/.*conversion.*\.go$ + - linters: + - revive + text: "var-naming: don't use underscores in Go names;" + path: .*(api|types)\/.*\/.*conversion.*\.go$ + - linters: + - revive + text: "receiver-naming: receiver name" + path: .*(api|types)\/.*\/.*conversion.*\.go$ + - linters: + - stylecheck + text: "ST1003: should not use underscores in Go names;" + path: .*(api|types)\/.*\/.*conversion.*\.go$ + - linters: + - stylecheck + text: "ST1016: methods on the same type should have the same receiver name" + path: .*(api|types)\/.*\/.*conversion.*\.go$ + # hack/tools + - linters: + - typecheck + text: import (".+") is a program, not an importable package + path: ^tools\.go$ + # We don't care about defer in for loops in test files. + - linters: + - gocritic + text: "deferInLoop: Possible resource leak, 'defer' is called in the 'for' loop" + path: _test\.go + +run: + timeout: 10m + build-tags: + - tools + - e2e + allow-parallel-runners: true + go: "1.22" \ No newline at end of file diff --git a/adhoc-controllers/controllers/nodeupdate_controller.go b/adhoc-controllers/controllers/nodeupdate_controller.go index e3bf54969..d7d6adb54 100644 --- a/adhoc-controllers/controllers/nodeupdate_controller.go +++ b/adhoc-controllers/controllers/nodeupdate_controller.go @@ -24,12 +24,13 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog/v2" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/cloud" ) -// NodeUpdateReconciler reconciles a NodeUpdate object +// NodeUpdateReconciler reconciles a NodeUpdate object. type NodeUpdateReconciler struct { Client client.Client Scheme *runtime.Scheme @@ -41,7 +42,6 @@ type NodeUpdateReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.10.0/pkg/reconcile func (r *NodeUpdateReconciler) Reconcile(_ context.Context, req ctrl.Request) (ctrl.Result, error) { - // Fetch the Node instance node := corev1.Node{} err := r.Client.Get(context.Background(), req.NamespacedName, &node) diff --git a/adhoc-controllers/main.go b/adhoc-controllers/main.go index eb83cc734..1174bab53 100644 --- a/adhoc-controllers/main.go +++ b/adhoc-controllers/main.go @@ -20,21 +20,22 @@ import ( "flag" "os" - // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) - // to ensure that exec-entrypoint and run can make use of them. - _ "k8s.io/client-go/plugin/pkg/client/auth" - "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics/server" "sigs.k8s.io/controller-runtime/pkg/webhook" - "sigs.k8s.io/ibm-powervs-block-csi-driver/adhoc-controllers/controllers" + //+kubebuilder:scaffold:imports + + // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) + // to ensure that exec-entrypoint and run can make use of them. + _ "k8s.io/client-go/plugin/pkg/client/auth" ) var ( diff --git a/cmd/main.go b/cmd/main.go index 890d9982f..c46e1e112 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -16,14 +16,14 @@ limitations under the License. package main -//DONE +// DONE. import ( "flag" - "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/driver" - "k8s.io/klog/v2" + + "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/driver" ) func main() { diff --git a/cmd/options.go b/cmd/options.go index 59f578b1a..2179b6bd1 100644 --- a/cmd/options.go +++ b/cmd/options.go @@ -16,17 +16,17 @@ limitations under the License. package main -//DONE +// DONE. import ( "flag" "os" "strings" + "k8s.io/klog/v2" + "sigs.k8s.io/ibm-powervs-block-csi-driver/cmd/options" "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/driver" - - "k8s.io/klog/v2" ) // Options is the combined set of options for all operating modes. @@ -38,7 +38,7 @@ type Options struct { *options.NodeOptions } -// used for testing +// used for testing. var osExit = os.Exit // GetOptions parses the command line options and returns a struct that contains @@ -63,7 +63,7 @@ func GetOptions(fs *flag.FlagSet) *Options { switch { case cmd == string(driver.ControllerMode): - //controllerOptions.AddFlags(fs) + // controllerOptions.AddFlags(fs) args = os.Args[2:] mode = driver.ControllerMode @@ -73,12 +73,12 @@ func GetOptions(fs *flag.FlagSet) *Options { mode = driver.NodeMode case cmd == string(driver.AllMode): - //controllerOptions.AddFlags(fs) + // controllerOptions.AddFlags(fs) nodeOptions.AddFlags(fs) args = os.Args[2:] case strings.HasPrefix(cmd, "-"): - //controllerOptions.AddFlags(fs) + // controllerOptions.AddFlags(fs) nodeOptions.AddFlags(fs) args = os.Args[1:] @@ -95,7 +95,7 @@ func GetOptions(fs *flag.FlagSet) *Options { if *version { info, err := driver.GetVersionJSON() if err != nil { - klog.Fatalf("error while retriving the CSI driver version. err: %v", err) + klog.Fatalf("error while retrieving the CSI driver version. err: %v", err) } klog.Info(info) osExit(0) diff --git a/cmd/options/controller_options.go b/cmd/options/controller_options.go index d235f3eff..e02ca1b52 100644 --- a/cmd/options/controller_options.go +++ b/cmd/options/controller_options.go @@ -16,23 +16,23 @@ limitations under the License. package options -//DONE - -//// ControllerOptions contains options and configuration settings for the controller service. -//type ControllerOptions struct { -// // ExtraTags is a map of tags that will be attached to each dynamically provisioned -// // resource. -// ExtraTags map[string]string -// //// ExtraVolumeTags is a map of tags that will be attached to each dynamically provisioned -// //// volume. -// //// DEPRECATED: Use ExtraTags instead. -// //ExtraVolumeTags map[string]string -// // ID of the kubernetes cluster. -// KubernetesClusterID string -//} +/* +// ControllerOptions contains options and configuration settings for the controller service. +type ControllerOptions struct { + // ExtraTags is a map of tags that will be attached to each dynamically provisioned + // resource. + ExtraTags map[string]string + //// ExtraVolumeTags is a map of tags that will be attached to each dynamically provisioned + //// volume. + //// DEPRECATED: Use ExtraTags instead. + //ExtraVolumeTags map[string]string + // ID of the kubernetes cluster. + KubernetesClusterID string +} -//func (s *ControllerOptions) AddFlags(fs *flag.FlagSet) { -// //fs.Var(cliflag.NewMapStringString(&s.ExtraTags), "extra-tags", "Extra tags to attach to each dynamically provisioned resource. It is a comma separated list of key value pairs like '=,='") -// //fs.Var(cliflag.NewMapStringString(&s.ExtraVolumeTags), "extra-volume-tags", "DEPRECATED: Please use --extra-tags instead. Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '=,='") -// //fs.StringVar(&s.KubernetesClusterID, "k8s-tag-cluster-id", "", "ID of the Kubernetes cluster used for tagging provisioned EBS volumes (optional).") -//} +func (s *ControllerOptions) AddFlags(fs *flag.FlagSet) { + fs.Var(cliflag.NewMapStringString(&s.ExtraTags), "extra-tags", "Extra tags to attach to each dynamically provisioned resource. It is a comma separated list of key value pairs like '=,='") + fs.Var(cliflag.NewMapStringString(&s.ExtraVolumeTags), "extra-volume-tags", "DEPRECATED: Please use --extra-tags instead. Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '=,='") + fs.StringVar(&s.KubernetesClusterID, "k8s-tag-cluster-id", "", "ID of the Kubernetes cluster used for tagging provisioned EBS volumes (optional).") +}. +*/ diff --git a/cmd/options/node_options.go b/cmd/options/node_options.go index f376bf67d..fd133af2d 100644 --- a/cmd/options/node_options.go +++ b/cmd/options/node_options.go @@ -16,7 +16,7 @@ limitations under the License. package options -//DONE +// DONE. import ( "flag" diff --git a/cmd/options/server_options.go b/cmd/options/server_options.go index 7c1153160..e38f2c01d 100644 --- a/cmd/options/server_options.go +++ b/cmd/options/server_options.go @@ -16,7 +16,7 @@ limitations under the License. package options -//DONE +// DONE. import ( "flag" diff --git a/cmd/options_test.go b/cmd/options_test.go index 7b620170f..88d603589 100644 --- a/cmd/options_test.go +++ b/cmd/options_test.go @@ -27,7 +27,6 @@ func TestGetOptions(t *testing.T) { t *testing.T, additionalArgs []string, withServerOptions bool, - withControllerOptions bool, withNodeOptions bool, ) *Options { flagSet := flag.NewFlagSet("test-flagset", flag.ContinueOnError) @@ -85,7 +84,7 @@ func TestGetOptions(t *testing.T) { { name: "no controller mode given - expect all mode", testFunc: func(t *testing.T) { - options := testFunc(t, nil, true, true, true) + options := testFunc(t, nil, true, true) if options.DriverMode != driver.AllMode { t.Fatalf("expected driver mode to be %q but it is %q", driver.AllMode, options.DriverMode) @@ -96,7 +95,7 @@ func TestGetOptions(t *testing.T) { { name: "all mode given - expect all mode", testFunc: func(t *testing.T) { - options := testFunc(t, []string{"all"}, true, true, true) + options := testFunc(t, []string{"all"}, true, true) if options.DriverMode != driver.AllMode { t.Fatalf("expected driver mode to be %q but it is %q", driver.AllMode, options.DriverMode) @@ -106,7 +105,7 @@ func TestGetOptions(t *testing.T) { { name: "controller mode given - expect controller mode", testFunc: func(t *testing.T) { - options := testFunc(t, []string{"controller"}, true, true, false) + options := testFunc(t, []string{"controller"}, true, false) if options.DriverMode != driver.ControllerMode { t.Fatalf("expected driver mode to be %q but it is %q", driver.ControllerMode, options.DriverMode) @@ -116,7 +115,7 @@ func TestGetOptions(t *testing.T) { { name: "node mode given - expect node mode", testFunc: func(t *testing.T) { - options := testFunc(t, []string{"node"}, true, false, true) + options := testFunc(t, []string{"node"}, true, true) if options.DriverMode != driver.NodeMode { t.Fatalf("expected driver mode to be %q but it is %q", driver.NodeMode, options.DriverMode) diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index ae9b5b685..4c4b64327 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -22,7 +22,7 @@ import ( "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/util" ) -// PowerVS volume types +// PowerVS volume types. const ( VolumeTypeTier1 = "tier1" VolumeTypeTier3 = "tier3" @@ -35,7 +35,7 @@ var ( } ) -// Defaults +// Defaults. const ( // DefaultVolumeSize represents the default volume size. DefaultVolumeSize int64 = 10 * util.GiB @@ -51,7 +51,7 @@ var ( ErrAlreadyExists = errors.New("resource already exists") ) -// Disk represents a PowerVS volume +// Disk represents a PowerVS volume. type Disk struct { VolumeID string DiskType string @@ -61,11 +61,11 @@ type Disk struct { CapacityGiB int64 } -// DiskOptions represents parameters to create an PowerVS volume +// DiskOptions represents parameters to create an PowerVS volume. type DiskOptions struct { - //PowerVS options + // PowerVS options Shareable bool - //CapacityGigaBytes float64 + // CapacityGigaBytes float64 CapacityBytes int64 VolumeType string } diff --git a/pkg/cloud/metadata.go b/pkg/cloud/metadata.go index aecb2b4e3..f18f72e51 100644 --- a/pkg/cloud/metadata.go +++ b/pkg/cloud/metadata.go @@ -29,7 +29,7 @@ const ( ProviderIDValidLength = 6 ) -// Metadata is info about the instance on which the driver is running +// Metadata is info about the instance on which the driver is running. type Metadata struct { region string zone string @@ -37,22 +37,22 @@ type Metadata struct { pvmInstanceId string } -// GetRegion returns region of the instance +// GetRegion returns region of the instance. func (m *Metadata) GetRegion() string { return m.region } -// GetZone returns zone of the instance +// GetZone returns zone of the instance. func (m *Metadata) GetZone() string { return m.zone } -// GetCloudInstanceId returns cloud instance id of the instance +// GetCloudInstanceId returns cloud instance id of the instance. func (m *Metadata) GetCloudInstanceId() string { return m.cloudInstanceId } -// GetPvmInstanceId returns pvm instance id of the instance +// GetPvmInstanceId returns pvm instance id of the instance. func (m *Metadata) GetPvmInstanceId() string { return m.pvmInstanceId } @@ -85,13 +85,13 @@ func TokenizeProviderID(providerID string) (*Metadata, error) { }, nil } -// Get New Metadata Service +// Get New Metadata Service. func NewMetadataService(k8sAPIClient KubernetesAPIClient, kubeconfig string) (MetadataService, error) { klog.Info("Retrieving instance data from Kubernetes API") clientset, err := k8sAPIClient(kubeconfig) if err != nil { klog.Errorf("error creating Kubernetes API client: %v", err) - return nil, fmt.Errorf("an error occured during creation of k8s API client: %w", err) + return nil, fmt.Errorf("an error occurred during creation of k8s API client: %w", err) } klog.Info("kubernetes API is available") return KubernetesAPIInstanceInfo(clientset) diff --git a/pkg/cloud/metadata_k8s.go b/pkg/cloud/metadata_k8s.go index f73953334..17861619c 100644 --- a/pkg/cloud/metadata_k8s.go +++ b/pkg/cloud/metadata_k8s.go @@ -30,7 +30,7 @@ import ( type KubernetesAPIClient func(kubeconfig string) (kubernetes.Interface, error) -// Get default kubernetes API client +// Get default kubernetes API client. var DefaultKubernetesAPIClient = func(kubeconfig string) (kubernetes.Interface, error) { config, err := clientcmd.BuildConfigFromFlags("", kubeconfig) if err != nil { @@ -40,7 +40,7 @@ var DefaultKubernetesAPIClient = func(kubeconfig string) (kubernetes.Interface, return kubernetes.NewForConfig(config) } -// Get instance info from kubernetes API +// Get instance info from kubernetes API. func KubernetesAPIInstanceInfo(clientset kubernetes.Interface) (*Metadata, error) { nodeName := os.Getenv("CSI_NODE_NAME") if nodeName == "" { diff --git a/pkg/cloud/powervs.go b/pkg/cloud/powervs.go index f01b6f0d3..0ebb943bc 100644 --- a/pkg/cloud/powervs.go +++ b/pkg/cloud/powervs.go @@ -24,12 +24,14 @@ import ( "strings" "time" + "github.com/davecgh/go-spew/spew" + "github.com/IBM-Cloud/power-go-client/clients/instance" "github.com/IBM-Cloud/power-go-client/ibmpisession" "github.com/IBM-Cloud/power-go-client/power/models" "github.com/IBM/go-sdk-core/v5/core" "github.com/IBM/platform-services-go-sdk/resourcecontrollerv2" - "github.com/davecgh/go-spew/spew" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" "k8s.io/utils/ptr" diff --git a/pkg/cloud/powervs_node.go b/pkg/cloud/powervs_node.go index 711c43367..0375b2b8c 100644 --- a/pkg/cloud/powervs_node.go +++ b/pkg/cloud/powervs_node.go @@ -20,6 +20,7 @@ import ( "errors" "github.com/IBM-Cloud/power-go-client/power/models" + "k8s.io/klog/v2" "k8s.io/utils/ptr" ) @@ -75,7 +76,6 @@ func NewNodeUpdateScope(params NodeUpdateScopeParams) (scope *NodeUpdateScope, e func (p *powerVSCloud) GetPVMInstanceDetails(instanceID string) (*models.PVMInstance, error) { return p.pvmInstancesClient.Get(instanceID) - } func (p *powerVSCloud) UpdateStoragePoolAffinity(instanceID string) error { diff --git a/pkg/device/device.go b/pkg/device/device.go index af4ff5933..d5f77c8be 100644 --- a/pkg/device/device.go +++ b/pkg/device/device.go @@ -41,14 +41,14 @@ type LinuxDevice interface { Populate(bool) error } -// Device struct +// Device struct. type Device struct { Mapper string `json:"mapper,omitempty"` WWN string `json:"wwn,omitempty"` Slaves int `json:"slaves,omitempty"` } -// NewLinuxDevice new device with given wwn +// NewLinuxDevice new device with given wwn. func NewLinuxDevice(wwn string) LinuxDevice { return &Device{ WWN: wwn, @@ -59,7 +59,7 @@ func (d *Device) GetMapper() string { return d.Mapper } -// Populate get all linux Devices +// Populate get all linux Devices. func (d *Device) Populate(needActivePath bool) error { args := []string{"ls", "--target", "multipath"} outBytes, err := exec.Command(dmsetupcommand, args...).CombinedOutput() @@ -112,7 +112,7 @@ func (d *Device) Populate(needActivePath bool) error { return nil } -// DeleteDevice delete the multipath device +// DeleteDevice delete the multipath device. func (d *Device) DeleteDevice() (err error) { if err := retryCleanupDevice(d); err != nil { klog.Warningf("error while deleting multipath device %s: %v", d.Mapper, err) @@ -123,9 +123,8 @@ func (d *Device) DeleteDevice() (err error) { return nil } -// CreateDevice attach and create linux devices to host +// CreateDevice attach and create linux devices to host. func (d *Device) CreateDevice() (err error) { - if err = d.createLinuxDevice(); err != nil { klog.Errorf("unable to create device for wwn %s", d.WWN) return err @@ -205,7 +204,7 @@ func (d *Device) createLinuxDevice() (err error) { return fmt.Errorf("fc device not found for wwn %s", d.WWN) } -// scsiHostRescan: scans all scsi hosts +// scsiHostRescan: scans all scsi hosts. func scsiHostRescan() error { scsiPath := "/sys/class/scsi_host/" dirs, err := os.ReadDir(scsiPath) diff --git a/pkg/device/multipath.go b/pkg/device/multipath.go index 8643ef9af..705a40545 100644 --- a/pkg/device/multipath.go +++ b/pkg/device/multipath.go @@ -41,7 +41,7 @@ var ( orphanPathRegexp = regexp.MustCompile(orphanPathsPattern) ) -// getPathsCount get number of slaves for a given device +// getPathsCount get number of slaves for a given device. func getPathsCount(mapper string) (count int, err error) { // TODO: This can be achieved reading the full line processing instead of piped command statusCmd := fmt.Sprintf("dmsetup status --target multipath %s | awk 'BEGIN{RS=\" \";active=0}/[0-9]+:[0-9]+/{dev=1}/A/{if (dev == 1) active++; dev=0} END{ print active }'", mapper) @@ -54,17 +54,17 @@ func getPathsCount(mapper string) (count int, err error) { return strconv.Atoi(out) } -// isDmsetupStatusError check for command failure or empty stdout msg +// isDmsetupStatusError check for command failure or empty stdout msg. func isDmsetupStatusError(msg string) bool { return msg == "" || strings.Contains(msg, "Command failed") } -// isMultipathTimeoutError check for timeout or similar error msg +// isMultipathTimeoutError check for timeout or similar error msg. func isMultipathTimeoutError(msg string) bool { return strings.Contains(msg, "timeout") || strings.Contains(msg, "receiving packet") } -// retryCleanupDevice retry for maxtries for device Cleanup +// retryCleanupDevice retry for maxtries for device Cleanup. func retryCleanupDevice(dev *Device) error { maxTries := 10 var err error @@ -78,7 +78,7 @@ func retryCleanupDevice(dev *Device) error { return err } -// multipathDisableQueuing disable queueing on the multipath device +// multipathDisableQueuing disable queueing on the multipath device. func multipathDisableQueuing(mapper string) error { args := []string{"message", mapper, "0", "fail_if_no_path"} outBytes, err := exec.Command(dmsetupcommand, args...).CombinedOutput() @@ -92,7 +92,7 @@ func multipathDisableQueuing(mapper string) error { return nil } -// multipathRemoveDmDevice remove multipath device via dmsetup +// multipathRemoveDmDevice remove multipath device via dmsetup. func multipathRemoveDmDevice(mapper string) error { if strings.HasSuffix(mapper, "mpatha") { klog.Warning("skipping remove mpatha which is root") @@ -117,12 +117,12 @@ func multipathRemoveDmDevice(mapper string) error { return nil } -// isDmsetupRemoveError check if dmsetup remove command did not return empty, "ok" or no device msg +// isDmsetupRemoveError check if dmsetup remove command did not return empty, "ok" or no device msg. func isDmsetupRemoveError(msg string) bool { return msg != "" && !strings.Contains(msg, "ok") && !strings.Contains(msg, deviceDoesNotExist) } -// cleanupOrphanPaths find orphan paths and remove them (best effort) +// cleanupOrphanPaths find orphan paths and remove them (best effort). func cleanupOrphanPaths() { // run multipathd show paths and fetch orphan maps outBytes, err := exec.Command(multipathd, showPathsFormat...).CombinedOutput() diff --git a/pkg/device/utils.go b/pkg/device/utils.go index 5997867a1..a9b0710e6 100644 --- a/pkg/device/utils.go +++ b/pkg/device/utils.go @@ -23,7 +23,7 @@ import ( "regexp" ) -// findStringSubmatchMap: find and build the map of named groups +// findStringSubmatchMap: find and build the map of named groups. func findStringSubmatchMap(s string, r *regexp.Regexp) map[string]string { captures := make(map[string]string) match := r.FindStringSubmatch(s) @@ -38,7 +38,7 @@ func findStringSubmatchMap(s string, r *regexp.Regexp) map[string]string { return captures } -// readFirstLine: read the file line no. 1 +// readFirstLine: read the file line no. 1. func readFirstLine(filePath string) (string, error) { file, err := os.Open(filePath) a := "" @@ -68,9 +68,9 @@ func getUUID(pathname string) (string, error) { return readFirstLine(fileName) } -// deleteSdDevice: delete the scsi device by writing "1" +// deleteSdDevice: delete the scsi device by writing "1". func deleteSdDevice(deletePath string) (err error) { - //deletePath for deleting the device + // deletePath for deleting the device err = os.WriteFile(deletePath, []byte("1"), 0644) if err != nil { err = fmt.Errorf("error writing to file %s: %v", deletePath, err) diff --git a/pkg/driver/constants.go b/pkg/driver/constants.go index dc9befc07..74a3c2b67 100644 --- a/pkg/driver/constants.go +++ b/pkg/driver/constants.go @@ -16,20 +16,20 @@ limitations under the License. package driver -//DONE +// DONE. -// constants of keys in PublishContext +// constants of keys in PublishContext. const ( WWNKey = "wwn" ) -// constants of keys in volume parameters +// constants of keys in volume parameters. const ( - // VolumeTypeKey represents key for volume type + // VolumeTypeKey represents key for volume type. VolumeTypeKey = "type" ) -// constants for default command line flag values +// constants for default command line flag values. const ( DefaultCSIEndpoint = "unix://tmp/csi.sock" ) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 62e1d2f29..c571254fa 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -25,12 +25,14 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/status" gcfg "gopkg.in/gcfg.v1" + "k8s.io/klog/v2" + "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/cloud" "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/util" ) -// Supported access modes +// Supported access modes. const ( SingleNodeWriter = csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER MultiNodeMultiWriter = csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER @@ -38,7 +40,7 @@ const ( ) var ( - // controllerCaps represents the capability of controller service + // controllerCaps represents the capability of controller service. controllerCaps = []csi.ControllerServiceCapability_RPC_Type{ csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME, csi.ControllerServiceCapability_RPC_PUBLISH_UNPUBLISH_VOLUME, @@ -46,7 +48,7 @@ var ( } ) -// controllerService represents the controller service of CSI driver +// controllerService represents the controller service of CSI driver. type controllerService struct { cloud cloud.Cloud driverOptions *Options @@ -72,8 +74,7 @@ var ( NewPowerVSCloudFunc = cloud.NewPowerVSCloud ) -// newControllerService creates a new controller service -// it will print stack trace and osexit if failed to create the service +// newControllerService creates a new controller service and prints stack trace and osexit if failed to create the service. func newControllerService(driverOptions *Options) controllerService { var cloudInstanceId, zone string @@ -90,7 +91,7 @@ func newControllerService(driverOptions *Options) controllerService { } else if driverOptions.cloudconfig != "" { var cloudConfig CloudConfig config, err := os.Open(driverOptions.cloudconfig) - if nil != err { + if err != nil { klog.Fatalf("Failed to get cloud config: %v", err) } defer config.Close() @@ -128,7 +129,7 @@ func newControllerService(driverOptions *Options) controllerService { func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) { klog.V(4).Infof("CreateVolume: called with args %+v", req) volName := req.GetName() - if len(volName) == 0 { + if volName == "" { return nil, status.Error(codes.InvalidArgument, "Volume name not provided") } @@ -198,7 +199,7 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol func (d *controllerService) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) { klog.V(4).Infof("DeleteVolume: called with args: %+v", req) volumeID := req.GetVolumeId() - if len(volumeID) == 0 { + if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "Volume ID not provided") } @@ -224,7 +225,7 @@ func (d *controllerService) DeleteVolume(ctx context.Context, req *csi.DeleteVol func (d *controllerService) ControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (*csi.ControllerPublishVolumeResponse, error) { klog.V(4).Infof("ControllerPublishVolume: called with args %+v", req) volumeID := req.GetVolumeId() - if len(volumeID) == 0 { + if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "Volume ID not provided") } @@ -234,7 +235,7 @@ func (d *controllerService) ControllerPublishVolume(ctx context.Context, req *cs defer d.volumeLocks.Release(volumeID) nodeID := req.GetNodeId() - if len(nodeID) == 0 { + if nodeID == "" { return nil, status.Error(codes.InvalidArgument, "Node ID not provided") } @@ -286,7 +287,7 @@ func (d *controllerService) ControllerPublishVolume(ctx context.Context, req *cs func (d *controllerService) ControllerUnpublishVolume(ctx context.Context, req *csi.ControllerUnpublishVolumeRequest) (*csi.ControllerUnpublishVolumeResponse, error) { klog.V(4).Infof("ControllerUnpublishVolume: called with args %+v", req) volumeID := req.GetVolumeId() - if len(volumeID) == 0 { + if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "Volume ID not provided") } @@ -296,7 +297,7 @@ func (d *controllerService) ControllerUnpublishVolume(ctx context.Context, req * defer d.volumeLocks.Release(volumeID) nodeID := req.GetNodeId() - if len(nodeID) == 0 { + if nodeID == "" { return nil, status.Error(codes.InvalidArgument, "Node ID not provided") } @@ -322,7 +323,7 @@ func (d *controllerService) ControllerUnpublishVolume(ctx context.Context, req * func (d *controllerService) ControllerGetCapabilities(ctx context.Context, req *csi.ControllerGetCapabilitiesRequest) (*csi.ControllerGetCapabilitiesResponse, error) { klog.V(4).Infof("ControllerGetCapabilities: called with args %+v", req) - var caps []*csi.ControllerServiceCapability + caps := make([]*csi.ControllerServiceCapability, 0, len(controllerCaps)) for _, cap := range controllerCaps { c := &csi.ControllerServiceCapability{ Type: &csi.ControllerServiceCapability_Rpc{ @@ -349,7 +350,7 @@ func (d *controllerService) ListVolumes(ctx context.Context, req *csi.ListVolume func (d *controllerService) ValidateVolumeCapabilities(ctx context.Context, req *csi.ValidateVolumeCapabilitiesRequest) (*csi.ValidateVolumeCapabilitiesResponse, error) { klog.V(4).Infof("ValidateVolumeCapabilities: called with args %+v", req) volumeID := req.GetVolumeId() - if len(volumeID) == 0 { + if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "Volume ID not provided") } @@ -377,7 +378,7 @@ func (d *controllerService) ValidateVolumeCapabilities(ctx context.Context, req func (d *controllerService) ControllerExpandVolume(ctx context.Context, req *csi.ControllerExpandVolumeRequest) (*csi.ControllerExpandVolumeResponse, error) { klog.V(4).Infof("ControllerExpandVolume: called with args %+v", req) volumeID := req.GetVolumeId() - if len(volumeID) == 0 { + if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "Volume ID not provided") } @@ -428,7 +429,7 @@ func isValidVolumeCapabilities(volCaps []*csi.VolumeCapability) bool { return true } -// Check if the volume is shareable +// Check if the volume is shareable. func isShareableVolume(volCaps []*csi.VolumeCapability) bool { for _, c := range volCaps { mode := c.AccessMode.GetMode() diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index 0d16300aa..7a072c87d 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -148,7 +148,6 @@ func TestCreateVolume(t *testing.T) { } t.Fatalf("Unexpected error: %v", srvErr.Code()) } - }, }, { @@ -204,7 +203,6 @@ func TestCreateVolume(t *testing.T) { } t.Fatalf("Unexpected error: %v", srvErr.Code()) } - }, }, { @@ -260,7 +258,6 @@ func TestCreateVolume(t *testing.T) { } t.Fatalf("Unexpected error: %v", srvErr.Code()) } - }, }, @@ -1167,7 +1164,6 @@ func TestControllerPublishVolume(t *testing.T) { _, err := powervsDriver.ControllerPublishVolume(ctx, req) checkExpectedErrorCode(t, err, codes.Aborted) - }, }, } @@ -1423,7 +1419,6 @@ func TestControllerExpandVolume(t *testing.T) { _, err := powervsDriver.ControllerExpandVolume(ctx, tc.req) checkExpectedErrorCode(t, err, codes.Aborted) - } else { mockCloud.EXPECT().ResizeDisk(gomock.Eq(tc.req.VolumeId), gomock.Any()).Return(retSizeGiB, nil).AnyTimes() resp, err := powervsDriver.ControllerExpandVolume(ctx, tc.req) @@ -1447,7 +1442,6 @@ func TestControllerExpandVolume(t *testing.T) { t.Fatalf("Expected size %d GiB, got %d GiB", expSizeGiB, sizeGiB) } } - }) } } @@ -1509,7 +1503,6 @@ func TestIsShareableVolume(t *testing.T) { } }) } - } func checkExpectedErrorCode(t *testing.T, err error, expectedCode codes.Code) { diff --git a/pkg/driver/driver.go b/pkg/driver/driver.go index da0a6d247..cd0a91bbf 100644 --- a/pkg/driver/driver.go +++ b/pkg/driver/driver.go @@ -16,7 +16,7 @@ limitations under the License. package driver -//DONE +// DONE. import ( "context" @@ -25,7 +25,9 @@ import ( "github.com/container-storage-interface/spec/lib/go/csi" "google.golang.org/grpc" + "k8s.io/klog/v2" + "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/util" ) diff --git a/pkg/driver/identity.go b/pkg/driver/identity.go index 7019ce2c5..29e38365d 100644 --- a/pkg/driver/identity.go +++ b/pkg/driver/identity.go @@ -20,6 +20,7 @@ import ( "context" csi "github.com/container-storage-interface/spec/lib/go/csi" + "k8s.io/klog/v2" ) diff --git a/pkg/driver/mount.go b/pkg/driver/mount.go index 700c08111..71c4d337b 100644 --- a/pkg/driver/mount.go +++ b/pkg/driver/mount.go @@ -23,7 +23,7 @@ import ( "k8s.io/utils/exec" ) -// Mounter is an interface for mount operations +// Mounter is an interface for mount operations. type Mounter interface { mount.Interface diff --git a/pkg/driver/mount_test.go b/pkg/driver/mount_test.go index faa7819a1..594819a09 100644 --- a/pkg/driver/mount_test.go +++ b/pkg/driver/mount_test.go @@ -67,7 +67,6 @@ func TestMakeFile(t *testing.T) { if exists, err := mountObj.ExistsPath(targetPath); !exists { t.Fatalf("Expect no error but got: %v", err) } - } func TestExistsPath(t *testing.T) { @@ -91,7 +90,6 @@ func TestExistsPath(t *testing.T) { if exists { t.Fatalf("Expected file %s to not exist", targetPath) } - } func TestGetDeviceName(t *testing.T) { @@ -109,5 +107,4 @@ func TestGetDeviceName(t *testing.T) { if _, _, err := mountObj.GetDeviceName(targetPath); err != nil { t.Fatalf("Expect no error but got: %v", err) } - } diff --git a/pkg/driver/node.go b/pkg/driver/node.go index e5d2c1db4..bf0c85d65 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -25,8 +25,10 @@ import ( "github.com/container-storage-interface/spec/lib/go/csi" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "k8s.io/klog/v2" "k8s.io/mount-utils" + "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/cloud" "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/device" "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/util" @@ -34,19 +36,19 @@ import ( const ( - // FSTypeExt2 represents the ext2 filesystem type + // FSTypeExt2 represents the ext2 filesystem type. FSTypeExt2 = "ext2" - // FSTypeExt3 represents the ext3 filesystem type + // FSTypeExt3 represents the ext3 filesystem type. FSTypeExt3 = "ext3" - // FSTypeExt4 represents the ext4 filesystem type + // FSTypeExt4 represents the ext4 filesystem type. FSTypeExt4 = "ext4" - // FSTypeXfs represents te xfs filesystem type + // FSTypeXfs represents te xfs filesystem type. FSTypeXfs = "xfs" - // default file system type to be used when it is not provided + // default file system type to be used when it is not provided. defaultFsType = "ext4" - // defaultMaxVolumesPerInstance is the limit of volumes can be attached in the PowerVS environment - // TODO: rightnow 99 is just a placeholder, this needs to be changed post discussion with PowerVS team + // defaultMaxVolumesPerInstance is the limit of volumes can be attached in the PowerVS environment. + // TODO: rightnow 99 is just a placeholder, this needs to be changed post discussion with PowerVS team. defaultMaxVolumesPerInstance = 127 - 1 ) @@ -62,7 +64,7 @@ var ( } ) -// nodeService represents the node service of CSI driver +// nodeService represents the node service of CSI driver. type nodeService struct { cloud cloud.Cloud mounter Mounter @@ -73,8 +75,7 @@ type nodeService struct { csi.UnimplementedNodeServer } -// newNodeService creates a new node service -// it will print stack trace and osexit if failed to create the service +// newNodeService creates a new node service and prints stack trace and osexit if failed to create the service. func newNodeService(driverOptions *Options) nodeService { var cloudInstanceId, zone, instanceID string @@ -116,12 +117,12 @@ func (d *nodeService) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol klog.V(4).Infof("NodeStageVolume: called with args %+v", req) volumeID := req.GetVolumeId() - if len(volumeID) == 0 { + if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "Volume ID not provided") } target := req.GetStagingTargetPath() - if len(target) == 0 { + if target == "" { return nil, status.Error(codes.InvalidArgument, "Staging target not provided") } @@ -205,7 +206,7 @@ func (d *nodeService) stageVolume(wwn string, req *csi.NodeStageVolumeRequest) e return status.Error(codes.InvalidArgument, fmt.Sprintf("mnt is nil within volume capability for volumeID %s", req.VolumeId)) } fsType := mnt.GetFsType() - if len(fsType) == 0 { + if fsType == "" { fsType = defaultFsType } var mountOptions []string @@ -257,12 +258,12 @@ func (d *nodeService) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag klog.V(4).Infof("NodeUnstageVolume: called with args %+v", req) volumeID := req.GetVolumeId() - if len(volumeID) == 0 { + if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "Volume ID not provided") } target := req.GetStagingTargetPath() - if len(target) == 0 { + if target == "" { return nil, status.Error(codes.InvalidArgument, "Staging target not provided") } @@ -305,7 +306,7 @@ func (d *nodeService) nodeUnstageVolume(req *csi.NodeUnstageVolumeRequest) error // Delete device klog.V(5).Infof("deleting device %s for volumeID %s", deviceName, volumeID) - //check if device is mounted or has holders + // check if device is mounted or has holders isDirMounted, err := d.mounter.IsMountPoint(stagingTarget) if err != nil { return status.Errorf(codes.Internal, "failed to check likely mount point for vol %s target %q: %v", volumeID, stagingTarget, err) @@ -322,7 +323,6 @@ func (d *nodeService) nodeUnstageVolume(req *csi.NodeUnstageVolumeRequest) error } func (d *nodeService) deleteDevice(deviceName string) error { - wwn, err := GetDeviceWWN(deviceName) if err != nil { return err @@ -345,12 +345,12 @@ func (d *nodeService) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandV klog.V(4).Infof("NodeExpandVolume: called with args %+v", req) volumeID := req.GetVolumeId() - if len(volumeID) == 0 { + if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "Volume ID not provided") } volumePath := req.GetVolumePath() - if len(volumePath) == 0 { + if volumePath == "" { return nil, status.Error(codes.InvalidArgument, "Volume path not provided") } @@ -415,17 +415,17 @@ func (d *nodeService) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandV func (d *nodeService) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolumeRequest) (*csi.NodePublishVolumeResponse, error) { klog.V(4).Infof("NodePublishVolume: called with args %+v", req) volumeID := req.GetVolumeId() - if len(volumeID) == 0 { + if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "Volume ID not provided") } source := req.GetStagingTargetPath() - if len(source) == 0 { + if source == "" { return nil, status.Error(codes.InvalidArgument, "Staging target not provided") } target := req.GetTargetPath() - if len(target) == 0 { + if target == "" { return nil, status.Error(codes.InvalidArgument, "Target path not provided") } @@ -471,12 +471,12 @@ func (d *nodeService) NodePublishVolume(ctx context.Context, req *csi.NodePublis func (d *nodeService) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublishVolumeRequest) (*csi.NodeUnpublishVolumeResponse, error) { klog.V(4).Infof("NodeUnpublishVolume: called with args %+v", req) volumeID := req.GetVolumeId() - if len(volumeID) == 0 { + if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "Volume ID not provided") } target := req.GetTargetPath() - if len(target) == 0 { + if target == "" { return nil, status.Error(codes.InvalidArgument, "Target path not provided") } @@ -569,7 +569,7 @@ func (d *nodeService) NodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVo func (d *nodeService) NodeGetCapabilities(ctx context.Context, req *csi.NodeGetCapabilitiesRequest) (*csi.NodeGetCapabilitiesResponse, error) { klog.V(4).Infof("NodeGetCapabilities: called with args %+v", req) - var caps []*csi.NodeServiceCapability + caps := make([]*csi.NodeServiceCapability, 0, len(nodeCaps)) for _, cap := range nodeCaps { c := &csi.NodeServiceCapability{ Type: &csi.NodeServiceCapability_Rpc{ @@ -676,7 +676,7 @@ func (d *nodeService) nodePublishVolumeForFileSystem(req *csi.NodePublishVolumeR } fsType := mode.Mount.GetFsType() - if len(fsType) == 0 { + if fsType == "" { fsType = defaultFsType } @@ -709,7 +709,7 @@ func (d *nodeService) nodePublishVolumeForFileSystem(req *csi.NodePublishVolumeR return nil } -// getVolumesLimit returns the limit of volumes that the node supports +// getVolumesLimit returns the limit of volumes that the node supports. func (d *nodeService) getVolumesLimit() int64 { if d.driverOptions.volumeAttachLimit >= 0 { return d.driverOptions.volumeAttachLimit diff --git a/pkg/driver/node_test.go b/pkg/driver/node_test.go index 4ef7a2d08..d4377183a 100644 --- a/pkg/driver/node_test.go +++ b/pkg/driver/node_test.go @@ -24,25 +24,26 @@ import ( "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/cloud" cloudmocks "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/cloud/mocks" "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/device" - mocks "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/driver/mocks" + "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/driver/mocks" "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/util" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) -// constants of keys in PublishContext +// constants of keys in PublishContext. const ( - // devicePathKey represents key for device path in PublishContext - // devicePath is the device path where the volume is attached to + // devicePathKey represents key for device path in PublishContext. + // devicePath is the device path where the volume is attached to. DevicePathKey = "devicePath" ) -// constants of keys in VolumeContext +// constants of keys in VolumeContext. const ( - // VolumeAttributePartition represents key for partition config in VolumeContext - // this represents the partition number on a device used to mount + // VolumeAttributePartition represents key for partition config in VolumeContext. + // It represents the partition number on a device used to mount. VolumeAttributePartition = "partition" + testDir = "./test" ) var ( @@ -50,7 +51,6 @@ var ( ) func TestNodeStageVolume(t *testing.T) { - var ( targetPath = "/tmp/test/path" devicePath = "/dev/fake" @@ -75,7 +75,6 @@ func TestNodeStageVolume(t *testing.T) { mockDevice.EXPECT().Populate(false).Return(nil) mockDevice.EXPECT().GetMapper().Return(devicePath).MinTimes(2) mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Any()).Return(true, nil) - } ) testCases := []struct { @@ -884,7 +883,6 @@ func TestNodePublishVolume(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, tc.testFunc) } - } func TestNodeUnpublishVolume(t *testing.T) { @@ -1124,7 +1122,6 @@ func TestNodeGetInfo(t *testing.T) { if resp.GetMaxVolumesPerNode() != tc.expMaxVolumes { t.Fatalf("Expected %d max volumes per node, got %d", tc.expMaxVolumes, resp.GetMaxVolumesPerNode()) } - }) } } @@ -1140,7 +1137,7 @@ func TestNodeGetVolumeStats(t *testing.T) { defer mockCtl.Finish() mockStatUtils := mocks.NewMockStatsUtils(mockCtl) - volumePath := "./test" + volumePath := testDir var mockCapacity int64 = 100 mockStatUtils.EXPECT().IsPathNotExist(volumePath).Return(false) mockStatUtils.EXPECT().IsBlockDevice(volumePath).Return(true, nil) @@ -1165,7 +1162,7 @@ func TestNodeGetVolumeStats(t *testing.T) { defer mockCtl.Finish() mockStatUtils := mocks.NewMockStatsUtils(mockCtl) - volumePath := "./test" + volumePath := testDir mockStatUtils.EXPECT().IsPathNotExist(volumePath).Return(true) driver := &nodeService{stats: mockStatUtils} @@ -1181,7 +1178,7 @@ func TestNodeGetVolumeStats(t *testing.T) { defer mockCtl.Finish() mockStatUtils := mocks.NewMockStatsUtils(mockCtl) - volumePath := "./test" + volumePath := testDir mockStatUtils.EXPECT().IsPathNotExist(volumePath).Return(false) mockStatUtils.EXPECT().IsBlockDevice(volumePath).Return(false, errors.New("Error checking for block device")) driver := &nodeService{stats: mockStatUtils} @@ -1198,7 +1195,7 @@ func TestNodeGetVolumeStats(t *testing.T) { defer mockCtl.Finish() mockStatUtils := mocks.NewMockStatsUtils(mockCtl) - volumePath := "./test" + volumePath := "testDir" mockStatUtils.EXPECT().IsPathNotExist(volumePath).Return(false) mockStatUtils.EXPECT().IsBlockDevice(volumePath).Return(true, nil) mockStatUtils.EXPECT().DeviceInfo(volumePath).Return(int64(0), errors.New("Error collecting block device info")) @@ -1218,7 +1215,7 @@ func TestNodeGetVolumeStats(t *testing.T) { defer mockCtl.Finish() mockStatUtils := mocks.NewMockStatsUtils(mockCtl) - volumePath := "./test" + volumePath := "testDir" mockStatUtils.EXPECT().IsPathNotExist(volumePath).Return(false) mockStatUtils.EXPECT().IsBlockDevice(volumePath).Return(false, nil) var statUnit int64 = 0 diff --git a/pkg/driver/sanity_test.go b/pkg/driver/sanity_test.go index 8cea7e0b3..724774945 100644 --- a/pkg/driver/sanity_test.go +++ b/pkg/driver/sanity_test.go @@ -87,22 +87,22 @@ func createDir(targetPath string) (string, error) { return targetPath, nil } -// Fake State interface methods implementation for getting +// Fake State interface methods implementation for getting. type MockStatSanity struct { targetPath string } -// FSInfo ... +// FSInfo. func (su *MockStatSanity) FSInfo(path string) (int64, int64, int64, int64, int64, int64, error) { return 1, 1, 1, 1, 1, 1, nil } -// DeviceInfo ... +// DeviceInfo. func (su *MockStatSanity) DeviceInfo(path string) (int64, error) { return 1, nil } -// IsBlockDevice .. +// IsBlockDevice. func (su *MockStatSanity) IsBlockDevice(devicePath string) (bool, error) { if !strings.Contains(devicePath, su.targetPath) { return false, errors.New("not a valid path") @@ -139,7 +139,6 @@ func (p *fakeCloudProvider) GetPVMInstanceByName(name string) (*cloud.PVMInstanc DiskType: "tier3", Name: name, }, nil - } func (p *fakeCloudProvider) GetPVMInstanceByID(instanceID string) (*cloud.PVMInstance, error) { @@ -151,16 +150,13 @@ func (p *fakeCloudProvider) GetPVMInstanceByID(instanceID string) (*cloud.PVMIns } func (p *fakeCloudProvider) GetPVMInstanceDetails(instanceID string) (*models.PVMInstance, error) { - return &models.PVMInstance{ PvmInstanceID: &instanceID, ServerName: &strings.Split(instanceID, "-")[0], }, nil - } func (p *fakeCloudProvider) UpdateStoragePoolAffinity(instanceID string) error { - return nil } @@ -168,7 +164,7 @@ func (c *fakeCloudProvider) CreateDisk(volumeName string, diskOptions *cloud.Dis r1 := rand.New(rand.NewSource(time.Now().UnixNano())) if existingDisk, ok := c.disks[volumeName]; ok { - //Already Created volume + // Already Created volume if existingDisk.Disk.CapacityGiB != util.BytesToGiB(diskOptions.CapacityBytes) { return nil, errors.New("disk Already exists") } else { @@ -216,14 +212,13 @@ func (c *fakeCloudProvider) WaitForVolumeState(volumeID, expectedState string) e } func (c *fakeCloudProvider) GetDiskByName(name string) (*cloud.Disk, error) { - var disks []*fakeDisk + disks := make([]*fakeDisk, 0, len(c.disks)) for _, d := range c.disks { disks = append(disks, d) } if len(disks) > 1 { return nil, cloud.ErrAlreadyExists } else if len(disks) == 1 { - return disks[0].Disk, nil } return nil, nil diff --git a/pkg/driver/stats.go b/pkg/driver/stats.go index 1b4188092..bc5112df1 100644 --- a/pkg/driver/stats.go +++ b/pkg/driver/stats.go @@ -8,10 +8,11 @@ import ( "strings" "golang.org/x/sys/unix" + "k8s.io/kubernetes/pkg/volume/util/fs" ) -// StatsUtils ... +// StatsUtils. type StatsUtils interface { FSInfo(path string) (int64, int64, int64, int64, int64, int64, error) IsBlockDevice(devicePath string) (bool, error) @@ -19,11 +20,11 @@ type StatsUtils interface { IsPathNotExist(path string) bool } -// VolumeStatUtils ... +// VolumeStatUtils. type VolumeStatUtils struct { } -// IsPathNotExist ... +// IsPathNotExist returns true if a particular path does not exists. func (su *VolumeStatUtils) IsPathNotExist(path string) bool { var stat unix.Stat_t err := unix.Stat(path, &stat) @@ -35,7 +36,7 @@ func (su *VolumeStatUtils) IsPathNotExist(path string) bool { return false } -// IsBlockDevice ... +// IsBlockDevice returns true if the path provided is a block device. func (su *VolumeStatUtils) IsBlockDevice(devicePath string) (bool, error) { var stat unix.Stat_t err := unix.Stat(devicePath, &stat) @@ -46,7 +47,7 @@ func (su *VolumeStatUtils) IsBlockDevice(devicePath string) (bool, error) { return (stat.Mode & unix.S_IFMT) == unix.S_IFBLK, nil } -// DeviceInfo ... +// DeviceInfo returns the size of the block device in bytes. func (su *VolumeStatUtils) DeviceInfo(devicePath string) (int64, error) { output, err := exec.Command("blockdev", "--getsize64", devicePath).CombinedOutput() if err != nil { @@ -61,7 +62,7 @@ func (su *VolumeStatUtils) DeviceInfo(devicePath string) (int64, error) { return gotSizeBytes, nil } -// FSInfo ... +// FSInfo returns the information related to the FS. func (su *VolumeStatUtils) FSInfo(path string) (int64, int64, int64, int64, int64, int64, error) { return fs.Info(path) } diff --git a/pkg/driver/version.go b/pkg/driver/version.go index 2b3ac246b..3d1e620f3 100644 --- a/pkg/driver/version.go +++ b/pkg/driver/version.go @@ -22,7 +22,7 @@ import ( "runtime" ) -// These are set during build time via -ldflags +// These are set during build time via -ldflags. var ( driverVersion string gitCommit string diff --git a/pkg/util/util.go b/pkg/util/util.go index cbfbddfd5..e19b7e955 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -16,7 +16,7 @@ limitations under the License. package util -//DONE +// DONE. import ( "fmt" @@ -33,24 +33,22 @@ const ( GiB = 1024 * 1024 * 1024 ) -// RoundUpBytes rounds up the volume size in bytes upto multiplications of GiB -// in the unit of Bytes +// RoundUpBytes converts the volume size in bytes to multiples of GiB, as bytes. func RoundUpBytes(volumeSizeBytes int64) int64 { return roundUpSize(volumeSizeBytes, GiB) * GiB } -// RoundUpGiB rounds up the volume size in bytes upto multiplications of GiB -// in the unit of GiB +// RoundUpGiB rounds up the volume size in bytes to multiples of GiB, as GiB. func RoundUpGiB(volumeSizeBytes int64) int64 { return roundUpSize(volumeSizeBytes, GiB) } -// BytesToGiB converts Bytes to GiB +// BytesToGiB converts Bytes to GiB. func BytesToGiB(volumeSizeBytes int64) int64 { return volumeSizeBytes / GiB } -// GiBToBytes converts GiB to Bytes +// GiBToBytes converts GiB to Bytes. func GiBToBytes(volumeSizeGiB int64) int64 { return volumeSizeGiB * GiB } @@ -78,7 +76,7 @@ func ParseEndpoint(endpoint string) (string, string, error) { return scheme, addr, nil } -// TODO: check division by zero and int overflow +// TODO: check division by zero and int overflow. func roundUpSize(volumeSizeBytes int64, allocationUnitBytes int64) int64 { return (volumeSizeBytes + allocationUnitBytes - 1) / allocationUnitBytes } diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index 9970e79fe..a56e2dd2f 100644 --- a/pkg/util/util_test.go +++ b/pkg/util/util_test.go @@ -114,7 +114,6 @@ func TestParseEndpoint(t *testing.T) { if err.Error() != tc.expErr.Error() { t.Fatalf("Expecting err: expected %v, got %v", tc.expErr, err) } - } else { if err != nil { t.Fatalf("err is not nil. got: %v", err) @@ -129,7 +128,6 @@ func TestParseEndpoint(t *testing.T) { } }) } - } func TestGetAccessModes(t *testing.T) { diff --git a/tests/e2e/driver/driver.go b/tests/e2e/driver/driver.go index ec57af6fa..767b0a685 100644 --- a/tests/e2e/driver/driver.go +++ b/tests/e2e/driver/driver.go @@ -27,13 +27,13 @@ type PVTestDriver interface { PreProvisionedVolumeTestDriver } -// DynamicPVTestDriver represents an interface for a CSI driver that supports DynamicPV +// DynamicPVTestDriver represents an interface for a CSI driver that supports DynamicPV. type DynamicPVTestDriver interface { // GetDynamicProvisionStorageClass returns a StorageClass dynamic provision Persistent Volume GetDynamicProvisionStorageClass(parameters map[string]string, mountOptions []string, reclaimPolicy *v1.PersistentVolumeReclaimPolicy, volumeExpansion *bool, bindingMode *storagev1.VolumeBindingMode, allowedTopologyValues []string, namespace string) *storagev1.StorageClass } -// PreProvisionedVolumeTestDriver represents an interface for a CSI driver that supports pre-provisioned volume +// PreProvisionedVolumeTestDriver represents an interface for a CSI driver that supports pre-provisioned volume. type PreProvisionedVolumeTestDriver interface { // GetPersistentVolume returns a PersistentVolume with pre-provisioned volumeHandle GetPersistentVolume(volumeID string, fsType string, size string, reclaimPolicy *v1.PersistentVolumeReclaimPolicy, namespace string) *v1.PersistentVolume diff --git a/tests/e2e/driver/powervs_csi_driver.go b/tests/e2e/driver/powervs_csi_driver.go index 23eeb08a0..8daeb2747 100644 --- a/tests/e2e/driver/powervs_csi_driver.go +++ b/tests/e2e/driver/powervs_csi_driver.go @@ -23,6 +23,7 @@ import ( storagev1 "k8s.io/api/storage/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + powervscsidriver "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/driver" ) @@ -30,12 +31,12 @@ const ( True = "true" ) -// Implement DynamicPVTestDriver interface +// Implement DynamicPVTestDriver interface. type powervsCSIDriver struct { driverName string } -// InitPowervsCSIDriver returns powervsCSIDriver that implements DynamicPVTestDriver interface +// InitPowervsCSIDriver returns powervsCSIDriver that implements DynamicPVTestDriver interface. func InitPowervsCSIDriver() PVTestDriver { return &powervsCSIDriver{ driverName: powervscsidriver.DriverName, @@ -83,7 +84,7 @@ func (d *powervsCSIDriver) GetPersistentVolume(volumeID string, fsType string, s Spec: v1.PersistentVolumeSpec{ AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, Capacity: v1.ResourceList{ - v1.ResourceName(v1.ResourceStorage): resource.MustParse(size), + v1.ResourceStorage: resource.MustParse(size), }, PersistentVolumeReclaimPolicy: pvReclaimPolicy, PersistentVolumeSource: v1.PersistentVolumeSource{ @@ -97,7 +98,7 @@ func (d *powervsCSIDriver) GetPersistentVolume(volumeID string, fsType string, s } } -// GetParameters returns the parameters specific for this driver +// GetParameters returns the parameters specific for this driver. func GetParameters(volumeType string, fsType string) map[string]string { parameters := map[string]string{ "type": volumeType, diff --git a/tests/e2e/dynamic_provisioning.go b/tests/e2e/dynamic_provisioning.go index 5c702eecc..34958374b 100644 --- a/tests/e2e/dynamic_provisioning.go +++ b/tests/e2e/dynamic_provisioning.go @@ -25,6 +25,7 @@ import ( clientset "k8s.io/client-go/kubernetes" "k8s.io/kubernetes/test/e2e/framework" admissionapi "k8s.io/pod-security-admission/api" + powervscloud "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/cloud" powervscsidriver "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/driver" "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" diff --git a/tests/e2e/pre_provisioning.go b/tests/e2e/pre_provisioning.go index fbd4be2aa..79a2c161f 100644 --- a/tests/e2e/pre_provisioning.go +++ b/tests/e2e/pre_provisioning.go @@ -22,9 +22,6 @@ import ( "os" "time" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" "k8s.io/kubernetes/test/e2e/framework" @@ -34,6 +31,9 @@ import ( powervscsidriver "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/driver" "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/testsuites" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" ) const ( diff --git a/tests/e2e/testsuites/dynamically_provisioned_cmd_volume_tester.go b/tests/e2e/testsuites/dynamically_provisioned_cmd_volume_tester.go index 37556777c..564b3d3e6 100644 --- a/tests/e2e/testsuites/dynamically_provisioned_cmd_volume_tester.go +++ b/tests/e2e/testsuites/dynamically_provisioned_cmd_volume_tester.go @@ -17,15 +17,17 @@ limitations under the License. package testsuites import ( - . "github.com/onsi/ginkgo/v2" v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" + "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" + + . "github.com/onsi/ginkgo/v2" ) // DynamicallyProvisionedCmdVolumeTest will provision required StorageClass(es), PVC(s) and Pod(s) -// Waiting for the PV provisioner to create a new PV -// Testing if the Pod(s) Cmd is run with a 0 exit code +// waits for the PV provisioner to create a new PV and +// tests if the Pod(s) Cmd is run with a 0 exit code. type DynamicallyProvisionedCmdVolumeTest struct { CSIDriver driver.DynamicPVTestDriver Pods []PodDetails diff --git a/tests/e2e/testsuites/dynamically_provisioned_collocated_pod_tester.go b/tests/e2e/testsuites/dynamically_provisioned_collocated_pod_tester.go index ae38bf266..0fb3a0eaa 100644 --- a/tests/e2e/testsuites/dynamically_provisioned_collocated_pod_tester.go +++ b/tests/e2e/testsuites/dynamically_provisioned_collocated_pod_tester.go @@ -17,17 +17,17 @@ limitations under the License. package testsuites import ( - "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" - v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" + "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" + . "github.com/onsi/ginkgo/v2" ) // DynamicallyProvisionedCollocatedPodTest will provision required StorageClass(es), PVC(s) and Pod(s) -// Waiting for the PV provisioner to create a new PV -// Testing if multiple Pod(s) can write simultaneously +// waits for the PV provisioner to create a new PV and +// tests if multiple Pod(s) can write simultaneously. type DynamicallyProvisionedCollocatedPodTest struct { CSIDriver driver.DynamicPVTestDriver Pods []PodDetails @@ -54,5 +54,4 @@ func (t *DynamicallyProvisionedCollocatedPodTest) Run(client clientset.Interface tpod.WaitForRunning() nodeName = tpod.pod.Spec.NodeName } - } diff --git a/tests/e2e/testsuites/dynamically_provisioned_delete_pod_tester.go b/tests/e2e/testsuites/dynamically_provisioned_delete_pod_tester.go index dc5382d88..93f03dd0a 100644 --- a/tests/e2e/testsuites/dynamically_provisioned_delete_pod_tester.go +++ b/tests/e2e/testsuites/dynamically_provisioned_delete_pod_tester.go @@ -17,15 +17,17 @@ limitations under the License. package testsuites import ( - . "github.com/onsi/ginkgo/v2" v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" + "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" + + . "github.com/onsi/ginkgo/v2" ) // DynamicallyProvisionedDeletePodTest will provision required StorageClass and Deployment -// Testing if the Pod can write and read to mounted volumes -// Deleting a pod, and again testing if the Pod can write and read to mounted volumes +// tests if the Pod can write and read to mounted volumes +// deletes a pod, and again tests if the Pod can write and read to mounted volumes. type DynamicallyProvisionedDeletePodTest struct { CSIDriver driver.DynamicPVTestDriver Pod PodDetails diff --git a/tests/e2e/testsuites/dynamically_provisioned_read_only_volume_tester.go b/tests/e2e/testsuites/dynamically_provisioned_read_only_volume_tester.go index e5c263b87..4c368f1ad 100644 --- a/tests/e2e/testsuites/dynamically_provisioned_read_only_volume_tester.go +++ b/tests/e2e/testsuites/dynamically_provisioned_read_only_volume_tester.go @@ -20,10 +20,10 @@ import ( "fmt" v1 "k8s.io/api/core/v1" + clientset "k8s.io/client-go/kubernetes" "k8s.io/kubernetes/test/e2e/framework" - "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" - clientset "k8s.io/client-go/kubernetes" + "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -32,8 +32,8 @@ import ( const expectedReadOnlyLog = "Read-only file system" // DynamicallyProvisionedReadOnlyVolumeTest will provision required StorageClass(es), PVC(s) and Pod(s) -// Waiting for the PV provisioner to create a new PV -// Testing that the Pod(s) cannot write to the volume when mounted +// waits for the PV provisioner to create a new PV +// tests that the Pod(s) cannot write to the volume when mounted. type DynamicallyProvisionedReadOnlyVolumeTest struct { CSIDriver driver.DynamicPVTestDriver Pods []PodDetails diff --git a/tests/e2e/testsuites/dynamically_provisioned_reclaim_policy_tester.go b/tests/e2e/testsuites/dynamically_provisioned_reclaim_policy_tester.go index 94d293b65..ac96d8f84 100644 --- a/tests/e2e/testsuites/dynamically_provisioned_reclaim_policy_tester.go +++ b/tests/e2e/testsuites/dynamically_provisioned_reclaim_policy_tester.go @@ -17,15 +17,15 @@ limitations under the License. package testsuites import ( - "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/cloud" - "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" - v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" + + "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/cloud" + "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" ) // DynamicallyProvisionedReclaimPolicyTest will provision required PV(s) and PVC(s) -// Testing the correct behavior for different reclaimPolicies +// testing the correct behavior for different reclaimPolicies. type DynamicallyProvisionedReclaimPolicyTest struct { CSIDriver driver.DynamicPVTestDriver Volumes []VolumeDetails diff --git a/tests/e2e/testsuites/dynamically_provisioned_resize_volume_tester.go b/tests/e2e/testsuites/dynamically_provisioned_resize_volume_tester.go index 0eb22d3c7..c3e78e9b7 100644 --- a/tests/e2e/testsuites/dynamically_provisioned_resize_volume_tester.go +++ b/tests/e2e/testsuites/dynamically_provisioned_resize_volume_tester.go @@ -24,12 +24,13 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clientset "k8s.io/client-go/kubernetes" "k8s.io/kubernetes/test/e2e/framework" + "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/util" "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" . "github.com/onsi/ginkgo/v2" - clientset "k8s.io/client-go/kubernetes" ) // DynamicallyProvisionedResizeVolumeTest will provision required StorageClass(es), PVC(s) and Pod(s) @@ -67,8 +68,8 @@ func (t *DynamicallyProvisionedResizeVolumeTest) Run(client clientset.Interface, updatedSize := updatedPvc.Spec.Resources.Requests["storage"] By("checking the resizing PV result") - error := WaitForPvToResize(client, namespace, updatedPvc.Spec.VolumeName, updatedSize, 10*time.Minute, 5*time.Second) - framework.ExpectNoError(error) + resizeErr := WaitForPvToResize(client, namespace, updatedPvc.Spec.VolumeName, updatedSize, 10*time.Minute, 5*time.Second) + framework.ExpectNoError(resizeErr) By("Validate volume can be attached") tpod := NewTestPod(client, namespace, t.Pod.Cmd) tpod.SetupVolume(tpvc.persistentVolumeClaim, volume.VolumeMount.NameGenerate+"1", volume.VolumeMount.MountPathGenerate+"1", volume.VolumeMount.ReadOnly) @@ -77,10 +78,9 @@ func (t *DynamicallyProvisionedResizeVolumeTest) Run(client clientset.Interface, By("checking that the pods is running") tpod.WaitForSuccess() defer tpod.Cleanup() - } -// WaitForPvToResize waiting for pvc size to be resized to desired size +// WaitForPvToResize waiting for pvc size to be resized to desired size. func WaitForPvToResize(c clientset.Interface, ns *v1.Namespace, pvName string, desiredSize resource.Quantity, timeout time.Duration, interval time.Duration) error { By(fmt.Sprintf("Waiting up to %v for pv in namespace %q to be complete", timeout, ns.Name)) for start := time.Now(); time.Since(start) < timeout; time.Sleep(interval) { diff --git a/tests/e2e/testsuites/dynamically_provisioned_topology_aware_volume_tester.go b/tests/e2e/testsuites/dynamically_provisioned_topology_aware_volume_tester.go index 1270118f4..6631a5ad3 100644 --- a/tests/e2e/testsuites/dynamically_provisioned_topology_aware_volume_tester.go +++ b/tests/e2e/testsuites/dynamically_provisioned_topology_aware_volume_tester.go @@ -19,18 +19,18 @@ package testsuites import ( "fmt" - "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" - v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" + "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" + . "github.com/onsi/ginkgo/v2" ) // DynamicallyProvisionedTopologyAwareVolumeTest will provision required StorageClass(es), PVC(s) and Pod(s) // Waiting for the PV provisioner to create a new PV -// Testing if the Pod(s) can write and read to mounted volumes -// Validate PVs have expected PV nodeAffinity +// tests if the Pod(s) can write and read to mounted volumes and +// validates PVs have expected PV nodeAffinity. type DynamicallyProvisionedTopologyAwareVolumeTest struct { CSIDriver driver.DynamicPVTestDriver Pods []PodDetails @@ -61,6 +61,5 @@ func (t *DynamicallyProvisionedTopologyAwareVolumeTest) Run(client clientset.Int tpvcs[n].WaitForBound() tpvcs[n].ValidateProvisionedPersistentVolume() } - } } diff --git a/tests/e2e/testsuites/pre_provisioned_read_only_volume_tester.go b/tests/e2e/testsuites/pre_provisioned_read_only_volume_tester.go index b264866c3..fa073f2f8 100644 --- a/tests/e2e/testsuites/pre_provisioned_read_only_volume_tester.go +++ b/tests/e2e/testsuites/pre_provisioned_read_only_volume_tester.go @@ -22,6 +22,7 @@ import ( v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" "k8s.io/kubernetes/test/e2e/framework" + "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" . "github.com/onsi/ginkgo/v2" @@ -29,7 +30,7 @@ import ( ) // PreProvisionedReadOnlyVolumeTest will provision required PV(s), PVC(s) and Pod(s) -// Testing that the Pod(s) cannot write to the volume when mounted +// tests that the Pod(s) cannot write to the volume when mounted. type PreProvisionedReadOnlyVolumeTest struct { CSIDriver driver.PreProvisionedVolumeTestDriver Pods []PodDetails diff --git a/tests/e2e/testsuites/pre_provisioned_reclaim_policy_tester.go b/tests/e2e/testsuites/pre_provisioned_reclaim_policy_tester.go index a69132685..8526a30ca 100644 --- a/tests/e2e/testsuites/pre_provisioned_reclaim_policy_tester.go +++ b/tests/e2e/testsuites/pre_provisioned_reclaim_policy_tester.go @@ -17,14 +17,14 @@ limitations under the License. package testsuites import ( - "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" - v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" + + "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" ) // PreProvisionedReclaimPolicyTest will provision required PV(s) and PVC(s) -// Testing the correct behavior for different reclaimPolicies +// Testing the correct behavior for different reclaimPolicies. type PreProvisionedReclaimPolicyTest struct { CSIDriver driver.PreProvisionedVolumeTestDriver Volumes []VolumeDetails diff --git a/tests/e2e/testsuites/pre_provisioned_volume_tester.go b/tests/e2e/testsuites/pre_provisioned_volume_tester.go index 58687d93a..591571b32 100644 --- a/tests/e2e/testsuites/pre_provisioned_volume_tester.go +++ b/tests/e2e/testsuites/pre_provisioned_volume_tester.go @@ -17,21 +17,22 @@ limitations under the License. package testsuites import ( - . "github.com/onsi/ginkgo/v2" v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" + "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" + + . "github.com/onsi/ginkgo/v2" ) // PreProvisionedVolumeTest will provision required PV(s), PVC(s) and Pod(s) -// Testing if the Pod(s) can write and read to mounted volumes +// tests if the Pod(s) can write and read to mounted volumes. type PreProvisionedVolumeTest struct { CSIDriver driver.PreProvisionedVolumeTestDriver Pods []PodDetails } func (t *PreProvisionedVolumeTest) Run(client clientset.Interface, namespace *v1.Namespace) { - for _, pod := range t.Pods { tpod, cleanup := pod.SetupWithPreProvisionedVolumes(client, namespace, t.CSIDriver) // defer must be called here for resources not get removed before using them diff --git a/tests/e2e/testsuites/specs.go b/tests/e2e/testsuites/specs.go index 5b487b201..897b21839 100644 --- a/tests/e2e/testsuites/specs.go +++ b/tests/e2e/testsuites/specs.go @@ -22,6 +22,7 @@ import ( v1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" clientset "k8s.io/client-go/kubernetes" + "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e/driver" . "github.com/onsi/ginkgo/v2" diff --git a/tests/e2e/testsuites/testsuites.go b/tests/e2e/testsuites/testsuites.go index 5ab5c8c41..6c83ae3bb 100644 --- a/tests/e2e/testsuites/testsuites.go +++ b/tests/e2e/testsuites/testsuites.go @@ -22,8 +22,6 @@ import ( "math/rand" "time" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" apps "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" @@ -37,14 +35,18 @@ import ( e2epodoutput "k8s.io/kubernetes/test/e2e/framework/pod/output" e2epv "k8s.io/kubernetes/test/e2e/framework/pv" imageutils "k8s.io/kubernetes/test/utils/image" + powervscloud "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/cloud" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" ) const ( execTimeout = 10 * time.Second // Some pods can take much longer to get ready due to volume attach/detach latency. slowPodStartTimeout = 15 * time.Minute - // Description that will printed during tests + // Description that will printed during tests. failedConditionDescription = "Error status code" ) @@ -123,7 +125,6 @@ func NewTestPersistentVolumeClaim(c clientset.Interface, ns *v1.Namespace, claim namespace: ns, storageClass: sc, } - } func NewTestPersistentVolumeClaimWithDataSource(c clientset.Interface, ns *v1.Namespace, claimSize string, volumeMode VolumeMode, sc *storagev1.StorageClass, dataSource *v1.TypedLocalObjectReference) *TestPersistentVolumeClaim { @@ -202,7 +203,7 @@ func generatePVC(namespace, storageClassName, claimSize string, volumeMode v1.Pe }, Resources: v1.VolumeResourceRequirements{ Requests: v1.ResourceList{ - v1.ResourceName(v1.ResourceStorage): resource.MustParse(claimSize), + v1.ResourceStorage: resource.MustParse(claimSize), }, }, VolumeMode: &volumeMode, @@ -220,11 +221,11 @@ func (t *TestPersistentVolumeClaim) ValidateProvisionedPersistentVolume() { framework.ExpectNoError(err) // Check sizes - expectedCapacity := t.requestedPersistentVolumeClaim.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)] - claimCapacity := t.persistentVolumeClaim.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)] + expectedCapacity := t.requestedPersistentVolumeClaim.Spec.Resources.Requests[v1.ResourceStorage] + claimCapacity := t.persistentVolumeClaim.Spec.Resources.Requests[v1.ResourceStorage] Expect(claimCapacity.Value()).To(Equal(expectedCapacity.Value()), "claimCapacity is not equal to requestedCapacity") - pvCapacity := t.persistentVolume.Spec.Capacity[v1.ResourceName(v1.ResourceStorage)] + pvCapacity := t.persistentVolume.Spec.Capacity[v1.ResourceStorage] Expect(pvCapacity.Value()).To(Equal(expectedCapacity.Value()), "pvCapacity is not equal to requestedCapacity") // Check PV properties @@ -242,7 +243,6 @@ func (t *TestPersistentVolumeClaim) ValidateProvisionedPersistentVolume() { for _, v := range t.persistentVolume.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions[0].Values { Expect(t.storageClass.AllowedTopologies[0].MatchLabelExpressions[0].Values).To(ContainElement(v)) } - } } } @@ -415,16 +415,16 @@ func (t *TestDeployment) Logs() ([]byte, error) { } // waitForPersistentVolumeClaimDeleted waits for a PersistentVolumeClaim to be removed from the system until timeout occurs, whichever comes first. -func waitForPersistentVolumeClaimDeleted(c clientset.Interface, ns string, pvcName string, Poll, timeout time.Duration) error { +func waitForPersistentVolumeClaimDeleted(c clientset.Interface, ns string, pvcName string, poll, timeout time.Duration) error { framework.Logf("Waiting up to %v for PersistentVolumeClaim %s to be removed", timeout, pvcName) - for start := time.Now(); time.Since(start) < timeout; time.Sleep(Poll) { + for start := time.Now(); time.Since(start) < timeout; time.Sleep(poll) { _, err := c.CoreV1().PersistentVolumeClaims(ns).Get(context.TODO(), pvcName, metav1.GetOptions{}) if err != nil { if apierrs.IsNotFound(err) { framework.Logf("Claim %q in namespace %q doesn't exist in the system", pvcName, ns) return nil } - framework.Logf("Failed to get claim %q in namespace %q, retrying in %v. Error: %v", pvcName, ns, Poll, err) + framework.Logf("Failed to get claim %q in namespace %q, retrying in %v. Error: %v", pvcName, ns, poll, err) } } return fmt.Errorf("PersistentVolumeClaim %s is not removed from the system within %v", pvcName, timeout) @@ -482,7 +482,7 @@ func (t *TestPod) WaitForRunning() { } // Ideally this would be in "k8s.io/kubernetes/test/e2e/framework" -// Similar to framework.WaitForPodSuccessInNamespaceSlow +// similar to framework.WaitForPodSuccessInNamespaceSlow. var podFailedCondition = func(pod *v1.Pod) (bool, error) { switch pod.Status.Phase { case v1.PodFailed: @@ -541,7 +541,7 @@ func podLogs(client clientset.Interface, name, namespace string) ([]byte, error) return client.CoreV1().Pods(namespace).GetLogs(name, &v1.PodLogOptions{}).Do(context.TODO()).Raw() } -// Method to get cloudInstanceId from Node Labels +// Method to get cloudInstanceId from Node Labels. func GetInstanceMetadataFromNodeSpec(client clientset.Interface) (*powervscloud.Metadata, error) { nodes, err := client.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{}) if err != nil { diff --git a/tests/it/utils.go b/tests/it/utils.go index 56875aea0..249e64a27 100644 --- a/tests/it/utils.go +++ b/tests/it/utils.go @@ -24,16 +24,19 @@ import ( "time" "github.com/container-storage-interface/spec/lib/go/csi" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/resolver" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" + "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/driver" "sigs.k8s.io/ibm-powervs-block-csi-driver/pkg/util" "sigs.k8s.io/ibm-powervs-block-csi-driver/tests/remote" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" ) const ( @@ -48,7 +51,7 @@ var ( endpoint = fmt.Sprintf("tcp://localhost:%d", port) ) -// Before runs the driver and creates a CSI client +// Before runs the driver and creates a CSI client. var _ = BeforeSuite(func() { var err error @@ -77,7 +80,7 @@ var _ = BeforeSuite(func() { Expect(csiClient).NotTo(BeNil()) }) -// After stops the driver +// After stops the driver. var _ = AfterSuite(func() { if os.Getenv("TEST_REMOTE_NODE") == "1" { r.TeardownDriver() @@ -86,13 +89,13 @@ var _ = AfterSuite(func() { } }) -// CSIClient controller and node clients +// CSIClient controller and node clients. type CSIClient struct { ctrl csi.ControllerClient node csi.NodeClient } -// verifyRequiredEnvVars Verify that PowerVS details are set in env +// verifyRequiredEnvVars Verify that PowerVS details are set in env. func verifyRequiredEnvVars(runRemotely bool) { Expect(os.Getenv("IBMCLOUD_API_KEY")).NotTo(BeEmpty(), "Missing env var IBMCLOUD_API_KEY") Expect(os.Getenv("POWERVS_CLOUD_INSTANCE_ID")).NotTo(BeEmpty(), "Missing env var POWERVS_CLOUD_INSTANCE_ID") @@ -105,7 +108,7 @@ func verifyRequiredEnvVars(runRemotely bool) { } } -// newCSIClient creates as CSI client +// newCSIClient creates as CSI client. func newCSIClient() (*CSIClient, error) { resolver.SetDefaultScheme("passthrough") opts := []grpc.DialOption{ @@ -122,7 +125,7 @@ func newCSIClient() (*CSIClient, error) { conn, err = net.Dial(scheme, addr) if err != nil { klog.Warningf("Client failed to dial endpoint %v", endpoint) - return false, nil + return false, err } klog.Infof("Client succeeded to dial endpoint %v", endpoint) return true, nil diff --git a/tests/remote/pi_resources.go b/tests/remote/pi_resources.go index 3eaf19786..26634685f 100644 --- a/tests/remote/pi_resources.go +++ b/tests/remote/pi_resources.go @@ -25,6 +25,7 @@ import ( "github.com/IBM-Cloud/power-go-client/clients/instance" "github.com/IBM-Cloud/power-go-client/power/models" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" @@ -109,8 +110,8 @@ func (r *Remote) createImage(image string) error { return nil } -// If given network name is not found in the network list -// create a new public network with the same name and use it +// If given network name is not found in the network list, +// create a new public network with the same name and use it. func (r *Remote) createPublicNetwork(network string) (string, error) { nc := r.powervsClients.nc netFound := "" @@ -139,7 +140,7 @@ func (r *Remote) createPublicNetwork(network string) (string, error) { return netFound, nil } -// Generate a new SSH key pair and create a SSHKey using the pub key +// Generate a new SSH key pair and create a SSHKey using the pub key. func (r *Remote) createSSHKey() error { kc := r.powervsClients.kc sshKey := r.resName @@ -158,7 +159,7 @@ func (r *Remote) createSSHKey() error { return err } -// Create a pvm instance +// Create a pvm instance. func (r *Remote) createInstance(image, network string) (string, string, error) { ic := r.powervsClients.ic name := r.resName @@ -217,7 +218,7 @@ func (r *Remote) createInstance(image, network string) (string, string, error) { return insID, publicIP, err } -// Wait till VLAN is attached to the network +// Wait till VLAN is attached to the network. func waitForNetworkVLAN(netID string, nc *instance.IBMPINetworkClient) (*models.Network, error) { var network *models.Network err := wait.PollUntilContextTimeout(context.Background(), 15*time.Second, 5*time.Minute, true, func(context.Context) (bool, error) { @@ -238,7 +239,7 @@ func waitForNetworkVLAN(netID string, nc *instance.IBMPINetworkClient) (*models. return network, err } -// Wait for VM status is ACTIVE and VM Health status to be OK/Warning +// Wait for VM status is ACTIVE and VM Health status to be OK/Warning. func waitForInstanceHealth(insID string, ic *instance.IBMPIInstanceClient) (*models.PVMInstance, error) { var pvm *models.PVMInstance err := wait.PollUntilContextTimeout(context.Background(), 30*time.Second, 45*time.Minute, true, func(context.Context) (bool, error) { @@ -267,14 +268,14 @@ func waitForInstanceHealth(insID string, ic *instance.IBMPIInstanceClient) (*mod return pvm, err } -// Wait till SSH test is complete +// Wait till SSH test is complete. func waitForInstanceSSH(publicIP string) error { err := wait.PollUntilContextTimeout(context.Background(), 20*time.Second, 30*time.Minute, true, func(context.Context) (bool, error) { var err error outp, err := runRemoteCommand(publicIP, "hostname") klog.Infof("out: %s err: %v", outp, err) if err != nil { - return false, nil + return false, err } return true, nil }) @@ -286,7 +287,7 @@ func waitForInstanceSSH(publicIP string) error { return err } -// Delete the VM and SSH key +// Delete the VM and SSH key. func (r *Remote) destroyPVSResources() { if r.powervsClients.ic == nil || r.powervsClients.kc == nil { klog.Warning("failed to retrieve PowerVS instance and key clients, ignoring") diff --git a/tests/remote/setup.go b/tests/remote/setup.go index 698ff253c..0d6d4165a 100644 --- a/tests/remote/setup.go +++ b/tests/remote/setup.go @@ -25,6 +25,7 @@ import ( "time" "github.com/IBM-Cloud/power-go-client/clients/instance" + "k8s.io/klog/v2" ) @@ -87,7 +88,7 @@ func (r *Remote) SetupNewDriver(endpoint string) (err error) { return nil } -// Create binary and archive it +// Create binary and archive it. func (r *Remote) createDriverArchive() (err error) { tarDir, err := os.MkdirTemp("", "powervscsidriver") if err != nil { @@ -137,10 +138,10 @@ func setupBinariesLocally(tarDir string) error { return nil } -// Upload archive to instance and run binaries -// TODO: grep pid of driver and kill it later -// Also, use random port -// May be track the job id if given +// Upload archive to instance and run binaries. +// TODO: grep pid of driver and kill it later. +// TODO: use random port. +// May be track the job id if given. func (r *Remote) uploadAndRun(endpoint string) error { r.remoteDir = filepath.Join("/tmp", "powervs-csi-"+time.Now().Format(timestampFormat)) diff --git a/tests/remote/util.go b/tests/remote/util.go index 1eb960c07..648231f5b 100644 --- a/tests/remote/util.go +++ b/tests/remote/util.go @@ -24,6 +24,7 @@ import ( "github.com/IBM-Cloud/power-go-client/ibmpisession" "github.com/IBM/go-sdk-core/v5/core" + "k8s.io/klog/v2" ) From 18694e1a2594dc3628784789b2b3937ac28c7121 Mon Sep 17 00:00:00 2001 From: Anshuman Date: Mon, 13 Jan 2025 09:46:25 +0530 Subject: [PATCH 2/2] Review changes --- .golangci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 834baad20..3435a9e54 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -108,7 +108,7 @@ linters-settings: - whyNoLint - wrapperFunc unused: - go: "1.22" + go: "1.23" issues: exclude-files: - "zz_generated.*\\.go$" @@ -199,4 +199,4 @@ run: - tools - e2e allow-parallel-runners: true - go: "1.22" \ No newline at end of file + go: "1.23"