controller: add PersistentVolumeClaimController #93

Rakshith-R · 2022-01-19T13:21:45Z

This controller watches for annotation on PVC,
if matches with reclaimspace.csiaddons.openshift.io/schedule
which can specify a schedule, a reclaimspace cron job cr
is created for that PVC witha generated name having pvc-name
as prefix.

Signed-off-by: Rakshith R [email protected]

[open to changing name of controller 😉 ]

Reconciles only if create or update happens to `reclaimspace.csiaddons.openshift.io/schedule: "@everyday"` annotation

steps: 
- delete all old child jobs
- check annotation for schedule
- if not found, clear name annotation if it exists and  exit
- if found, continue
- generate unique name = pvc.name+time.Now().Unix()  (ex rbd-pvc-1642598479)
- construct rscronjob
- set controllerref, create rscronjob
- exit

Rakshith-R · 2022-01-19T13:29:44Z

I'll add documentation for this by tomorrow.

nixpanic

looks quite reasonable. Missing update to the documentation under doc/reclaimspace.md

controllers/persistentvolumeclaim_controller.go

Madhu-1 · 2022-01-19T13:26:24Z

config/rbac/role.yaml

+  resources:
+  - persistentvolumeclaims
+  verbs:
+  - create


why create,delete and update permission is required?

Madhu-1 · 2022-01-19T13:26:40Z

config/rbac/role.yaml

+  - persistentvolumeclaims/finalizers
+  verbs:
+  - update
+- apiGroups:
+  - ""
+  resources:
+  - persistentvolumeclaims/status
+  verbs:
+  - get
+  - patch
+  - update


same here why this permission is required?

Madhu-1 · 2022-01-19T13:29:48Z

controllers/persistentvolumeclaim_controller.go

+
+// SetupWithManager sets up the controller with the Manager.
+func (r *PersistentVolumeClaimReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	if err := mgr.GetFieldIndexer().IndexField(context.Background(), &csiaddonsv1alpha1.ReclaimSpaceCronJob{}, jobOwnerKey, func(rawObj client.Object) []string {


split this into multiple lines for better reading

Madhu-1 · 2022-01-19T13:32:03Z

controllers/persistentvolumeclaim_controller.go

+	err = r.deleteChildCronJobs(ctx, &logger, &req)
+	if err != nil {
+		return ctrl.Result{}, err
+	}


one PVC will have only one Cron job why this is required?

one PVC will have only one Cron job why this is required?

yes, recreating Cron job each time simplifies the logic for a lot of scenarios.
This will also help in handling deletion when schedule is removed.

i still need to understand this one. what are childCronJobs referred here?

i still need to understand this one. what are childCronJobs referred here?

Just the cronjob created for this PVC previously.
Each reconcile will delete old cronjob and create new one with new name.
Using saved name led to lot of complexity.

Madhu-1 · 2022-01-19T13:34:26Z

controllers/persistentvolumeclaim_controller.go

+		_, nameFound := annotations[rsCronJobNameAnnotation]
+		if nameFound {
+			delete(annotations, rsCronJobNameAnnotation)
+			pvc.SetAnnotations(annotations)
+			err = r.Client.Update(ctx, pvc)
+			if err != nil {
+				logger.Error(err, "Failed to update annotation")
+
+				return ctrl.Result{}, err
+			}
+		}


can you please explain a bit more why this block is required?

can you please explain a bit more why this block is required?

when schedule annotation is removed, we are checking if name annotation exists and removing it (since we added it).

Madhu-1 · 2022-01-19T13:35:01Z

controllers/persistentvolumeclaim_controller.go

+	pvc.SetAnnotations(annotations)
+	err = r.Client.Update(ctx, pvc)
+	if err != nil {
+		logger.Error(err, "Failed to update annotation")
+
+		return ctrl.Result{}, err
+	}


set annotation only after creating the cronJob?

Madhu-1 · 2022-01-19T13:36:07Z

controllers/persistentvolumeclaim_controller_test.go

+	"testing"
+
+	csiaddonsv1alpha1 "github.com/csi-addons/kubernetes-csi-addons/api/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"


new line separation

Madhu-1 · 2022-01-19T13:36:47Z

doc changes are required with examples/usage.

Rakshith-R · 2022-01-20T07:54:00Z

updated permissions, using patch, added doc and refactored the code
PTAL.
Thanks!

humblec · 2022-01-20T14:02:45Z

if matches with reclaimspace.csiaddons.openshift.io/schedule
which can specify a schedule, a reclaimspace cron job cr
is created for that PVC witha generated name having pvc-name
as prefix.

What happens if the PVC get recreated with same name @Rakshith-R ?

Madhu-1 · 2022-01-20T13:59:45Z

controllers/persistentvolumeclaim_controller.go

+	err = r.deleteChildCronJobs(ctx, &logger, &req)
+	if err != nil {
+		return ctrl.Result{}, err
+	}


i still need to understand this one. what are childCronJobs referred here?

Madhu-1 · 2022-01-20T14:01:04Z

controllers/persistentvolumeclaim_controller.go

+	}
+
+	annotations := pvc.GetAnnotations()
+	schedule, scheduleFound := getScheduleFromAnnotation(&logger, annotations)


one more question. if the annotations are found but the cronjob CR is not created are we going to recreate it with same name or a new one?

one more question. if the annotations are found but the cronjob CR is not created are we going to recreate it with same name or a new one?

new name each time, using the same name causes pre-condition error since name/namespace remains same while uid changes for other controllers.

Madhu-1 · 2022-01-20T14:02:28Z

docs/reclaimspace.md

+to PersistentVolumeClaim object.
+
+```
+$ kubectl get pvc pvc-1


use meaningful name instead of pvc-1, pvc-sample and pvc-1 names are not matching

Madhu-1 · 2022-01-20T14:02:43Z

docs/reclaimspace.md

+$ kubectl annotate pvc pvc-sample "reclaimspace.csiaddons.openshift.io/schedule=@midnight"
+persistentvolumeclaim/pvc-sample annotated
+
+$ k get reclaimspacecronjobs.csiaddons.openshift.io 


k get to kubectl get

Madhu-1 · 2022-01-20T14:03:00Z

docs/reclaimspace.md

+$ kubectl annotate pvc pvc-sample "reclaimspace.csiaddons.openshift.io/schedule=*/1 * * * *" --overwrite=true
+persistentvolumeclaim/pvc-sample annotated
+
+$ k get reclaimspacecronjobs.csiaddons.openshift.io 


k get to kubectl get

humblec

any hiccups if the PVC get recreated with the same name?

Rakshith-R · 2022-01-20T14:06:49Z

any hiccups if the PVC get recreated with the same name?

Due to controllerOwnerRef, previous child cronjobs will get deleted

Pull request has been modified.

pkalever

I don't have a better naming suggestion at the moment but PersistentVolumeClaimController doesn't sound great.

pkalever · 2022-01-20T14:23:50Z

controllers/persistentvolumeclaim_controller.go

+	}
+
+	// create cronjob name
+	rsCronJobName := fmt.Sprintf("%s-%v", pvc.Name, time.Now().Unix())


This can be moved into a new function getCronJobName()?

This can be moved into a new function getCronJobName()?

done, names it generateCronJobName()

Well you forget to use it :-)

🤣 its used now,

pkalever · 2022-01-20T14:27:18Z

controllers/persistentvolumeclaim_controller.go

+	}
+
+	// add cronjob name in annotations.
+	patch := []byte(fmt.Sprintf(`{"metadata":{"annotations":{%q:%q}}}`, rsCronJobNameAnnotation, rsCronJobName))


may be name it annotations instead of patch?

may be name it annotations instead of patch?

annotations is already taken, just patch sounds fine to me

pkalever · 2022-01-20T14:28:06Z

controllers/persistentvolumeclaim_controller.go

+		return ctrl.Result{}, err
+	}
+
+	logger.Info("Successfully created reclaimSpaceCronJob")


new line before return?

pkalever · 2022-01-20T14:28:29Z

controllers/persistentvolumeclaim_controller.go

+			}
+			// reconcile only if schedule annotation is found.
+			_, ok := e.Object.GetAnnotations()[rsCronJobScheduleTimeAnnotation]
+			return ok


new line here too!

pkalever · 2022-01-20T14:28:38Z

controllers/persistentvolumeclaim_controller.go

+			// reconcile only if schedule annotation between old and new objects have changed.
+			oldSchdeule, oldOk := e.ObjectOld.GetAnnotations()[rsCronJobScheduleTimeAnnotation]
+			newSchdeule, newOk := e.ObjectNew.GetAnnotations()[rsCronJobScheduleTimeAnnotation]
+			return oldOk != newOk || oldSchdeule != newSchdeule


new line here too!

pkalever · 2022-01-20T14:32:51Z

controllers/persistentvolumeclaim_controller.go

+
+		return defaultSchedule, true
+	}
+	return schedule, true


pkalever · 2022-01-20T14:33:40Z

controllers/persistentvolumeclaim_controller.go

+	}
+	_, err := cron.ParseStandard(schedule)
+	if err != nil {
+		logger.Info(fmt.Sprintf("Parsing given schedule %q failed, using default schedule %q",


Sounds like logger.Warning?

Sounds like logger.Warning?

Should be fine for now, there is no default logger.Warning,
We may add it the future.

pkalever · 2022-01-20T14:38:05Z

controllers/persistentvolumeclaim_controller.go

+// Reconcile is part of the main kubernetes reconciliation loop which aims to
+// move the current state of the cluster closer to the desired state.
+// This is triggered when `reclaimspace.csiaddons.openshift/schedule` annotation
+// is found on newly created PVC or if there is a change in valude of the annotation.


s/valude/value

done @pkalever PTAL

Rakshith-R · 2022-01-20T15:27:44Z

I don't have a better naming suggestion at the moment but PersistentVolumeClaimController doesn't sound great.

😆 me too, check the pr description.

We can always come back and change it, this is completely internal.
I dont any reason to block this pr for that reason.
(needs to get merged by tomorrow)

pkalever

Well, I didn't block this PR for the naming part, but for the rest of the comments.

You have one more to take care of for now i.e use generateCronJobName

Rakshith-R · 2022-01-21T04:55:46Z

Well, I didn't block this PR for the naming part, but for the rest of the comments.

You have one more to take care of for now i.e use generateCronJobName

done 👍 thanks!

pkalever

Looks good to me, the only thing I don't like is the Controller name as mentioned before.

How about PVCAnnotationsController?

Rakshith-R · 2022-01-21T08:12:04Z

making some changes to update child cronjob instead of recreating them

Pull request has been modified.

Rakshith-R · 2022-01-21T09:11:00Z

Reconciles if create or update happens to `reclaimspace.csiaddons.openshift.io/schedule: "@everyday"` annotation
Now controller also reacts to events on the child cronjob.


steps: 
- get first child cron job, delete other child jobs(precautionary, multiple child cronjobs should not exist)
- check annotation for schedule
- if not found, delete child cron job and clear name annotation if it exists and  exit
- if found, continue
- check default spec and child cronjob spec, update if necessary and cron job is present on the cluster.
- generate unique name = pvc.name+time.Now().Unix()  (ex rbd-pvc-1642598479)
- set name annotaion
- construct rscronjob
- set controllerref, create rscronjob
- exit

PTAL @csi-addons/kubernetes-csi-addons-reviewers @csi-addons/kubernetes-csi-addons-contributors

Madhu-1 · 2022-01-21T09:21:57Z

controllers/persistentvolumeclaim_controller.go

+		logger.Error(err, "Failed to delete child reclaimSpaceCronJob",
+			"ReclaimSpaceCronJobName", job.Name)
+
+		return fmt.Errorf("Failed to delete child reclaimSpaceCronJob: %v", err)


use %w return childjob name also?

use %w return childjob name also?

done

Rakshith-R · 2022-01-21T10:58:02Z

@Mergifyio rebase

This controller watches for annotation on PVC, if matches with `reclaimspace.csiaddons.openshift.io/schedule` which can specify a schedule, a reclaimspace cron job cr is created for that PVC witha generated name having pvc-name as prefix. Signed-off-by: Rakshith R <[email protected]>

mergify · 2022-01-21T10:58:42Z

rebase

✅ Branch has been successfully rebased

pkalever

Looks good to me, Thanks!

nixpanic · 2022-01-21T12:21:16Z

controllers/persistentvolumeclaim_controller.go

+
+			return ok
+		},
+		UpdateFunc: func(e event.UpdateEvent) bool {


Does this not require the update permission for the PVC/annotations? patch is there, is it not cleaner to patch it in any case?

Watch perm is enough to check for events.

Ah, sure. Thanks!

Syncing latest changes from main for kubernetes-csi-addons

mergify bot requested review from nixpanic, yati1998 and Yuggupta27 January 19, 2022 13:22

Rakshith-R requested a review from Madhu-1 January 19, 2022 13:23

nixpanic requested changes Jan 19, 2022

View reviewed changes

controllers/persistentvolumeclaim_controller.go Outdated Show resolved Hide resolved

controllers/persistentvolumeclaim_controller.go Outdated Show resolved Hide resolved

controllers/persistentvolumeclaim_controller.go Outdated Show resolved Hide resolved

Madhu-1 reviewed Jan 19, 2022

View reviewed changes

Rakshith-R force-pushed the Annotation-controller branch from befbf09 to be8eced Compare January 20, 2022 07:48

Rakshith-R requested review from Madhu-1 and nixpanic January 20, 2022 07:54

nixpanic previously approved these changes Jan 20, 2022

View reviewed changes

Madhu-1 requested changes Jan 20, 2022

View reviewed changes

humblec suggested changes Jan 20, 2022

View reviewed changes

Rakshith-R force-pushed the Annotation-controller branch 2 times, most recently from 220258c to 3e71ef7 Compare January 20, 2022 14:11

Rakshith-R requested review from humblec, nixpanic and Madhu-1 January 20, 2022 14:12

pkalever suggested changes Jan 20, 2022

View reviewed changes

Rakshith-R force-pushed the Annotation-controller branch from 3e71ef7 to 2612f1c Compare January 20, 2022 15:23

pkalever suggested changes Jan 20, 2022

View reviewed changes

Rakshith-R force-pushed the Annotation-controller branch from 2612f1c to 50644f3 Compare January 21, 2022 04:54

Rakshith-R requested a review from a team January 21, 2022 04:56

pkalever approved these changes Jan 21, 2022

View reviewed changes

nixpanic previously approved these changes Jan 21, 2022

View reviewed changes

Rakshith-R marked this pull request as draft January 21, 2022 08:12

Rakshith-R force-pushed the Annotation-controller branch from 50644f3 to a86c379 Compare January 21, 2022 08:46

Rakshith-R force-pushed the Annotation-controller branch 2 times, most recently from 2ebeffb to d87ed84 Compare January 21, 2022 09:07

Rakshith-R marked this pull request as ready for review January 21, 2022 09:07

Rakshith-R force-pushed the Annotation-controller branch from d87ed84 to ca9ae69 Compare January 21, 2022 09:15

Rakshith-R requested a review from nixpanic January 21, 2022 09:16

Madhu-1 reviewed Jan 21, 2022

View reviewed changes

Rakshith-R force-pushed the Annotation-controller branch from ca9ae69 to 611422e Compare January 21, 2022 09:25

Madhu-1 approved these changes Jan 21, 2022

View reviewed changes

Rakshith-R force-pushed the Annotation-controller branch from 611422e to e344894 Compare January 21, 2022 10:58

pkalever approved these changes Jan 21, 2022

View reviewed changes

nixpanic reviewed Jan 21, 2022

View reviewed changes

Rakshith-R requested a review from nixpanic January 21, 2022 12:27

nixpanic approved these changes Jan 21, 2022

View reviewed changes

mergify bot merged commit 7835881 into csi-addons:main Jan 21, 2022

nixpanic pushed a commit to nixpanic/kubernetes-csi-addons that referenced this pull request Jan 12, 2024

Merge pull request csi-addons#93 from red-hat-storage/sync_us--main

c91e962

Syncing latest changes from main for kubernetes-csi-addons

controller: add PersistentVolumeClaimController #93

controller: add PersistentVolumeClaimController #93

Conversation

Rakshith-R commented Jan 19, 2022

[open to changing name of controller 😉 ]

Rakshith-R commented Jan 19, 2022

nixpanic 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

Madhu-1 commented Jan 19, 2022

Rakshith-R commented Jan 20, 2022 • edited Loading

humblec commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R Jan 20, 2022 • edited Loading

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

humblec left a comment

Choose a reason for hiding this comment

Rakshith-R commented Jan 20, 2022 • edited Loading

pkalever 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

Rakshith-R commented Jan 20, 2022

pkalever left a comment • edited Loading

Choose a reason for hiding this comment

Rakshith-R commented Jan 21, 2022

pkalever left a comment

Choose a reason for hiding this comment

Rakshith-R commented Jan 21, 2022

Rakshith-R commented Jan 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R commented Jan 21, 2022

mergify bot commented Jan 21, 2022

✅ Branch has been successfully rebased

pkalever 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

Rakshith-R commented Jan 20, 2022 •

edited

Loading

Rakshith-R Jan 20, 2022 •

edited

Loading

Rakshith-R commented Jan 20, 2022 •

edited

Loading

pkalever left a comment •

edited

Loading