csi: add csi driver and controller plugin deployment #13
Conversation
| r.Log.Error(err, "unable to retrieve csi controller deployment", "TopolvmController", cd.Name) | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The deployment should be deleted here.
There was a problem hiding this comment.
For a deployment, an ownerReference should be enough for cleanup.
Not for cluster scoped objects or complex cleanups though.
There was a problem hiding this comment.
What is the expected behaviour if the lvmcluster object is deleted? Shouldn't the deployment be deleted as well?
There was a problem hiding this comment.
- I understood that if lvm cr is deleted then controller will also be deleted due to owner ref
- However I'm unsure whether dependents will be deleted after realizing deletestamp on owner or owner needs to be deleted (like actual deletion ~Not found)🤔
There was a problem hiding this comment.
At what point will the deployment be deleted? As soon as the delete operation is performed or after the finalizer is removed from the LvmController? The operator waits for all resources to be deleted before removing the finalizer.
If it is deleted before the finalizer is removed, then this method should return an error as long as the deployment resource exists in order to prevent the premature removal of the finalizer. If not, this is not a good candidate for the resource manager.
There was a problem hiding this comment.
- I did some testing using configmaps with finalizers and ownerreferences and here's the gist in current context
- If controller deployment were to depend on lvmcluster CR, if I set non-blocking ownerref on deployment and you set finalizer on CR, when you remove the finalizer then only controller deployment will be deleted
- However the current reconciler only removes the finalizer when all the units get deleted, in this case I do not depend on lvm CR to be the owner and listen to reconciler to delete deployment whenever it's called
- I see two ways out of it
- You do not change the reconcile and send delete op to the resource units and I'll add deletion logic
- You can just remove the finalizer and send a delete op to the resource units and reconcile for nil (because some resources like csidriver is cluster scoped and doesn't respect ownerref)?
There was a problem hiding this comment.
Lets stick with the current approach for now - the ensureDeleted will effectively be a noop for topolvm-controller. The lvmcluster controller will remove the finalizer when all resource managers return nil . The LVMCluster object will then be removed followed the topolvm-controller.
There was a problem hiding this comment.
- uh oh, just saw this review after adding tests
- anyways will raise separate PRs
There was a problem hiding this comment.
- I'm continuing with a non-blocking owner reference and also implementing custom logic for controller deletion
- We can decide what exactly to be used as it's not of utmost importance as of now
| } | ||
|
|
||
| // TODO: Remove custom generation of TLS certs, find where it's being used in the first place in Topolvm Code | ||
| iContainers := []corev1.Container{*getInitContainer()} |
There was a problem hiding this comment.
We do not have any way to avoid the creation and use of self-signed certs. "cert manager" can provide certificates in a secret. if we verify the secret exists we can use these certificates, if the secret does not exist, then we can create self signed certificates.
There was a problem hiding this comment.
- ack, I wasn't able to confirm where does topolvm actually use these certs and I mean that in the comment.
- for now we are going with manual generation of certs using initContainer and replacing it with cert-manager is being investigated
|
|
||
| ssCertGenerator := &corev1.Container{ | ||
| Name: "self-signed-cert-generator", | ||
| Image: "alpine/openssl", |
There was a problem hiding this comment.
I think we need to make very easy to change this image. What about to use a constant?
There was a problem hiding this comment.
- for now we just need to generate and use those certs, so changing base image would have any effect in openssl?
|
Thanks all for your early reviews, I'll address them at the earliest 😀 |
|
Does the topolvm-controller need anything special wrt SCCs? |
Afair, normal rbac would do, I will add them (rbac) based on topolvm helm chart. |
after referring to topolvm controller rbac, I don't see the need for SCC, those will be need for vgmanager and nodeplugin. |
| @@ -95,7 +95,11 @@ func (r *LVMClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) | |||
|
|
|||
| // errors returned by this will be updated in the reconcileSucceeded condition of the LVMCluster | |||
| func (r *LVMClusterReconciler) reconcile(ctx context.Context, instance *lvmv1alpha1.LVMCluster) (ctrl.Result, error) { | |||
| resourceList := []resourceManager{} | |||
| // TODO: Resource deletion should be in reverse order of install | |||
| resourceList := []resourceManager{ | |||
There was a problem hiding this comment.
This will need to be split up amongst the 2 PRs (CSIDriver and topolvm-controller)
|
|
||
| // get the desired state of topolvm controller deployment | ||
| controllerDeployment := getControllerDeployment(lvmCluster) | ||
| result, err := cutil.CreateOrUpdate(ctx, r.Client, controllerDeployment, func() error { |
There was a problem hiding this comment.
Better to update only if there is a change in the spec.
There was a problem hiding this comment.
- Topolvm Controller deployment is resistant to LVM Cluster CR Updates
- After first creation the result will mostly be
Unchanged - Current implementation is used only to facilitate upgrade due to change in images, if both desired and existing state is same then it'll be
Unchanged
| // Let's make sure our Schedule string value was properly converted/handled. | ||
| Expect(lvmCluster1.Status.Ready).Should(Equal(true)) | ||
|
|
||
| By("Confirming deletion of lvm cluster") |
There was a problem hiding this comment.
Please move the deletion to a separate test.
There was a problem hiding this comment.
- this is all part of
It("Reconciles an LvmCluster, "if I were to move it to a separate test then I think I need to deploy again and then delete
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G [email protected]