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

Remove deprecated flag --conntrack-max from kube-proxy #78399

Merged

Conversation

@rikatz
Copy link
Contributor

commented May 27, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it: This PR removes the deprecated --conntrack-max from kube-proxy and also the tests related to this flag

Which issue(s) this PR fixes:

Fixes #78296

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

ACTION REQUIRED  The deprecated flag --conntrack-max has been removed from kube-proxy. Users of this flag should switch to --conntrack-min and --conntrack-max-per-core instead.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

Hi @rikatz. 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.

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

/assign @vllry

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

/uncc @cmluciano
/uncc @chuckha

@k8s-ci-robot k8s-ci-robot removed request for chuckha and cmluciano May 27, 2019

@yagonobre

This comment has been minimized.

Copy link
Member

commented May 27, 2019

/ok-to-test

@vllry

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

Cool, looks like you resolved the main issue! I'm curious how you figured it out. :)

@vllry

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

@rikatz you can remove the first test case in errorCases in TestValidateKubeProxyConntrackConfiguration. The test fails because that configuration is now valid (it was specifically testing that the now-removed negative Max value would be rejected).

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

Cool, looks like you resolved the main issue! I'm curious how you figured it out. :)

YES! :D

Actually I've missed some Test Cases to be adjusted. They were generating the fuzzers wrongly :)

Also, before sending the PR I did a make clean_generated && make generated_files to regenerate all the zz files. After all, running a make verify gave to me no error (don't know how I missed this validation test, but it worked).

Now let's really cross fingers and hope all the CI passes successfully :)

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

@rikatz you can remove the first test case in errorCases in TestValidateKubeProxyConntrackConfiguration. The test fails because that configuration is now valid (it was specifically testing that the now-removed negative Max value would be rejected).

Missed this comment as I was deeply trying to figure out what went wrong ;( But tks for pointing that

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

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

@rikatz rikatz force-pushed the rikatz:remove-deprecated-conntrack-max branch from d8d0dfe to 469706f May 29, 2019

@vllry

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Thanks @rikatz, looks good! The code freeze is imminent for 1.15, so this may have to wait until 1.16.

/assign @thockin
/assign @chuckha
/assign @cmluciano

@vllry

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 29, 2019

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

/retest

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

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

obj.Conntrack.Min = pointer.Int32Ptr(128 * 1024)
}

// If ConntrackMax is *not* set, use per-core scaling.

This comment has been minimized.

Copy link
@dcbw

dcbw May 30, 2019

Member

@rikatz Is this comment still accurate?

This comment has been minimized.

Copy link
@vllry

vllry May 30, 2019

Contributor

I don't think it is.

This comment has been minimized.

Copy link
@thockin
@dcbw

This comment has been minimized.

Copy link
Member

commented May 30, 2019

/lgtm

@thockin
Copy link
Member

left a comment

The comment can be fixed in a followup, please.

/approve

obj.Conntrack.Min = pointer.Int32Ptr(128 * 1024)
}

// If ConntrackMax is *not* set, use per-core scaling.

This comment has been minimized.

Copy link
@thockin
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, thockin

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

@claurence

This comment has been minimized.

Copy link

commented May 31, 2019

/milestone v1.15

@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone May 31, 2019

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

The comment can be fixed in a followup, please.

/approve

OK, will wait until this gets merged and open another one following just to some comments cleanup.

Also when the issue gets automatically closed, will reopen it just to associate with the comment cleanup. Is that ok?

@rikatz rikatz force-pushed the rikatz:remove-deprecated-conntrack-max branch from 469706f to 82c42bb Jun 1, 2019

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

@vllry As prow asked to rebase, I did it and also removed the unnecessary comment. Pinging you just to check if this is still good to enter into this milestone, otherwise no rush :)

@vllry

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

/lgtm

But I believe we missed the boat for 1.15. Luckily there's no pressure to get this out, I just thought it would be good to do sooner rather than later.

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

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

Got it. No problems at all. I’m leaving on vacations on Monday and will be away from computers until 18th, jun. Hope this doesn’t lock any other PRs, will try anyway to follow here and make the necessary changes ASAP.

Thanks for the patience and the mentorship, this community is amazing!

@rikatz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

/test pull-kubernetes-integration

@k8s-ci-robot k8s-ci-robot merged commit cc30c0d into kubernetes:master Jun 2, 2019

21 checks passed

cla/linuxfoundation rikatz 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-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big 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
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.