Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reclaimspace: fix schedule handling #604

Merged
merged 2 commits into from
Jul 2, 2024
Merged

reclaimspace: fix schedule handling #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6971dd9
reclaimspace: fix schedule handling
iPraveenParihar Jun 26, 2024
ef5c97d
reclaimspace: add unit test case for schedule handling
iPraveenParihar Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions controllers/csiaddons/persistentvolumeclaim_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,29 +278,39 @@ func (r *PersistentVolumeClaimReconciler) determineScheduleAndRequeue(
return "", err
}
schedule, scheduleFound = getScheduleFromAnnotation(logger, ns.Annotations)
if !scheduleFound {
return "", ErrScheduleNotFound
}

// If the schedule is found, check whether driver supports the
// space reclamation using annotation on namespace and registered driver
// capability for decision on requeue.

requeue, supportReclaimspace := r.checkDriverSupportReclaimsSpace(logger, ns.Annotations, driverName)
if supportReclaimspace {
// if driver supports space reclamation,
// return schedule from ns annotation.
return schedule, nil
if scheduleFound {
requeue, supportReclaimspace := r.checkDriverSupportReclaimsSpace(logger, ns.Annotations, driverName)
if supportReclaimspace {
// if driver supports space reclamation,
// return schedule from ns annotation.
return schedule, nil
}
if requeue {
// The request needs to be requeued for checking
// driver support again.
return "", ErrConnNotFoundRequeueNeeded
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about the same where storageclass is deleted after the PVC creation, we need to consider not found error as the scheduleNotFound so as not to fail the reconcile because it's possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well IMO, we should be returning the precise error so that the users are aware of the exact state of the resources and they get it resolved?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would have made sense it was a VR controller but this is a pvc controller and user just created the PVC object nothing else. can you please check it again and let me know if am missing something.

Copy link
Member Author

@iPraveenParihar iPraveenParihar Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would have made sense it was a VR controller but this is a pvc controller and user just created the PVC object nothing else. can you please check it again and let me know if am missing something.

@Madhu-1, Apologies, but I quite didn't understand this part.
I understand your concern about reconcile failures when StorageClass is not found. However, masking the SC notFound error with scheduleNotFound error will lead to confusion? Its users responsibility to take appropriate actions when SC notFound error is thrown?

Or did I miss your point?

Copy link
Member

@Madhu-1 Madhu-1 Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

take the case of what happens below

  • PVC is created
  • Storageclass is deleted
  • The reconciliation triggered for the PVC as we get the pvc and check for annotation, if the SC is not found we keep on reqeueing this PVC which never completes, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, okay. I was talking about the same scenario. In such cases, there are two ways to handle it:

  • Return a "StorageClass not found" error, and the reconciliation process continues to requeue the PVC. This allows the user to take appropriate action on the error.
  • Return a "ScheduleNotFound" error and exit the reconciliation process. This approach masks the "StorageClass not found" error, potentially causing confusion for the user.

If masking the error isn't a concern, we can agree on returning the ScheduleNotFound error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as both are independent objects once created, masking the error makes sense to me and avoids a lot of reconciles, @nixpanic @Rakshith-R WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we just log the sc not found info and move on.

}
if requeue {
// The request needs to be requeued for checking
// driver support again.
return "", ErrConnNotFoundRequeueNeeded

// For static provisioned PVs, StorageClassName is empty.
if len(*pvc.Spec.StorageClassName) == 0 {
logger.Info("StorageClassName is empty")
return "", ErrScheduleNotFound
}

// check for storageclass schedule annotation.
sc := &storagev1.StorageClass{}
err = r.Client.Get(ctx, types.NamespacedName{Name: *pvc.Spec.StorageClassName}, sc)
if err != nil {
if apierrors.IsNotFound(err) {
logger.Error(err, "StorageClass not found", "StorageClass", *pvc.Spec.StorageClassName)
return "", ErrScheduleNotFound
}

logger.Error(err, "Failed to get StorageClass", "StorageClass", *pvc.Spec.StorageClassName)
return "", err
}
Expand Down
117 changes: 117 additions & 0 deletions controllers/csiaddons/persistentvolumeclaim_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,14 @@ import (
"testing"

csiaddonsv1alpha1 "github.com/csi-addons/kubernetes-csi-addons/apis/csiaddons/v1alpha1"
"github.com/csi-addons/kubernetes-csi-addons/internal/connection"
"github.com/go-logr/logr"
"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/log"
)

Expand Down Expand Up @@ -248,3 +252,116 @@ func TestGetScheduleFromAnnotation(t *testing.T) {
})
}
}

func TestDetermineScheduleAndRequeue(t *testing.T) {
type args struct {
pvcAnnotations map[string]string
nsAnnotations map[string]string
scAnnotations map[string]string
}
tests := []struct {
name string
args args
want string
}{
{
name: "pvc annotation set",
args: args{
pvcAnnotations: map[string]string{rsCronJobScheduleTimeAnnotation: "@daily"},
},
want: "@daily",
},
{
name: "sc annotation set",
args: args{
scAnnotations: map[string]string{rsCronJobScheduleTimeAnnotation: "@monthly"},
},
want: "@monthly",
},
{
name: "pvc & sc annotation set",
args: args{
pvcAnnotations: map[string]string{rsCronJobScheduleTimeAnnotation: "@daily"},
scAnnotations: map[string]string{rsCronJobScheduleTimeAnnotation: "@weekly"},
},
want: "@daily",
},
}

ctx := context.TODO()
logger := logr.Discard()
client := fake.NewClientBuilder().Build()
driverName := "test-driver"

ns := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: "test-namespace",
},
}
sc := &storagev1.StorageClass{
ObjectMeta: metav1.ObjectMeta{
Name: "test-sc",
},
}
pvc := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: "test-pvc",
Namespace: ns.Name,
},
Spec: corev1.PersistentVolumeClaimSpec{
StorageClassName: &sc.Name,
},
}

r := &PersistentVolumeClaimReconciler{
Client: client,
ConnPool: connection.NewConnectionPool(),
}

// Create the namespace, storage class, and PVC
err := r.Client.Create(ctx, ns)
assert.NoError(t, err)
err = r.Client.Create(ctx, sc)
assert.NoError(t, err)
err = r.Client.Create(ctx, pvc)
assert.NoError(t, err)

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
pvc.Annotations = tt.args.pvcAnnotations
ns.Annotations = tt.args.nsAnnotations
sc.Annotations = tt.args.scAnnotations

err = r.Client.Update(ctx, ns)
assert.NoError(t, err)
err = r.Client.Update(ctx, sc)
assert.NoError(t, err)
err = r.Client.Update(ctx, pvc)
assert.NoError(t, err)

schedule, error := r.determineScheduleAndRequeue(ctx, &logger, pvc, driverName)
assert.NoError(t, error)
assert.Equal(t, tt.want, schedule)
})
}

t.Run("empty StorageClassName for static pv", func(t *testing.T) {
emptyScName := ""
pvc.Spec.StorageClassName = &emptyScName
pvc.Annotations = nil
schedule, error := r.determineScheduleAndRequeue(ctx, &logger, pvc, driverName)
assert.ErrorIs(t, error, ErrScheduleNotFound)
assert.Equal(t, "", schedule)
})

// test for StorageClassName not found
t.Run("StorageClassName not found", func(t *testing.T) {
sc.Name = "non-existent-sc"
pvc.Spec.StorageClassName = &sc.Name
pvc.Annotations = nil
schedule, error := r.determineScheduleAndRequeue(ctx, &logger, pvc, driverName)
assert.ErrorIs(t, error, ErrScheduleNotFound)
assert.Equal(t, "", schedule)
})

}
Loading