Skip to content

Commit

Permalink
Merge pull request #369 from jakobmoellerdev/OCPBUGS-15576
Browse files Browse the repository at this point in the history
OCPBUGS-15576: fix: ensure panic safety in PVC controller for non set storageClassName
  • Loading branch information
openshift-merge-robot authored Jul 27, 2023
2 parents e0ccb5c + deac46e commit 255d754
Show file tree
Hide file tree
Showing 5 changed files with 169 additions and 16 deletions.
34 changes: 22 additions & 12 deletions controllers/persistent-volume-claim/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand All @@ -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):
Expand All @@ -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
}

Expand All @@ -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
}
Expand Down Expand Up @@ -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)
}
Expand Down
143 changes: 143 additions & 0 deletions controllers/persistent-volume-claim/controller_test.go
Original file line number Diff line number Diff line change
@@ -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
}

}
})
}
}
2 changes: 1 addition & 1 deletion controllers/persistent-volume/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down Expand Up @@ -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
)
4 changes: 2 additions & 2 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 255d754

Please sign in to comment.