refactored NFSPVC Logic #56

dvirgilad · 2024-05-20T12:51:05Z

With this pr, several files where separated and refactored in order to improve readability and general code quality.
Also removed usage of deprecated kustomize functionality.

mzeevi

Looks better but there are still stuff to do. Some generate notes:

Make sure every function has a comment above it with proper English.
Make sure variables and constants that should not be unexported are not exported.
If a constant is only used in a single package and a single file then it should be a part of this package and not part of different package.
Return errors where needed with fmt.Errorf and add context to the errors where it's suitable. Only add context when it makes things clearer to the reader of the logs.
The RetryOnConflict update is repeated a lot, perhaps in case be generalized and reused in a function.
Go over the functions and think whether you can minimize the scopes, especially in if-else parts.

mzeevi · 2024-05-21T07:14:35Z

internal/controller/finalizer/finalizer.go

+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+// RemoveFinalizer remove the dana finalizer from the nfspvc object.


mzeevi · 2024-05-21T07:16:12Z

internal/controller/finalizer/finalizer.go

+	return nil
+}
+
+// EnsureFinalizer ensures the nfspvc has the finalizer.


// EnsureFinalizer adds a finalizer to the NFSPVC object if such does not exist.

mzeevi · 2024-05-21T07:22:57Z

internal/controller/status/status.go

+	return nil
+}
+
+// UpdatePvcStatus Update the status of the pvc.


mzeevi · 2024-05-21T07:25:30Z

internal/controller/status/status.go

+		}
+		return updateErr
+	})
+	//Should be nil


What do you mean? I think it can be non-nil if there is an issue with the update, or the default retries number has been reached

I meant for this comment to be explanatory. If everything went ok, err = nil but since we return err, the reader might get confused. I will replace this with more indicative logic

mzeevi · 2024-05-21T07:25:54Z

internal/controller/status/status.go

 }

-// getPvcStatus return the Pvc's Phase.
+// getPvcStatus return the phase of the pvc


returns* Fix the present simple everywhere :)

Also make sure there's a . at the end of each comment.

mzeevi · 2024-05-21T07:52:44Z

internal/controller/storage/storage.go

 			} else if !pvDeleted && pvcDeleted { // if only the pvc was deleted successfully, the reconcile function will be triggered again for the pv cleanup.
 				// if the pv still exists, delete it.


The comment should not be inline

mzeevi · 2024-05-21T07:54:11Z

internal/controller/storage/storage.go

+			if getErr := k8sClient.Get(ctx, types.NamespacedName{Name: nfspvc.Name + "-" + nfspvc.Namespace + "-pv"}, &pv); getErr != nil {
+				return getErr
+			}


Move the naming concatenation to a variable outside this scope so it doesn't need to be recomputed on every retry

mzeevi · 2024-05-21T07:54:58Z

internal/controller/sync/sync.go

@@ -4,30 +4,26 @@ import (
 	"context"
 	"fmt"

+	statusutils "github.com/dana-team/nfspvc-operator/internal/controller/status"


Why not just status with no renaming?

mzeevi · 2024-05-21T07:55:09Z

internal/controller/sync/sync.go

@@ -4,30 +4,26 @@ import (
 	"context"
 	"fmt"

+	statusutils "github.com/dana-team/nfspvc-operator/internal/controller/status"
+
+	storageutils "github.com/dana-team/nfspvc-operator/internal/controller/storage"


Why not just storage with no renaming?

mzeevi · 2024-05-21T07:57:22Z

internal/controller/sync/sync.go

+// DeletePvcBindAnnotation Deletes the "bind" annotation from a pvc
+func DeletePvcBindAnnotation(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, k8sClient client.Client, pvc corev1.PersistentVolumeClaim) error {
+	bindStatus, ok := pvc.ObjectMeta.Annotations[utils.PvcBindStatusAnnotation]
+	if ok && bindStatus == "yes" {


Change the "yes" to a constant

mzeevi

Some more comments

mzeevi · 2024-05-21T17:31:23Z

api/v1alpha1/nfspvc_webhook.go

 	PVCAlreadyExists       = "a PVC of this name already exists in the namespace. Please rename your NFSPVC"
 	InvalidAccessModeError = "forbidden: only the following AccessModes are permitted"


Those don't need to be exported

mzeevi · 2024-05-21T17:38:36Z

internal/controller/finalizer/finalizer.go

+func RemoveFinalizer(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, log logr.Logger, k8sClient client.Client) error {
+	controllerutil.RemoveFinalizer(&nfspvc, utils.NfsPvcDeletionFinalizer)
+	if err := k8sClient.Update(ctx, &nfspvc); err != nil {
+		log.Error(err, "unable to remove the finalizer from the NfsPvc - "+nfspvc.Name)


The log message here is kind of redundant since I'd get the same information on the caller when it fails

mzeevi · 2024-05-21T17:38:43Z

internal/controller/finalizer/finalizer.go

+	if !controllerutil.ContainsFinalizer(&nfspvc, utils.NfsPvcDeletionFinalizer) {
+		controllerutil.AddFinalizer(&nfspvc, utils.NfsPvcDeletionFinalizer)
+		if err := k8sClient.Update(ctx, &nfspvc); err != nil {
+			log.Error(err, "unable to add the finalizer to the nfspvc - "+nfspvc.Name)


Same thing here

mzeevi · 2024-05-21T17:39:18Z

internal/controller/finalizer/finalizer.go

+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+// RemoveFinalizer removes the dana finalizer from the nfspvc object.


// RemoveFinalizer removes a finalizer from the nfspvc object.

mzeevi · 2024-05-21T17:42:53Z

internal/controller/nfspvc_controller.go

+		if resources.IsFailedCleanUp(err) {
 			// this means the error is of type *FailedCleanUpError.
 			logger.Info(fmt.Sprintf("failed to handle NfsPvc deletion: %s, so trying again in a few seconds", err.Error()))


Any reason we are not using errors.Is() instead of creating a new error struct of type FailedCleanUpError? Can't we just use errors.New or fmt.Errorf() to create a new error and check for it here? Mind you we'd have to unwrap the error, so return %w when needed

Not sure I understand this one, skipping for now.

mzeevi · 2024-05-21T18:17:20Z

internal/controller/storage/storage.go

+	nfsPvcDanaLabel = "nfspvc.dana.io/nfspvc-owner"
+)
+
+func PreparePVC(nfspvc danaiov1alpha1.NfsPvc, StorageClass string) corev1.PersistentVolumeClaim {


Missing comment

It doesn't actually create a new PVC, it just returns a PersistentVolumeClaim object as per given parameters

mzeevi · 2024-05-21T18:17:28Z

internal/controller/storage/storage.go

+	}
+}
+
+func PreparePV(nfspvc danaiov1alpha1.NfsPvc, StorageClass string, ReclaimPolicy string) corev1.PersistentVolume {


Missing comment

It doesn't actually create a new PV, it just returns a PersistentVolume object as per given parameters

mzeevi · 2024-05-21T18:19:09Z

internal/controller/storage/storage.go

+// UpdatePV updates a PV if the nfspvc is updated.
+func UpdatePV(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, k8sClient client.Client, pv corev1.PersistentVolume) error {


The function makes no conditional on whether the nfspvc is updated or not. This logic is on the caller side.

mzeevi · 2024-05-21T18:20:02Z

internal/controller/sync/sync.go

-			return err
-		}
-		return nil
+	if pvc.Status.Phase == corev1.ClaimLost { // if the pvc's phase is 'lost', so probably the associated pv was deleted


Don't use comments like this, always in a new line

omers fault

mzeevi · 2024-05-21T18:22:13Z

internal/controller/sync/sync.go

+// DeletePVCBindAnnotation deletes the "bind" annotation from a pvc.
+func DeletePVCBindAnnotation(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, k8sClient client.Client, pvc corev1.PersistentVolumeClaim) error {


Does this need to be exported?

mzeevi

A few more minor things

mzeevi · 2024-05-22T14:28:10Z

internal/controller/nfspvc_controller.go

@@ -18,13 +18,18 @@ package controller

 import (
 	"context"
+	goerrors "errors"


I'd rather you keep this package using the same name and rename the apimachinery errors package as apierrors

mzeevi · 2024-05-22T14:28:44Z

internal/controller/nfspvc_controller.go

+	"github.com/dana-team/nfspvc-operator/internal/controller/status"
+
+	"github.com/dana-team/nfspvc-operator/internal/controller/resources"
+
+	"github.com/dana-team/nfspvc-operator/internal/controller/sync"
+


Remove the unneeded spaces

mzeevi · 2024-05-22T14:34:48Z

internal/controller/resources/resources.go

+	if nfspvc.ObjectMeta.DeletionTimestamp != nil {
+		if controllerutil.ContainsFinalizer(&nfspvc, utils.NfsPvcDeletionFinalizer) {


This logic should sit on the caller in the Reconciler imo

mzeevi · 2024-05-22T14:35:16Z

internal/controller/resources/resources.go

+var FailedCleanupError = goerrors.New("failed nfspvc cleanup")
+
+// HandleDelete ensures the deletion of the nfspvc.
+func HandleDelete(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, k8sClient client.Client) (error, bool) {


error should always be the last returned parameter

mzeevi · 2024-05-22T14:35:48Z

internal/controller/resources/resources.go

+				return err, false
+			}
+			if pvDeleted && pvcDeleted {
+				return finalizer.Remove(ctx, nfspvc, k8sClient), true


Calling the finalizer.Remove should be done from the Reconciler code and not from here, I think it'd be clearer

mzeevi · 2024-05-22T14:51:44Z

internal/controller/storage/storage.go

@@ -0,0 +1,99 @@
+package storage


This entire file can move to the resources package I think, no need for its own package

mzeevi · 2024-05-22T14:52:44Z

internal/controller/sync/sync.go

+	"github.com/dana-team/nfspvc-operator/internal/controller/storage"
+	"github.com/dana-team/nfspvc-operator/internal/controller/utils"


This file can move to the resources package

mzeevi · 2024-05-22T14:53:10Z

internal/controller/sync/sync.go

+func deletePVCBindAnnotation(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, k8sClient client.Client, pvc corev1.PersistentVolumeClaim) error {
+	bindStatus, ok := pvc.ObjectMeta.Annotations[pvcBindStatusAnnotation]
+	if ok && bindStatus == desiredBindStatus {
+		// Use retry on conflict to update the PVC.


Remove comment

mzeevi · 2024-05-22T14:53:16Z

internal/controller/sync/sync.go

+			delete(pvc.ObjectMeta.Annotations, pvcBindStatusAnnotation)
+			updateErr := k8sClient.Update(ctx, &pvc)
+			if errors.IsConflict(updateErr) {
+				// Conflict occurred, let's re-fetch the latest version of PVC and retry the update.


Remove comment

mzeevi · 2024-05-22T14:54:36Z

internal/controller/utils/common.go

+	StorageClassEnv  = "STORAGE_CLASS"
+	ReclaimPolicyEnv = "RECLAIM_POLICY"


These can be moved to the resources package. We should only have things in common if they are used across packages (or they don't fit anywhere else)

The variables are verified in main so I will keep this here.

mzeevi

Looks a lot better, very little remaining

mzeevi · 2024-05-23T09:48:34Z

internal/controller/resources/resources.go

+				return false, err
+			}
+			pvName := nfspvc.Name + "-" + nfspvc.Namespace + "-pv"
+			return false, fmt.Errorf("the pv %s is not deleted yet, %w", pvName, FailedCleanupError)


For names, use %q instead of %s since it makes it easier to read. Also use : instead of ,:

fmt.Errorf("pv %q has not been deleted yet: %w", pvName, FailedCleanupError)

mzeevi · 2024-05-23T09:49:36Z

internal/controller/resources/resources.go

+	}
+	if !pvcDeleted {
+		if err := deleteResource(ctx, pvc, k8sClient); err != nil {
+			return fmt.Errorf("failed to delete pvc - %s: %s", nfsPvcName, err.Error())


return fmt.Errorf("failed to delete pvc %q: %v", nfsPvcName, err)

Use %w when you need to unwrap the error, and %v when you don't

mzeevi · 2024-05-23T09:50:14Z

internal/controller/resources/resources.go

+	}
+	if !pvDeleted {
+		if err := deleteResource(ctx, pv, k8sClient); err != nil {
+			return fmt.Errorf("failed to delete pv - %s: %s", pvName, err.Error())


Same here, fix the error messages everywhere

With this pr, several files where seperated and refactored in order to improve readability and general code quality Also removed usage of deprecated kustomize functionality

mzeevi

/lgtm
/approve

dana-prow-ci · 2024-05-23T10:57:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dvirgilad, mzeevi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mzeevi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dana-prow-ci bot requested review from Itay5 and mzeevi May 20, 2024 12:51

dvirgilad force-pushed the dvir-refactor branch from 4af619b to 99a4dab Compare May 20, 2024 12:57

mzeevi requested changes May 21, 2024

View reviewed changes

dvirgilad force-pushed the dvir-refactor branch 2 times, most recently from a9340fd to 66b41f4 Compare May 21, 2024 14:46

mzeevi requested changes May 21, 2024

View reviewed changes

dvirgilad force-pushed the dvir-refactor branch 2 times, most recently from 3b1215c to 733d930 Compare May 22, 2024 14:07

mzeevi requested changes May 22, 2024

View reviewed changes

dvirgilad force-pushed the dvir-refactor branch 2 times, most recently from a24d8eb to fc2d1b4 Compare May 23, 2024 06:53

mzeevi requested changes May 23, 2024

View reviewed changes

dvirgilad force-pushed the dvir-refactor branch 2 times, most recently from 19a9f27 to ff9893f Compare May 23, 2024 10:45

Refactor NFSPVC Logic

c59e520

With this pr, several files where seperated and refactored in order to improve readability and general code quality Also removed usage of deprecated kustomize functionality

dvirgilad force-pushed the dvir-refactor branch from ff9893f to c59e520 Compare May 23, 2024 10:48

mzeevi approved these changes May 23, 2024

View reviewed changes

dana-prow-ci bot assigned mzeevi May 23, 2024

dana-prow-ci bot added the lgtm label May 23, 2024

dana-prow-ci bot added the approved label May 23, 2024

dana-prow-ci bot merged commit 031b784 into main May 23, 2024
3 of 4 checks passed

dana-prow-ci bot deleted the dvir-refactor branch May 23, 2024 10:57

		} else if !pvDeleted && pvcDeleted { // if only the pvc was deleted successfully, the reconcile function will be triggered again for the pv cleanup.
		// if the pv still exists, delete it.

		PVCAlreadyExists = "a PVC of this name already exists in the namespace. Please rename your NFSPVC"
		InvalidAccessModeError = "forbidden: only the following AccessModes are permitted"

		// UpdatePV updates a PV if the nfspvc is updated.
		func UpdatePV(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, k8sClient client.Client, pv corev1.PersistentVolume) error {

		// DeletePVCBindAnnotation deletes the "bind" annotation from a pvc.
		func DeletePVCBindAnnotation(ctx context.Context, nfspvc danaiov1alpha1.NfsPvc, k8sClient client.Client, pvc corev1.PersistentVolumeClaim) error {

		if nfspvc.ObjectMeta.DeletionTimestamp != nil {
		if controllerutil.ContainsFinalizer(&nfspvc, utils.NfsPvcDeletionFinalizer) {

		"github.com/dana-team/nfspvc-operator/internal/controller/storage"
		"github.com/dana-team/nfspvc-operator/internal/controller/utils"

		StorageClassEnv = "STORAGE_CLASS"
		ReclaimPolicyEnv = "RECLAIM_POLICY"

refactored NFSPVC Logic #56

refactored NFSPVC Logic #56

Conversation

dvirgilad commented May 20, 2024

mzeevi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzeevi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzeevi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzeevi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzeevi left a comment

Choose a reason for hiding this comment

dana-prow-ci bot commented May 23, 2024