kubeproxy: Migrate `cleanup` flag to config #94559

mayankshah1607 · 2020-09-05T12:44:35Z

Signed-off-by: Mayank Shah [email protected]

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR migrates --cleanup flag to kube-proxy config
Which issue(s) this PR fixes:

Part of #86024

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The `--cleanup` kube-proxy configuration flag will be moved to the proxy config file.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2020-09-05T12:44:43Z

Welcome @mayankshah1607!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-09-05T12:44:43Z

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

mayankshah1607 · 2020-09-05T12:46:21Z

Pinging @mtaufen for a review! 😄

k8s-ci-robot · 2020-09-05T12:47:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mayankshah1607
To complete the pull request process, please assign thockin
You can assign the PR to them by writing /assign @thockin in a comment when ready.

The full list of commands accepted by this bot can be found 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 · 2020-09-05T13:12:33Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

mtaufen · 2020-09-05T18:53:27Z

/ok-to-test

mtaufen · 2020-09-05T19:01:37Z

/lgtm

mtaufen · 2020-09-05T19:04:55Z

/retest

Signed-off-by: Mayank Shah <[email protected]>

cmluciano

This looks good, one comment on how to fix the tests

cmluciano · 2020-09-08T17:04:17Z

pkg/proxy/apis/config/v1alpha1/zz_generated.conversion.go

@@ -123,6 +123,7 @@ func autoConvert_v1alpha1_KubeProxyConfiguration_To_config_KubeProxyConfiguratio
 	}
 	out.ShowHiddenMetricsForVersion = in.ShowHiddenMetricsForVersion
 	out.DetectLocalMode = config.LocalMode(in.DetectLocalMode)
+	out.CleanupAndExit = in.CleanupAndExit


The round trip test is failing because you are missing this line for the reverse autoConvert_config_KubeProxyConfiguration_To_v1alpha1_KubeProxyConfiguration on line 167.

You can add this automatically by either running the test bazel test //pkg/proxy/apis/config/scheme:go_default_test or running make update

Signed-off-by: Mayank Shah <[email protected]>

mayankshah1607 · 2020-09-09T10:33:37Z

Thanks @cmluciano @mtaufen - I've fixed it! Not really sure why the Azure disk E2E is failing though. Any tips on that?
Thanks.

cmluciano · 2020-09-09T17:57:27Z

Thanks @cmluciano @mtaufen - I've fixed it! Not really sure why the Azure disk E2E is failing though. Any tips on that?
Thanks.

I was not even aware that this job existed. It seems pretty flaky.

/retest

mayankshah1607 · 2020-09-10T06:53:08Z

Thanks! The tests seem to pass now :)

cmluciano

Can you add a release note on this too @mayankshah1607 ?

mayankshah1607 · 2020-09-10T16:22:09Z

@cmluciano Done! :)

mtaufen · 2020-09-10T22:20:23Z

/lgtm

aojea · 2020-09-14T11:50:04Z

doe sit makes sense to move this flag to config?
is just doing that kube-proxy delete the leftover iptables or ipvs rules and exits, it seems awkward to me to use it as a configuration parameter, I mean, if you set it it will keep restarting the kube-proxy and delete the rules forever, until you switch it back to false.

/assign @thockin

mtaufen · 2020-09-14T18:24:38Z

Then does this flag only exist for using kube-proxy as a CLI tool?

aojea · 2020-09-14T21:10:51Z

Then does this flag only exist for using kube-proxy as a CLI tool?

I have the same question 😄 , I would say yes, but Tim will know best for sure

mayankshah1607 · 2020-09-29T08:37:08Z

Hi @mtaufen @aojea @thockin
Just wanted to quickly check with you regarding the status of this PR. Do we have to make any changes to this?
Thanks :)

thockin · 2020-11-05T00:38:50Z

I lost track of this PR, so sorry!

I do NOT think we should move this into config - it's a very imperative and human-oriented flag.

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 5, 2020

k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 5, 2020

k8s-ci-robot requested review from johnbelamaric and thockin September 5, 2020 12:46

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 5, 2020

k8s-ci-robot assigned mtaufen Sep 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2020

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2020

kubeproxy: Migrate cleanup flag to config

58b1728

Signed-off-by: Mayank Shah <[email protected]>

mayankshah1607 force-pushed the mayank/cleanup-kubeproxy-flag branch from 6491b3d to 58b1728 Compare September 7, 2020 15:23

cmluciano reviewed Sep 8, 2020

View reviewed changes

fix bazel test

90bb8cc

Signed-off-by: Mayank Shah <[email protected]>

cmluciano reviewed Sep 10, 2020

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 10, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2020

k8s-ci-robot assigned thockin Sep 14, 2020

mayankshah1607 mentioned this pull request Oct 27, 2020

REQUEST: New membership for mayankshah1607 kubernetes/org#2290

Closed

6 tasks

thockin closed this Nov 5, 2020

knabben mentioned this pull request Apr 25, 2021

Migrate kube-proxy flags to config #86024

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeproxy: Migrate `cleanup` flag to config #94559

kubeproxy: Migrate `cleanup` flag to config #94559

mayankshah1607 commented Sep 5, 2020 •

edited

Loading

k8s-ci-robot commented Sep 5, 2020

k8s-ci-robot commented Sep 5, 2020

mayankshah1607 commented Sep 5, 2020

k8s-ci-robot commented Sep 5, 2020

fejta-bot commented Sep 5, 2020

mtaufen commented Sep 5, 2020

mtaufen commented Sep 5, 2020

mtaufen commented Sep 5, 2020

cmluciano left a comment

cmluciano Sep 8, 2020

mayankshah1607 commented Sep 9, 2020

cmluciano commented Sep 9, 2020

mayankshah1607 commented Sep 10, 2020

cmluciano left a comment

mayankshah1607 commented Sep 10, 2020

mtaufen commented Sep 10, 2020

aojea commented Sep 14, 2020

mtaufen commented Sep 14, 2020

aojea commented Sep 14, 2020

mayankshah1607 commented Sep 29, 2020

thockin commented Nov 5, 2020

kubeproxy: Migrate cleanup flag to config #94559

kubeproxy: Migrate cleanup flag to config #94559

Conversation

mayankshah1607 commented Sep 5, 2020 • edited Loading

k8s-ci-robot commented Sep 5, 2020

k8s-ci-robot commented Sep 5, 2020

mayankshah1607 commented Sep 5, 2020

k8s-ci-robot commented Sep 5, 2020

fejta-bot commented Sep 5, 2020

mtaufen commented Sep 5, 2020

mtaufen commented Sep 5, 2020

mtaufen commented Sep 5, 2020

cmluciano left a comment

Choose a reason for hiding this comment

cmluciano Sep 8, 2020

Choose a reason for hiding this comment

mayankshah1607 commented Sep 9, 2020

cmluciano commented Sep 9, 2020

mayankshah1607 commented Sep 10, 2020

cmluciano left a comment

Choose a reason for hiding this comment

mayankshah1607 commented Sep 10, 2020

mtaufen commented Sep 10, 2020

aojea commented Sep 14, 2020

mtaufen commented Sep 14, 2020

aojea commented Sep 14, 2020

mayankshah1607 commented Sep 29, 2020

thockin commented Nov 5, 2020

kubeproxy: Migrate `cleanup` flag to config #94559

kubeproxy: Migrate `cleanup` flag to config #94559

mayankshah1607 commented Sep 5, 2020 •

edited

Loading