diff --git a/controllers/persistent-volume-claim/controller.go b/controllers/persistent-volume-claim/controller.go index 7ad6207f4..9d3b4a209 100644 --- a/controllers/persistent-volume-claim/controller.go +++ b/controllers/persistent-volume-claim/controller.go @@ -26,17 +26,15 @@ const ( // PersistentVolumeClaimReconciler reconciles a PersistentVolumeClaim object type PersistentVolumeClaimReconciler struct { - client client.Client - apiReader client.Reader - recorder record.EventRecorder + Client client.Client + Recorder record.EventRecorder } // NewPersistentVolumeClaimReconciler returns PersistentVolumeClaimReconciler. -func NewPersistentVolumeClaimReconciler(client client.Client, apiReader client.Reader, eventRecorder record.EventRecorder) *PersistentVolumeClaimReconciler { +func NewPersistentVolumeClaimReconciler(client client.Client, eventRecorder record.EventRecorder) *PersistentVolumeClaimReconciler { return &PersistentVolumeClaimReconciler{ - client: client, - apiReader: apiReader, - recorder: eventRecorder, + Client: client, + Recorder: eventRecorder, } } @@ -46,10 +44,10 @@ func NewPersistentVolumeClaimReconciler(client client.Client, apiReader client.R // Reconcile PVC func (r *PersistentVolumeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - logger := log.Log.WithName("persistentvolume-controller").WithValues("Request.Name", req.Name, "Request.Namespace", req.Namespace) + logger := log.Log.WithName("pvc-controller").WithValues("Request.Name", req.Name, "Request.Namespace", req.Namespace) pvc := &corev1.PersistentVolumeClaim{} - err := r.client.Get(ctx, req.NamespacedName, pvc) + err := r.Client.Get(ctx, req.NamespacedName, pvc) switch { case err == nil: case apierrors.IsNotFound(err): @@ -59,7 +57,19 @@ func (r *PersistentVolumeClaimReconciler) Reconcile(ctx context.Context, req ctr } // Skip if the PVC is deleted or does not use the lvms storage class. - if pvc.DeletionTimestamp != nil || !strings.HasPrefix(*pvc.Spec.StorageClassName, controllers.StorageClassPrefix) { + if pvc.DeletionTimestamp != nil { + logger.Info("skipping pvc as it is about to be deleted (deletionTimestamp is set)") + return ctrl.Result{}, nil + } + + if pvc.Spec.StorageClassName == nil { + logger.Info("skipping pvc as the storageClassName is not set") + return ctrl.Result{}, nil + } + + if !strings.HasPrefix(*pvc.Spec.StorageClassName, controllers.StorageClassPrefix) { + logger.Info("skipping pvc as the storageClassName does not contain desired prefix", + "desired-prefix", controllers.StorageClassPrefix) return ctrl.Result{}, nil } @@ -70,7 +80,7 @@ func (r *PersistentVolumeClaimReconciler) Reconcile(ctx context.Context, req ctr // List the nodes nodeList := &corev1.NodeList{} - err = r.client.List(ctx, nodeList) + err = r.Client.List(ctx, nodeList) if err != nil { return ctrl.Result{}, err } @@ -101,7 +111,7 @@ func (r *PersistentVolumeClaimReconciler) Reconcile(ctx context.Context, req ctr // Publish an event if the requested storage is greater than the available capacity if !found { - r.recorder.Event(pvc, "Warning", "NotEnoughCapacity", + r.Recorder.Event(pvc, "Warning", "NotEnoughCapacity", fmt.Sprintf("Requested storage (%s) is greater than available capacity on any node (%s).", requestedStorage.String(), strings.Join(nodeMessage, ","))) logger.Info("Event published for the PVC", "PVC", req.NamespacedName) } diff --git a/controllers/persistent-volume-claim/controller_test.go b/controllers/persistent-volume-claim/controller_test.go new file mode 100644 index 000000000..f75968418 --- /dev/null +++ b/controllers/persistent-volume-claim/controller_test.go @@ -0,0 +1,143 @@ +package persistent_volume_claim_test + +import ( + "context" + "reflect" + "strings" + "testing" + "time" + + "github.com/openshift/lvm-operator/controllers" + persistentvolumeclaim "github.com/openshift/lvm-operator/controllers/persistent-volume-claim" + + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" + "k8s.io/utils/pointer" + controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestPersistentVolumeClaimReconciler_Reconcile(t *testing.T) { + defaultNamespace := "openshift-storage" + + tests := []struct { + name string + req controllerruntime.Request + objs []client.Object + wantErr bool + expectNoStorageAvailable bool + }{ + { + name: "testing set deletionTimestamp", + req: controllerruntime.Request{NamespacedName: types.NamespacedName{ + Namespace: defaultNamespace, + Name: "test-deletionTimestamp", + }}, + objs: []client.Object{ + &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNamespace, Name: "test-deletionTimestamp", + DeletionTimestamp: &metav1.Time{Time: time.Now()}}, + }, + }, + }, + { + name: "testing empty storageClassName", + req: controllerruntime.Request{NamespacedName: types.NamespacedName{ + Namespace: defaultNamespace, + Name: "test-emptyStorageClassName", + }}, + objs: []client.Object{ + &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNamespace, Name: "test-emptyStorageClassName"}, + }, + }, + }, + { + name: "testing non-applicable storageClassName", + req: controllerruntime.Request{NamespacedName: types.NamespacedName{ + Namespace: defaultNamespace, + Name: "test-nonApplicableStorageClassName", + }}, + objs: []client.Object{ + &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNamespace, Name: "test-nonApplicableStorageClassName"}, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: pointer.String("blabla"), + }, + }, + }, + }, + { + name: "testing non-pending PVC is skipped", + req: controllerruntime.Request{NamespacedName: types.NamespacedName{ + Namespace: defaultNamespace, + Name: "test-non-pending-PVC", + }}, + objs: []client.Object{ + &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNamespace, Name: "test-non-pending-PVC"}, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: pointer.String(controllers.StorageClassPrefix + "bla"), + }, + Status: v1.PersistentVolumeClaimStatus{ + Phase: v1.ClaimBound, + }, + }, + }, + }, + { + name: "testing pending PVC is proccessed", + req: controllerruntime.Request{NamespacedName: types.NamespacedName{ + Namespace: defaultNamespace, + Name: "test-pending-PVC", + }}, + objs: []client.Object{ + &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNamespace, Name: "test-pending-PVC"}, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: pointer.String(controllers.StorageClassPrefix + "bla"), + }, + Status: v1.PersistentVolumeClaimStatus{ + Phase: v1.ClaimPending, + }, + }, + }, + expectNoStorageAvailable: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + recorder := record.NewFakeRecorder(1) + r := &persistentvolumeclaim.PersistentVolumeClaimReconciler{ + Client: fake.NewClientBuilder().WithObjects(tt.objs...).Build(), + Recorder: recorder, + } + got, err := r.Reconcile(context.Background(), tt.req) + if (err != nil) != tt.wantErr { + t.Errorf("Reconcile() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, controllerruntime.Result{}) { + t.Errorf("Reconcile() got non default Result") + return + } + + if tt.expectNoStorageAvailable { + select { + case event := <-recorder.Events: + if !strings.Contains(event, "NotEnoughCapacity") { + t.Errorf("event was captured but it did not contain the reason NotEnoughCapacity") + return + } + case <-time.After(100 * time.Millisecond): + t.Errorf("wanted event that no storage is available but none was sent") + return + } + + } + }) + } +} diff --git a/controllers/persistent-volume/controller.go b/controllers/persistent-volume/controller.go index ce77ee98b..a38033d14 100644 --- a/controllers/persistent-volume/controller.go +++ b/controllers/persistent-volume/controller.go @@ -37,7 +37,7 @@ func NewPersistentVolumeReconciler(client client.Client, apiReader client.Reader // Reconcile PV func (r *PersistentVolumeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - logger := log.Log.WithName("persistentvolume-controller").WithValues("Request.Name", req.Name, "Request.Namespace", req.Namespace) + logger := log.Log.WithName("pv-controller").WithValues("Request.Name", req.Name, "Request.Namespace", req.Namespace) pv := &corev1.PersistentVolume{} err := r.client.Get(ctx, req.NamespacedName, pv) diff --git a/go.mod b/go.mod index 034095750..613ce942f 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ require ( k8s.io/apimachinery v0.26.1 k8s.io/client-go v0.26.1 k8s.io/component-helpers v0.23.4 + k8s.io/utils v0.0.0-20221128185143-99ec85e7a448 sigs.k8s.io/controller-runtime v0.14.6 sigs.k8s.io/yaml v1.3.0 ) @@ -101,7 +102,6 @@ require ( k8s.io/component-base v0.26.1 // indirect k8s.io/klog/v2 v2.80.1 // indirect k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280 // indirect - k8s.io/utils v0.0.0-20221128185143-99ec85e7a448 // indirect sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect ) diff --git a/main.go b/main.go index 3f4621cb8..46f803ce3 100644 --- a/main.go +++ b/main.go @@ -110,13 +110,13 @@ func main() { os.Exit(1) } - pvController := persistent_volume.NewPersistentVolumeReconciler(mgr.GetClient(), mgr.GetAPIReader(), mgr.GetEventRecorderFor("lvms-persistentvolume-controller")) + pvController := persistent_volume.NewPersistentVolumeReconciler(mgr.GetClient(), mgr.GetAPIReader(), mgr.GetEventRecorderFor("lvms-pv-controller")) if err := pvController.SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "PersistentVolume") os.Exit(1) } - pvcController := persistent_volume_claim.NewPersistentVolumeClaimReconciler(mgr.GetClient(), mgr.GetAPIReader(), mgr.GetEventRecorderFor("lvms-persistentvolumeclaim-controller")) + pvcController := persistent_volume_claim.NewPersistentVolumeClaimReconciler(mgr.GetClient(), mgr.GetEventRecorderFor("lvms-pvc-controller")) if err := pvcController.SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "PersistentVolumeClaim") os.Exit(1)