Add PVC syncing support #179
Conversation
Signed-off-by: galal-hussein <[email protected]>
galal-hussein
force-pushed
the
storage
branch
from
ed83dd8 to
f2ca5d7
Compare
|
|
||
| const ( | ||
| pvcController = "pvc-syncer-controller" | ||
| pvcFinalizerName = "pv.k3k.io/finalizer" |
There was a problem hiding this comment.
Should this be pvc.k3k.io/finalizer?
| pvcFinalizerName = "pv.k3k.io/finalizer" | |
| pvcFinalizerName = "pvc.k3k.io/finalizer" |
| HostScheme *runtime.Scheme | ||
| logger *log.Logger | ||
| Translater translate.ToHostTranslater | ||
| //objs sets.Set[types.NamespacedName] |
There was a problem hiding this comment.
Can we drop the comment?
| //objs sets.Set[types.NamespacedName] |
| Complete(&reconciler) | ||
| } | ||
|
|
||
| func (v *PVCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { |
There was a problem hiding this comment.
This is probably a nit but I had some issues to understand what v was. Maybe an r for Reconciler` is clearer?
| func (v *PVCReconciler) pvc(obj *v1.PersistentVolumeClaim) *v1.PersistentVolumeClaim { | ||
| hostPVC := obj.DeepCopy() | ||
| v.Translater.TranslateTo(hostPVC) | ||
| // don't sync finalizers to the host |
There was a problem hiding this comment.
Does it mean we are still missing to remove the finalizers here?
| // deleting the synced service if exists | ||
| if err := v.hostClient.Delete(ctx, syncedPVC); err != nil { | ||
| return reconcile.Result{}, ctrlruntimeclient.IgnoreNotFound(err) | ||
| } |
There was a problem hiding this comment.
Hm, I think this could lead to some orphaned resource. I.e. what if the syncedPVC does not exist? This will error, and it will not be requed because of the IgnoreNotFound.
Maybe it's better to return only if the error is a different one. In case of ErrNotFound we should continue the deletion of the virtual PVC.
| if controllerutil.ContainsFinalizer(&virtPVC, pvcFinalizerName) { | ||
| controllerutil.RemoveFinalizer(&virtPVC, pvcFinalizerName) | ||
| if err := v.virtualClient.Update(ctx, &virtPVC); err != nil { | ||
| return reconcile.Result{}, err | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be shortened using only the RemoveFinalizer func. It does the same login of checking the finalizer, and it returns if the finalizer list was updated:
| if controllerutil.ContainsFinalizer(&virtPVC, pvcFinalizerName) { | |
| controllerutil.RemoveFinalizer(&virtPVC, pvcFinalizerName) | |
| if err := v.virtualClient.Update(ctx, &virtPVC); err != nil { | |
| return reconcile.Result{}, err | |
| } | |
| } | |
| if controllerutil.RemoveFinalizer(&virtPVC, pvcFinalizerName) { | |
| if err := v.virtualClient.Update(ctx, &virtPVC); err != nil { | |
| return reconcile.Result{}, err | |
| } | |
| } |
Probably the Contains is useful when you need to check the existence and do some logic before actually remove it.
| // Add finalizer if it does not exist | ||
| if !controllerutil.ContainsFinalizer(&virtPVC, pvcFinalizerName) { | ||
| controllerutil.AddFinalizer(&virtPVC, pvcFinalizerName) | ||
| if err := v.virtualClient.Update(ctx, &virtPVC); err != nil { | ||
| return reconcile.Result{}, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Same as before:
| // Add finalizer if it does not exist | |
| if !controllerutil.ContainsFinalizer(&virtPVC, pvcFinalizerName) { | |
| controllerutil.AddFinalizer(&virtPVC, pvcFinalizerName) | |
| if err := v.virtualClient.Update(ctx, &virtPVC); err != nil { | |
| return reconcile.Result{}, err | |
| } | |
| } | |
| // Add finalizer if it does not exist | |
| if controllerutil.AddFinalizer(&virtPVC, pvcFinalizerName) { | |
| if err := v.virtualClient.Update(ctx, &virtPVC); err != nil { | |
| return reconcile.Result{}, err | |
| } | |
| } |
| return reconcile.Result{}, err | ||
| } | ||
| } | ||
| // create or update the pv on host |
There was a problem hiding this comment.
| // create or update the pv on host | |
| // create or update the pvc on host |
| webhookName = "nodename.podmutator.k3k.io" | ||
| webhookTimeout = int32(10) | ||
| webhookPort = "9443" | ||
| webhookPath = "/mutate--v1-pod" |
There was a problem hiding this comment.
Is the -- a convention?
There was a problem hiding this comment.
Left some comments, it looks ok, I'm just not completely aware of the TLS/cert stuff, but just because I'm not an expert.
IIRC cert-manager is a required dependency, or maybe not yet, and it will be required for the CAPI provider? Could it simplify that part?
spec.nodeNameto the virtual-kubelet node name.