🐛 Remove old ns when klusterlet ns is changed #442
Conversation
|
/hold |
|
/assign @elgnay |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #442 +/- ##
==========================================
+ Coverage 61.54% 62.57% +1.02%
==========================================
Files 133 135 +2
Lines 14078 11472 -2606
==========================================
- Hits 8665 7179 -1486
+ Misses 4664 3528 -1136
- Partials 749 765 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
qiujian16
force-pushed
the
ns-remove
branch
2 times, most recently
from
e68430f to
b0f2c8b
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
qiujian16
force-pushed
the
ns-remove
branch
2 times, most recently
from
4cdcb28 to
3a9ad95
Compare
Signed-off-by: Jian Qiu <jqiu@redhat.com>
...ator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_namespace_reconcile.go
Outdated
Show resolved
Hide resolved
| if ns.Name == config.KlusterletNamespace || ns.Name == config.KlusterletNamespace+"-addon" { | ||
| continue | ||
| } | ||
| if err := r.managedClusterClients.kubeClient.CoreV1().Namespaces().Delete( |
There was a problem hiding this comment.
will the old addon namespaces be deleted?
There was a problem hiding this comment.
but we should not delete old addon ns actually.
| namespace string, recorder events.Recorder) error { | ||
| if err := ensureAgentNamespace(ctx, kubeClient, namespace, recorder); err != nil { | ||
| if err := ensureAgentNamespace(ctx, kubeClient, namespace, map[string]string{ |
There was a problem hiding this comment.
I do not recommend calling the ensureAgentNamespace in ensureKlusterletNamespace func, since agent namespace is the namespace that klusterlet-agent runs in, it is on the manamegent cluster for hosted mode.
can we change the current ensureAgentNamespace to ensureNamespace, so here let the ensureKlusterletNamespace call the ensureNamespace func, and we add another ensureAgentNamespace which wraps the ensureNamespace.
- managed_reconcile -> ensureKlusterletNamespace -> ensureNamespace
- management_reconcile -> ensureAgentNamespace -> ensureNamespace
and after the separation, not sure if we only need to pass the labels for ensureKlusterletNamespace based on the current implemation?
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go
Outdated
Show resolved
Hide resolved
| return err | ||
| }, t.EventuallyTimeout*5, t.EventuallyInterval*5).Should(Succeed()) | ||
|
|
||
| By("old namespace should be removed") |
There was a problem hiding this comment.
A little confusing here, in the klusterlet_namespace_reconcile, we check only after the new klusterlet status is available, we delete the old ns. but here the old ns is removed, while the new CSR is still not approved?
There was a problem hiding this comment.
we check if it is applied
| ctx context.Context, | ||
| klusterlet *operatorapiv1.Klusterlet, | ||
| config klusterletConfig) (*operatorapiv1.Klusterlet, reconcileState, error) { | ||
| if !meta.IsStatusConditionTrue(klusterlet.Status.Conditions, operatorapiv1.ConditionKlusterletApplied) { |
There was a problem hiding this comment.
Should we also check the observed generation in the klusterlet status?
Signed-off-by: Jian Qiu <jqiu@redhat.com>
|
LGTM |
|
/lgtm |
|
/unhold |
openshift-merge-bot
bot
merged commit 5fc1dbd
into
open-cluster-management-io:main
Summary
Related issue(s)
Fixes #