Support multiple label selection ability in EtcdNodeSelectorLabels #5321
Conversation
|
Hi, this is a bot comment, just put the label of /ok-to-test |
karmada-bot
added
the
kind/feature
karmada-bot
added
the
size/L
|
/ok-to-test |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5321 +/- ##
==========================================
+ Coverage 30.97% 31.68% +0.71%
==========================================
Files 637 643 +6
Lines 44266 44456 +190
==========================================
+ Hits 13712 14088 +376
+ Misses 29563 29337 -226
- Partials 991 1031 +40
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/retest |
1 similar comment
|
/retest |
| func mapsEqual(a, b map[string]string) bool { | ||
| if len(a) != len(b) { | ||
| return false | ||
| } | ||
| for k, v := range a { | ||
| if b[k] != v { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Just use reflect.DeepEqual(m1,m2), so this is not needed
There was a problem hiding this comment.
Add a description of what this PR does and why it is needed so that other reviewers can understand the PR.
/cc @zhzhuang-zju for help to review,thanks.
Probably because I made a mistake with the rabase operation, I might need to review the code again😭😭 |
|
/lgtm |
Description for review:Design PurposeIn certain scenarios, users may need to deploy Etcd on nodes with specific attributes, such as operating system type, hardware architecture, or custom labels. By allowing multiple labels to be specified, users can more accurately control the deployment of Etcd, thereby improving system robustness and performance. Implementation DetailsCommand Flag: Users can specify multiple labels using the --EtcdNodeSelectorLabels flag. The format for labels is key1=value1,key2=value2, with each label separated by a comma.
UsageUsers can specify the EtcdNodeSelectorLabels as follows when using the karmadactl init command: Testing and ValidationTo ensure the correctness of this feature, corresponding unit tests have been added, including:
|
|
This PR is related to the documentation PR: #5277 |
|
for reviewers, this PR is coming from #5277 (comment) |
|
@hulizhe you may interested in |
|
/assign @zhzhuang-zju |
| labels := strings.Split(i.EtcdNodeSelectorLabels, ",") | ||
| for _, label := range labels { | ||
| if !i.isNodeExist(label) { | ||
| return fmt.Errorf("no node found by label %s", label) | ||
| } |
There was a problem hiding this comment.
| labels := strings.Split(i.EtcdNodeSelectorLabels, ",") | |
| for _, label := range labels { | |
| if !i.isNodeExist(label) { | |
| return fmt.Errorf("no node found by label %s", label) | |
| } | |
| if !i.isNodeExist(i.EtcdNodeSelectorLabels) { | |
| return fmt.Errorf("no node found by label %s", i.EtcdNodeSelectorLabels) |
Here, we should verify if there are any nodes that match all the labels specified in a single string like "key1=value1,key2=value2", so we should not split the string into separate conditions.
|
karmada/pkg/karmadactl/cmdinit/cmdinit.go Line 143 in d4bfbb5 We can refine the description of the flag |
karmada-bot
added
size/M
|
/lgtm |
|
Hi @tiansuo114, can you help add a release note? |
What should I do? Is it related to modifications to the karmadactl documentation? If so, I think it can be included in the final supplementary documentation phase of my OSPP task |
|
Add the description into the Maybe: |
Okay, I put it in the pr body |
| if err != nil { | ||
| return fmt.Errorf("failed to convert etcdNodeSelector labels to map: %v", err) | ||
| } | ||
| i.EtcdNodeSelectorLabelsMap = labelMap |
There was a problem hiding this comment.
I have a question, and the variable setting logic is involved here, is it more reasonable to put it in the Complete function?
There was a problem hiding this comment.
I think this is make sense,I'm glad we're getting closer and closer to merge this PR.
| if i.EtcdStorageMode == "hostPath" && i.EtcdNodeSelectorLabelsMap != nil { | ||
| podSpec.NodeSelector = i.EtcdNodeSelectorLabelsMap | ||
| } | ||
| if i.EtcdStorageMode == "hostPath" && i.EtcdNodeSelectorLabels == "" { | ||
| podSpec.NodeSelector = map[string]string{"karmada.io/etcd": ""} |
There was a problem hiding this comment.
How about update like this:
if i.EtcdStorageMode == "hostPath" {
if i.EtcdNodeSelectorLabelsMap != nil {
podSpec.NodeSelector = i.EtcdNodeSelectorLabelsMap
} else {
podSpec.NodeSelector = map[string]string{"karmada.io/etcd": ""}
}
}There was a problem hiding this comment.
This is actually better, I fixed the above problem, now how does the code look
Signed-off-by: tiansuo114 <1729765480@qq.com> fix ci Signed-off-by: tiansuo114 <1729765480@qq.com> add unit test Signed-off-by: tiansuo114 <1729765480@qq.com> change unit test Signed-off-by: tiansuo114 <1729765480@qq.com> rm mapsEqual Signed-off-by: tiansuo114 <1729765480@qq.com> fix Signed-off-by: tiansuo114 <1729765480@qq.com> 1111111111111111111111 Signed-off-by: tiansuo114 <1729765480@qq.com> Update pkg/karmadactl/cmdinit/cmdinit.go Co-authored-by: zhzhuang-zju <m17799853869@163.com> use metav1.ParseToLabelSelector Signed-off-by: tiansuo114 <1729765480@qq.com> delete useless test Signed-off-by: tiansuo114 <1729765480@qq.com> 11 Signed-off-by: tiansuo114 <1729765480@qq.com> fix Signed-off-by: tiansuo114 <1729765480@qq.com>
|
kindly ping @XiShanYongYe-Chang |
|
Thanks~ |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: XiShanYongYe-Chang 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 |
karmada-bot
added
the
approved
|
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Supports the function of --etcd-node-selector-labels for multiple labels of nodes in the cluster
Which issue(s) this PR fixes:
Fixes #5320
This PR is related to the documentation PR: #5277
Special notes for your reviewer:
@liangyuanpeng
Does this PR introduce a user-facing change?: