Skip to content
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

Merged
merged 1 commit into from Jun 22, 2019

Conversation

@Sn0rt
Copy link
Contributor

commented May 10, 2019

if targetPort is changed will process by service controller.

/kind feature

Fixes #77586

if targetPort is changed that will process by service controller 

in fact it's depend on Cloud Provider

/ok-to-test

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@bowei

This comment has been minimized.

Copy link
Member

commented May 10, 2019

/assign

@bowei

This comment has been minimized.

Copy link
Member

commented May 10, 2019

/ok-to-test

@Sn0rt Sn0rt force-pushed the Sn0rt:check-target-port-check branch 2 times, most recently from dca3023 to c2c0d71 May 10, 2019
@Sn0rt

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

/retest

@Sn0rt Sn0rt force-pushed the Sn0rt:check-target-port-check branch from c2c0d71 to 4bf1822 May 10, 2019
@Sn0rt

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@bowei hi, can you help me to check this PR again ?

@Sn0rt Sn0rt force-pushed the Sn0rt:check-target-port-check branch from 4bf1822 to 276b191 Jun 6, 2019
@Sn0rt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

/test pull-kubernetes-integration

@Sn0rt Sn0rt force-pushed the Sn0rt:check-target-port-check branch from 276b191 to 9c3cedb Jun 6, 2019
@@ -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) {

This comment has been minimized.

Copy link
@bowei

bowei Jun 10, 2019

Member

I think you should be able to do:

if x.TargetPort != y.TargetPort

This comment has been minimized.

Copy link
@Sn0rt

Sn0rt Jun 11, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@bowei

bowei Jun 18, 2019

Member

It seems to be a struct of all non pointer types, so it should be straightforwardly comparable? Use of reflect is usually pretty heavyweight...

This comment has been minimized.

Copy link
@Sn0rt

Sn0rt Jun 18, 2019

Author Contributor

reasonable, I will fix it.

@Sn0rt Sn0rt force-pushed the Sn0rt:check-target-port-check branch from 9c3cedb to b59be75 Jun 18, 2019
@Sn0rt Sn0rt force-pushed the Sn0rt:check-target-port-check branch from b59be75 to 790b9b2 Jun 18, 2019
@vllry

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

/lgtm
/priority important-longterm

@Sn0rt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

/test pull-kubernetes-e2e-gce-100-performance

@Sn0rt Sn0rt force-pushed the Sn0rt:check-target-port-check branch from 790b9b2 to 226840b Jun 21, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 21, 2019
@bowei

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

can we do the following in the test to make it less verbose and easier to read:

type serviceBuilder struct {
 object *v1.Service
}

func (sb *serviceBuilder) targetPort(v intstr) *serviceBuilder {
  sb.object = sb.object.DeepCopy()
  sb.object.Spec.Port[0].TargetPort = v
  return sb
}

// then your test case becomes

{
  ...
  updateFn: func() {
    oldSvc   = &v1.Service { ... }
    newSvc = &serviceBuilder{oldSvc}.targetPort(intstr.Parse("21")).object
  },
  expectedNeedsUpdate: true,
},
@Sn0rt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

/test pull-kubernetes-integration

…rovider

Signed-off-by: guohaowang <wangguohao.2009@gmail.com>
@Sn0rt Sn0rt force-pushed the Sn0rt:check-target-port-check branch from 226840b to d41f7b9 Jun 21, 2019
@Sn0rt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@bowei

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 21, 2019
@bowei

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

commented Jun 21, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

commented Jun 21, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

commented Jun 22, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 4b8a116 into kubernetes:master Jun 22, 2019
22 of 23 checks passed
22 of 23 checks passed
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation Sn0rt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@Sn0rt Sn0rt deleted the Sn0rt:check-target-port-check branch Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.