check whether pods should be managed when ns is managed #958
Conversation
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Codecov ReportAttention: Patch coverage is
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -209,12 +209,12 @@ func (kmc *KmeshManageController) handleNamespaceUpdateFunc(oldObj, newObj inter | |||
| // Compare labels to check if they have actually changed | |||
| if !utils.ShouldEnroll(nil, oldNS) && utils.ShouldEnroll(nil, newNS) { | |||
| log.Infof("Enabling Kmesh for all pods in namespace: %s", newNS.Name) | |||
| kmc.enableKmeshForPodsInNamespace(newNS.Name) | |||
| kmc.enableKmeshForPodsInNamespace(newNS) | |||
There was a problem hiding this comment.
When there is no manage label on the ns and there is a manage label on the pod, this pod will be managed by Kmesh. So when there is no manage label on the pod but there is a manag label on the ns, will the pod be managed in this case?
Would this be a scenario where something goes wrong and should we report an error?
There was a problem hiding this comment.
If the ns is managed by Kmesh, the pods with no manage label or with label of "istio.io/dataplane-mode: Kmesh" should be managed. But if the label of pods is "istio.io/dataplane-mode: none", they should not be managed.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LiZhenCheng9527 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 |
| return | ||
| } | ||
|
|
||
| for _, pod := range pods { | ||
| kmc.enableKmeshManage(pod) | ||
| if utils.ShouldEnroll(pod, namespace) { |
There was a problem hiding this comment.
It is redundant, namespace already enabled
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #956
Special notes for your reviewer:
Does this PR introduce a user-facing change?: