🐛 Allow rancher cluster to be deleted from UI #87
Conversation
Danil-Grigorev
force-pushed
the
fix-enqueue-for-rancher-cluster
branch
from
0594e3c
to
b113c85
Compare
| if annotations == nil { | ||
| annotations = map[string]string{} | ||
| } | ||
| // If the Rancher Cluster was already imported, then annotate the CAPI cluster so that we don't auto-import again. |
There was a problem hiding this comment.
where is the check in the code from this comment, I seem to be missing it
There was a problem hiding this comment.
There was a problem hiding this comment.
can we log the message in that case, it was not obvious from first look
There was a problem hiding this comment.
Operator does not reconcile non-imported rancher clusters. Imported rancher cluster follows the <capi-cluster>-capi naming convention. Therefore it is not possible here that any of the reconcile code, including reconcile delete would be executed on non-imported rancher cluster, unless it follows given naming convention. So log message on the https://github.com/rancher-sandbox/rancher-turtles/pull/87/files#diff-df81a5620bfede50d1c70fd0ebb982e652d28721e15b825c52dddcf97ceda0deR311 and https://github.com/rancher-sandbox/rancher-turtles/pull/87/files#diff-df81a5620bfede50d1c70fd0ebb982e652d28721e15b825c52dddcf97ceda0deR314 are both describing what is happening with the cluster. Don’t see a need to make it more vivid than that.
There was a problem hiding this comment.
It will also be logged in the predicate itself. My concern is that as far as I know predicates might not always work as expected and we should structure our code like they don't exist, it's nice if they filter out resources for us but don't expect them to always do that. I wouldn't remove the code.
| if annotations == nil { | ||
| annotations = map[string]string{} | ||
| } | ||
| // If the Rancher Cluster was already imported, then annotate the CAPI cluster so that we don't auto-import again. |
There was a problem hiding this comment.
can we log the message in that case, it was not obvious from first look
|
How does this affect #85? |
| log := log.FromContext(ctx) | ||
| log.Info("Reconciling CAPI Cluster Delete") | ||
| log.Info("Reconciling rancher cluster deletion") |
There was a problem hiding this comment.
but we are reconciling CAPI cluster deletion
There was a problem hiding this comment.
Right now we have to act upon the fact that rancher cluster is getting deleted
| if annotations == nil { | ||
| annotations = map[string]string{} | ||
| } | ||
| // If the Rancher Cluster was already imported, then annotate the CAPI cluster so that we don't auto-import again. |
There was a problem hiding this comment.
It will also be logged in the predicate itself. My concern is that as far as I know predicates might not always work as expected and we should structure our code like they don't exist, it's nice if they filter out resources for us but don't expect them to always do that. I wouldn't remove the code.
|
@alexander-demicev Could you provide some details to support this claim? #87 (comment) Predicates are a feature which was present in controller-runtime for multiple years, and is a simple filtering system for incoming objects from client cache. If it was ever true (only due to assumption that API server may return not a fully valid object, this was fixed). It is used in numerous production installations. I don’t think this is a concern. |
|
@Danil-Grigorev It happened about 2 years ago when we worked on externally managed infrastructure for CAPI, I can't find this discussion. If not working predicate appears, let's fix it when it happens :) |
Danil-Grigorev
dismissed stale reviews from mjura and furkatgofurov7
via
29223c7
Danil-Grigorev
force-pushed
the
fix-enqueue-for-rancher-cluster
branch
from
b113c85
to
29223c7
Compare
…luster Add converter method for rancher/CAPI cluster names Signed-off-by: Danil Grigorev <danil.grigorev@suse.com>
…clsuter Signed-off-by: Danil Grigorev <danil.grigorev@suse.com>
Signed-off-by: Danil Grigorev <danil.grigorev@suse.com>
Danil-Grigorev
force-pushed
the
fix-enqueue-for-rancher-cluster
branch
from
29223c7
to
3a7b43f
Compare
Danil-Grigorev
requested review from
alexander-demicev,
mjura and
furkatgofurov7
|
Can we merge this? Capi cluster event causes rancher cluster to be re-imported as of right now. @furkatgofurov7 @alexander-demicev I don't understand the delay in reviews |
I did pass through it once already and not blocking it from my side as per comments? |
| @@ -173,7 +176,7 @@ func (r *CAPIImportReconciler) Reconcile(ctx context.Context, req ctrl.Request) | |||
| func (r *CAPIImportReconciler) reconcile(ctx context.Context, capiCluster *clusterv1.Cluster) (ctrl.Result, error) { | |||
| // fetch the rancher clusters | |||
| rancherClusterHandler := rancher.NewClusterHandler(ctx, r.Client) | |||
| rancherClusterName := rancherClusterNameFromCAPICluster(capiCluster.Name) | |||
| rancherClusterName := helpers.Name(capiCluster.Name).ToRancherName() | |||
There was a problem hiding this comment.
Not entirely convinced by the change here, its feels less obvious.
kind/bug
What this PR does / why we need it:
This addresses the problems described in #86.
Checklist: