fix: add a kusion-control label in svc labels #510
Conversation
|
lgtm |
| if len(ports) == 0 { | ||
| return ErrEmptyPorts | ||
| } | ||
|
|
There was a problem hiding this comment.
If you don't want to render k8s service, should check the length of ports before new ports generator. If empty, don't new the generator.
Deleting the validation of ports length is not correct, cause the k8s service will still try to get generated but failed.
There was a problem hiding this comment.
If the ports are nil, the logic below will return immediately. I don't understand why this generator will fail.
There was a problem hiding this comment.
1, I don't know what is "the logic below" you refer to. Besides, what will get returned? an error, or an empty result?
2, If not k8s svc will get generated, why you new the ports generator? It will generate nothing. Add a judgement in “service generator" is much more concise and correct.
There was a problem hiding this comment.
- The logic below this line. That means the logic in this function. This function will return nil if the ports are empty
- Yes, deliberately return before the validation function is more readable
| @@ -163,16 +163,13 @@ func (g *portsGenerator) generateK8sSvc(public bool, ports []network.Port) *v1.S | |||
|
|
|||
| // only support Aliyun SLB for now, and set SLB spec by default. | |||
| svc.Annotations[aliyunLBSpec] = aliyunSLBS1Small | |||
| svc.Labels[kusionControl] = "true" | |||
There was a problem hiding this comment.
why you need to add this label? K8s Service will Inherit the label that Collaset/Deployment have, does they don't have the label of kusionControl?
There was a problem hiding this comment.
This label is used to cooperate with the KAFE svc controller to implement gracefully rollout. The KAFE svc controller will only process svcs with this label and it is given in the svc sample before.
There was a problem hiding this comment.
I know this. But I want to know does this label is not attached to the KAFE workload, Collaset? Does it only need to attach to the service?
This label works only for KAFE system, should not get added under other situation.
There was a problem hiding this comment.
It only works with the KAFE workload. We will move this label and all Aliyun-related labels to platform.k in the next version
What type of PR is this?
/kind bug
What this PR does / why we need it:
add a kusion-control label in svc labels
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., design docs, usage docs, etc.: