-
Notifications
You must be signed in to change notification settings - Fork 38.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
service controller: if targetPort has changed will process by cloud-p… #77712
Conversation
Hi @Sn0rt. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign |
/ok-to-test |
dca3023
to
c2c0d71
Compare
/retest |
c2c0d71
to
4bf1822
Compare
@bowei hi, can you help me to check this PR again ? |
4bf1822
to
276b191
Compare
/test pull-kubernetes-integration |
276b191
to
9c3cedb
Compare
@@ -568,8 +568,9 @@ func portEqualForLB(x, y *v1.ServicePort) bool { | |||
return false | |||
} | |||
|
|||
// We don't check TargetPort; that is not relevant for load balancing | |||
// TODO: Should we blank it out? Or just check it anyway? | |||
if !reflect.DeepEqual(x.TargetPort, y.TargetPort) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should be able to do:
if x.TargetPort != y.TargetPort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the struct of targetPort is TargetPort intstr.IntOrString
. and I saw that the author to check status change or not by reflect.DeepEqual
in the other place of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be a struct of all non pointer types, so it should be straightforwardly comparable? Use of reflect is usually pretty heavyweight...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reasonable, I will fix it.
9c3cedb
to
b59be75
Compare
b59be75
to
790b9b2
Compare
/lgtm |
/test pull-kubernetes-e2e-gce-100-performance |
790b9b2
to
226840b
Compare
can we do the following in the test to make it less verbose and easier to read:
|
/test pull-kubernetes-integration |
…rovider Signed-off-by: guohaowang <wangguohao.2009@gmail.com>
226840b
to
d41f7b9
Compare
/test pull-kubernetes-kubemark-e2e-gce-big |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, Sn0rt 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 |
/retest Review the full test history for this PR. Silence the bot with an |
2 similar comments
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
if targetPort is changed will process by service controller.
/kind feature
Fixes #77586
in fact it's depend on
Cloud Provider
/ok-to-test