Merge pull request #674 from kishen-v/cleanup

Refactor small sections of code and tests

Makefile

@@ -129,3 +129,6 @@ init-buildx:
 
 test-integration:
 	go test -v -timeout 100m sigs.k8s.io/ibm-powervs-block-csi-driver/tests/it -run ^TestIntegration$
+
+test-e2e:
+	go test -v -timeout 100m sigs.k8s.io/ibm-powervs-block-csi-driver/tests/e2e -run ^TestE2E$

README.md

@@ -76,7 +76,7 @@ Please see the compatibility matrix above before you deploy the driver
 
 To deploy the CSI driver:
 ```sh
-kubectl apply -k "https://github.com/kubernetes-sigs/ibm-powervs-block-csi-driver/deploy/kubernetes/overlays/stable/?ref=v0.3.0"
+kubectl apply -k "https://github.com/kubernetes-sigs/ibm-powervs-block-csi-driver/deploy/kubernetes/overlays/stable/?ref=v0.6.0"
 ```
 
 Verify driver is running:
@@ -87,7 +87,7 @@ kubectl get pods -n kube-system
 #### Deploy driver with debug mode
 To view driver debug logs, run the CSI driver with `-v=5` command line option
 
-To enable powervs debug logs, run the CSI driver with `debug=true` command line option.
+To enable PowerVS debug logs, run the CSI driver with `-debug=true` command line option.
 
 ## Examples
 Make sure you follow the [Prerequisites](README.md#Prerequisites) before the examples:

adhoc-controllers/controllers/nodeupdate_controller.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -63,7 +62,7 @@ func (r *NodeUpdateReconciler) Reconcile(_ context.Context, req ctrl.Request) (c
 		klog.Infof("PROVIDER-ID: %s", node.Spec.ProviderID)
 		metadata, err := cloud.TokenizeProviderID(node.Spec.ProviderID)
 		if err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to tokenize the providerID and err: %v", err)
+			return ctrl.Result{}, fmt.Errorf("failed to tokenize the providerID. err: %v", err)
 		}
 
 		nodeUpdateScope, err := cloud.NewNodeUpdateScope(cloud.NodeUpdateScopeParams{
@@ -73,12 +72,12 @@ func (r *NodeUpdateReconciler) Reconcile(_ context.Context, req ctrl.Request) (c
 		})
 
 		if err != nil {
-			return ctrl.Result{}, errors.Errorf("failed to create nodeUpdateScope: %+v", err)
+			return ctrl.Result{}, fmt.Errorf("failed to create nodeUpdateScope: %w", err)
 		}
 
 		instance, err := nodeUpdateScope.Cloud.GetPVMInstanceDetails(nodeUpdateScope.InstanceId)
 		if err != nil {
-			klog.Infof("Unable to fetch Instance Details %v", err)
+			klog.Errorf("unable to fetch instance details. err: %v", err)
 			return ctrl.Result{}, nil
 		}
 
@@ -92,8 +91,8 @@ func (r *NodeUpdateReconciler) Reconcile(_ context.Context, req ctrl.Request) (c
 					} else {
 						err := r.getOrUpdate(nodeUpdateScope)
 						if err != nil {
-							klog.Infof("unable to update instance StoragePoolAffinity %v", err)
-							return ctrl.Result{}, errors.Wrapf(err, "failed to reconcile VSI for IBMPowerVSMachine %s/%s", node.Namespace, node.Name)
+							klog.Errorf("unable to update instance StoragePoolAffinity. err: %v", err)
+							return ctrl.Result{}, fmt.Errorf("failed to reconcile VSI for IBMPowerVSMachine %s/%s. err: %w", node.Namespace, node.Name, err)
 						}
 					}
 				default:
@@ -107,10 +106,7 @@ func (r *NodeUpdateReconciler) Reconcile(_ context.Context, req ctrl.Request) (c
 }
 
 func (r *NodeUpdateReconciler) getOrUpdate(scope *cloud.NodeUpdateScope) error {
-	if err := scope.Cloud.UpdateStoragePoolAffinity(scope.InstanceId); err != nil {
-		return err
-	}
-	return nil
+	return scope.Cloud.UpdateStoragePoolAffinity(scope.InstanceId)
 }
 
 // SetupWithManager sets up the controller with the Manager.

adhoc-controllers/main.go

@@ -49,10 +49,11 @@ func init() {
 }
 
 func main() {
-	var metricsAddr string
-	var enableLeaderElection bool
-	var probeAddr string
-	var webhookPort int
+	var (
+		metricsAddr, probeAddr string
+		enableLeaderElection   bool
+		webhookPort            int
+	)
 
 	flag.StringVar(&metricsAddr, "metrics-bind-address", ":8081", "The address the metric endpoint binds to.")
 	flag.StringVar(&probeAddr, "health-probe-bind-address", ":8082", "The address the probe endpoint binds to.")

cmd/main.go

@@ -39,9 +39,9 @@ func main() {
 		driver.WithCloudConfig(options.ServerOptions.Cloudconfig),
 	)
 	if err != nil {
-		klog.Fatalln(err)
+		klog.Fatalf("unable to create CSI driver. err: %v", err)
 	}
 	if err := drv.Run(); err != nil {
-		klog.Fatalln(err)
+		klog.Fatalf("failed to run the CSI driver. err: %v", err)
 	}
 }

cmd/options.go

@@ -89,15 +89,15 @@ func GetOptions(fs *flag.FlagSet) *Options {
 	}
 
 	if err := fs.Parse(args); err != nil {
-		klog.Fatal(err)
+		klog.Fatalf("Error while parsing flag options. err: %v", err)
 	}
 
 	if *version {
 		info, err := driver.GetVersionJSON()
 		if err != nil {
-			klog.Fatalln(err)
+			klog.Fatalf("error while retriving the CSI driver version. err: %v", err)
 		}
-		klog.Infoln(info)
+		klog.Info(info)
 		osExit(0)
 	}
 

go.mod

@@ -12,7 +12,6 @@ require (
 	github.com/kubernetes-csi/csi-test v2.2.0+incompatible
 	github.com/onsi/ginkgo/v2 v2.19.0
 	github.com/onsi/gomega v1.33.1
-	github.com/pkg/errors v0.9.1
 	go.uber.org/mock v0.4.0
 	golang.org/x/sys v0.22.0
 	google.golang.org/grpc v1.65.0
@@ -95,6 +94,7 @@ require (
 	github.com/opencontainers/go-digest v1.0.0 // indirect
 	github.com/opencontainers/selinux v1.11.0 // indirect
 	github.com/opentracing/opentracing-go v1.2.0 // indirect
+	github.com/pkg/errors v0.9.1 // indirect
 	github.com/prometheus/client_golang v1.17.0 // indirect
 	github.com/prometheus/client_model v0.6.1 // indirect
 	github.com/prometheus/common v0.44.0 // indirect

pkg/cloud/cloud_interface.go

@@ -22,7 +22,7 @@ import (
 
 type Cloud interface {
 	CreateDisk(volumeName string, diskOptions *DiskOptions) (disk *Disk, err error)
-	DeleteDisk(volumeID string) (success bool, err error)
+	DeleteDisk(volumeID string) (err error)
 	AttachDisk(volumeID string, nodeID string) (err error)
 	DetachDisk(volumeID string, nodeID string) (err error)
 	ResizeDisk(volumeID string, reqSize int64) (newSize int64, err error)
@@ -33,5 +33,5 @@ type Cloud interface {
 	GetPVMInstanceByID(instanceID string) (instance *PVMInstance, err error)
 	GetPVMInstanceDetails(instanceID string) (*models.PVMInstance, error)
 	UpdateStoragePoolAffinity(instanceID string) error
-	IsAttached(volumeID string, nodeID string) (attached bool, err error)
+	IsAttached(volumeID string, nodeID string) (err error)
 }

pkg/cloud/metadata.go

@@ -87,10 +87,10 @@ func TokenizeProviderID(providerID string) (*Metadata, error) {
 
 // Get New Metadata Service
 func NewMetadataService(k8sAPIClient KubernetesAPIClient, kubeconfig string) (MetadataService, error) {
-	klog.Infof("retrieving instance data from kubernetes API")
+	klog.Info("Retrieving instance data from Kubernetes API")
 	clientset, err := k8sAPIClient(kubeconfig)
 	if err != nil {
-		klog.Warningf("error creating kubernetes api client: %v", err)
+		klog.Errorf("error creating Kubernetes API client: %v", err)
 		return nil, fmt.Errorf("an error occured during creation of k8s API client: %w", err)
 	}
 	klog.Info("kubernetes API is available")

pkg/cloud/powervs.go

@@ -159,38 +159,28 @@ func (p *powerVSCloud) CreateDisk(volumeName string, diskOptions *DiskOptions) (
 	return &Disk{CapacityGiB: capacityGiB, VolumeID: *v.VolumeID, DiskType: v.DiskType, WWN: strings.ToLower(v.Wwn)}, nil
 }
 
-func (p *powerVSCloud) DeleteDisk(volumeID string) (success bool, err error) {
-	err = p.volClient.DeleteVolume(volumeID)
-	if err != nil {
-		return false, err
-	}
-
-	return true, nil
+func (p *powerVSCloud) DeleteDisk(volumeID string) (err error) {
+	return p.volClient.DeleteVolume(volumeID)
 }
 
 func (p *powerVSCloud) AttachDisk(volumeID string, nodeID string) (err error) {
-	err = p.volClient.Attach(nodeID, volumeID)
-	if err != nil {
+	if err = p.volClient.Attach(nodeID, volumeID); err != nil {
 		return err
 	}
 	return p.WaitForVolumeState(volumeID, VolumeInUseState)
-
 }
 
 func (p *powerVSCloud) DetachDisk(volumeID string, nodeID string) (err error) {
-	err = p.volClient.Detach(nodeID, volumeID)
-	if err != nil {
+	if err = p.volClient.Detach(nodeID, volumeID); err != nil {
 		return err
 	}
 	return p.WaitForVolumeState(volumeID, VolumeAvailableState)
 }
 
-func (p *powerVSCloud) IsAttached(volumeID string, nodeID string) (attached bool, err error) {
+// IsAttached returns an error if a volume isn't attached to a node, else nil.
+func (p *powerVSCloud) IsAttached(volumeID string, nodeID string) (err error) {
 	_, err = p.volClient.CheckVolumeAttach(nodeID, volumeID)
-	if err != nil {
-		return false, err
-	}
-	return true, nil
+	return err
 }
 
 func (p *powerVSCloud) ResizeDisk(volumeID string, reqSize int64) (newSize int64, err error) {
@@ -214,18 +204,14 @@ func (p *powerVSCloud) ResizeDisk(volumeID string, reqSize int64) (newSize int64
 
 func (p *powerVSCloud) WaitForVolumeState(volumeID, state string) error {
 	ctx := context.Background()
-	err := wait.PollUntilContextTimeout(ctx, PollInterval, PollTimeout, true, func(ctx context.Context) (bool, error) {
+	return wait.PollUntilContextTimeout(ctx, PollInterval, PollTimeout, true, func(ctx context.Context) (bool, error) {
 		v, err := p.volClient.Get(volumeID)
 		if err != nil {
 			return false, err
 		}
 		spew.Dump(v)
 		return v.State == state, nil
 	})
-	if err != nil {
-		return err
-	}
-	return nil
 }
 
 func (p *powerVSCloud) GetDiskByName(name string) (disk *Disk, err error) {

pkg/cloud/powervs_node.go

@@ -25,9 +25,12 @@ import (
 )
 
 const (
-	PowerVSInstanceStateSHUTOFF = "SHUTOFF"
-	PowerVSInstanceStateACTIVE  = "ACTIVE"
-	StoragePoolAffinity         = false
+	PowerVSInstanceStateSHUTOFF  = "SHUTOFF"
+	PowerVSInstanceStateACTIVE   = "ACTIVE"
+	PowerVSInstanceStateERROR    = "ERROR"
+	PowerVSInstanceHealthWARNING = "WARNING"
+	PowerVSInstanceHealthOK      = "OK"
+	StoragePoolAffinity          = false
 )
 
 type NodeUpdateScopeParams struct {
@@ -78,7 +81,7 @@ func (p *powerVSCloud) GetPVMInstanceDetails(instanceID string) (*models.PVMInst
 func (p *powerVSCloud) UpdateStoragePoolAffinity(instanceID string) error {
 	_, err := p.pvmInstancesClient.Update(instanceID,
 		&models.PVMInstanceUpdate{
-			StoragePoolAffinity: ptr.To[bool](StoragePoolAffinity),
+			StoragePoolAffinity: ptr.To(StoragePoolAffinity),
 		})
 	return err
 }

pkg/driver/controller.go

@@ -230,7 +230,7 @@ func (d *controllerService) DeleteVolume(ctx context.Context, req *csi.DeleteVol
 		}
 	}
 
-	if _, err := d.cloud.DeleteDisk(volumeID); err != nil {
+	if err := d.cloud.DeleteDisk(volumeID); err != nil {
 		return nil, status.Errorf(codes.Internal, "Could not delete volume ID %q: %v", volumeID, err)
 	}
 
@@ -282,8 +282,7 @@ func (d *controllerService) ControllerPublishVolume(ctx context.Context, req *cs
 
 	pvInfo := map[string]string{WWNKey: disk.WWN}
 
-	attached, _ := d.cloud.IsAttached(volumeID, nodeID)
-	if attached {
+	if err = d.cloud.IsAttached(volumeID, nodeID); err == nil {
 		klog.V(5).Infof("ControllerPublishVolume: volume %s already attached to node %s, returning success", volumeID, nodeID)
 		return &csi.ControllerPublishVolumeResponse{PublishContext: pvInfo}, nil
 	}
@@ -324,7 +323,7 @@ func (d *controllerService) ControllerUnpublishVolume(ctx context.Context, req *
 		}
 	}
 
-	if attached, err := d.cloud.IsAttached(volumeID, nodeID); !attached {
+	if err := d.cloud.IsAttached(volumeID, nodeID); err != nil {
 		klog.V(4).Infof("ControllerUnpublishVolume: volume %s is not attached to %s, err: %v, returning with success", volumeID, nodeID, err)
 		return &csi.ControllerUnpublishVolumeResponse{}, nil
 	}

pkg/driver/controller_test.go

@@ -747,7 +747,7 @@ func TestDeleteVolume(t *testing.T) {
 				defer mockCtl.Finish()
 
 				mockCloud := mocks.NewMockCloud(mockCtl)
-				mockCloud.EXPECT().DeleteDisk(gomock.Eq(req.VolumeId)).Return(true, nil)
+				mockCloud.EXPECT().DeleteDisk(gomock.Eq(req.VolumeId)).Return(nil)
 				mockCloud.EXPECT().GetDiskByID(gomock.Eq(req.VolumeId)).Return(nil, nil)
 				powervsDriver := controllerService{
 					cloud:         mockCloud,
@@ -811,7 +811,7 @@ func TestDeleteVolume(t *testing.T) {
 				defer mockCtl.Finish()
 
 				mockCloud := mocks.NewMockCloud(mockCtl)
-				mockCloud.EXPECT().DeleteDisk(gomock.Eq(req.VolumeId)).Return(false, fmt.Errorf("DeleteDisk could not delete volume"))
+				mockCloud.EXPECT().DeleteDisk(gomock.Eq(req.VolumeId)).Return(fmt.Errorf("DeleteDisk could not delete volume"))
 				mockCloud.EXPECT().GetDiskByID(gomock.Eq(req.VolumeId)).Return(nil, nil)
 				powervsDriver := controllerService{
 					cloud:         mockCloud,
@@ -906,7 +906,7 @@ func TestControllerPublishVolume(t *testing.T) {
 				mockCloud := mocks.NewMockCloud(mockCtl)
 				mockCloud.EXPECT().GetPVMInstanceByID(gomock.Eq(expInstanceID)).Return(nil, nil)
 				mockCloud.EXPECT().GetDiskByID(gomock.Eq(volumeName)).Return(&cloud.Disk{WWN: expDevicePath}, nil)
-				mockCloud.EXPECT().IsAttached(gomock.Eq(volumeName), gomock.Eq(expInstanceID)).Return(false, nil)
+				mockCloud.EXPECT().IsAttached(gomock.Eq(volumeName), gomock.Eq(expInstanceID)).Return(fmt.Errorf("the disk is unattached"))
 				mockCloud.EXPECT().AttachDisk(gomock.Eq(volumeName), gomock.Eq(expInstanceID)).Return(nil)
 
 				powervsDriver := controllerService{
@@ -1199,7 +1199,7 @@ func TestControllerUnpublishVolume(t *testing.T) {
 
 				mockCloud := mocks.NewMockCloud(mockCtl)
 				mockCloud.EXPECT().GetDiskByID(gomock.Eq("vol-test")).Return(&cloud.Disk{WWN: expDevicePath}, nil)
-				mockCloud.EXPECT().IsAttached(gomock.Eq("vol-test"), gomock.Eq(expInstanceID)).Return(true, nil)
+				mockCloud.EXPECT().IsAttached(gomock.Eq("vol-test"), gomock.Eq(expInstanceID)).Return(nil)
 				mockCloud.EXPECT().DetachDisk(req.VolumeId, req.NodeId).Return(nil)
 
 				powervsDriver := controllerService{

pkg/driver/sanity_test.go

@@ -186,13 +186,13 @@ func (c *fakeCloudProvider) CreateDisk(volumeName string, diskOptions *cloud.Dis
 	return d.Disk, nil
 }
 
-func (c *fakeCloudProvider) DeleteDisk(volumeID string) (bool, error) {
+func (c *fakeCloudProvider) DeleteDisk(volumeID string) error {
 	for volName, f := range c.disks {
 		if f.Disk.VolumeID == volumeID {
 			delete(c.disks, volName)
 		}
 	}
-	return true, nil
+	return nil
 }
 
 func (c *fakeCloudProvider) AttachDisk(volumeID, nodeID string) error {
@@ -207,8 +207,8 @@ func (c *fakeCloudProvider) DetachDisk(volumeID, nodeID string) error {
 	return nil
 }
 
-func (c *fakeCloudProvider) IsAttached(volumeID string, nodeID string) (attached bool, err error) {
-	return true, nil
+func (c *fakeCloudProvider) IsAttached(volumeID string, nodeID string) (err error) {
+	return nil
 }
 
 func (c *fakeCloudProvider) WaitForVolumeState(volumeID, expectedState string) error {