Fix kube-proxy post deployment removal #5554

MrFreezeex · 2020-01-17T02:08:20Z

What type of PR is this?
/kind bug
/kind feature

What this PR does / why we need it:

It remove all the unnecessary kube_proxy_remove: false in defaults sections as the default is already sets in kubespray-defaults.
It removes an unnecessary condition that states that kube-proxy may be removed only if kubespray deploys local proxy on worker nodes for apiserver...

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

NONE

MrFreezeex · 2020-01-17T02:42:25Z

/retest

k8s-ci-robot · 2020-01-17T02:42:38Z

@MrFreezeex: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

MrFreezeex · 2020-01-17T03:44:42Z

/assign @ant31

MrFreezeex · 2020-03-04T15:08:45Z

I just rebased my PR, any chance to have this reviewed ? Deploying kubespray without kube-proxy is not possible without it...

MrFreezeex · 2020-03-07T00:22:17Z

I added a check to skip kube-proxy init phase only if kubernetes version is >= to 1.16.0.

roles/kubernetes/kubeadm/defaults/main.yml

MrFreezeex · 2020-05-02T13:22:19Z

I removed the check on Kubernetes 1.16 as It now required by kubespray as a minimum...

Miouge1 · 2020-05-28T11:41:01Z

Is there any CI job testing this?

MrFreezeex · 2020-05-28T12:01:18Z

@Miouge1 just added a test that enables kube-router to run with service proxy mode.

It will add kube_proxy_remove: true as per

kubespray/roles/kubespray-defaults/defaults/main.yaml

Line 30 in f959cc2

    
           kube_proxy_remove: "{{ (kube_network_plugin == 'kube-router') and (kube_router_run_service_proxy is defined and kube_router_run_service_proxy)| bool }}"

.

EppO · 2020-06-02T14:32:41Z

@Miouge1 just added a test that enables kube-router to run with service proxy mode.

It will add kube_proxy_remove: true as per

kubespray/roles/kubespray-defaults/defaults/main.yaml

Line 30 in f959cc2

kube_proxy_remove: "{{ (kube_network_plugin == 'kube-router') and (kube_router_run_service_proxy is defined and kube_router_run_service_proxy)| bool }}"

.

kube-router is not the only network plugin that can do without kube-proxy, cilium also has a no-kube-proxy mode. The best approach would be to have kube_proxy_remove's default set to false in kubespray-defaults like it is today and let the network plugin's defaults override it.
But some use cases may require it (like cilium can work with or without), so we need to let the user decide at the end (via the inventory).

MrFreezeex · 2020-06-02T15:21:20Z

@Miouge1 just added a test that enables kube-router to run with service proxy mode.
It will add kube_proxy_remove: true as per

kubespray/roles/kubespray-defaults/defaults/main.yaml

Line 30 in f959cc2

kube_proxy_remove: "{{ (kube_network_plugin == 'kube-router') and (kube_router_run_service_proxy is defined and kube_router_run_service_proxy)| bool }}"

.

kube-router is not the only network plugin that can do without kube-proxy, cilium also has a no-kube-proxy mode. The best approach would be to have kube_proxy_remove's default set to true in kubespray-defaults like it is today and let the network plugin's defaults override it.
But some use cases may require it (like cilium can work with or without), so we need to let the user decide at the end (via the inventory).

I may have misunderstood your point but regarding "default removals" this PR just fixes a bug because multiple defaults are conflicting (kubespray-defaults with others roles).

I am not fully aware of cillium installation but I don't see any reference to this variable in cillium playbook and kubespray-defaults...

Also kube_proxy_remove should be sets globally (because used by multiple roles) so It would be either in kubespray-defaults (like It is today) or sets by the user manually. I don't really understand your point about having this variable defaults to true.

EppO · 2020-06-02T18:49:57Z

I may have misunderstood your point but regarding "default removals" this PR just fixes a bug because multiple defaults are conflicting (kubespray-defaults with others roles).

I see the goal of your PR, I just wanted to think out loud of a better solution moving forward.

I am not fully aware of cillium installation but I don't see any reference to this variable in cillium playbook and kubespray-defaults...

That's because kubespray doesn't support this mode yet. Like I said, this mode is opt-in

Also kube_proxy_remove should be sets globally (because used by multiple roles) so It would be either in kubespray-defaults (like It is today) or sets by the user manually. I don't really understand your point about having this variable defaults to true.

Sorry I meant false by default. I fixed my comment. Default should be obviously to keep kube-proxy, but I wanted to see if that value could be driven by the network plugin capabilities by default with user override by the inventory. But it's true this value is used outside of the network plugin in several places so there might be a challenge here.

* Fix unwanted skipped task for kube-proxy * Fix kube_proxy_remove default Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>

MrFreezeex · 2020-06-28T17:45:38Z

I reduced the scope of the PR (not introducing changes to kubeadm init anymore)...

EppO · 2020-07-08T22:01:18Z

I ran the kube-router job with kube-proxy removal to see how it goes

MrFreezeex · 2020-07-08T22:57:26Z

The Gitlab pipeline is all green now 👀

EppO · 2020-07-08T23:33:37Z

/ok-to-test

EppO · 2020-07-08T23:33:43Z

https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/614716742 passed
/lgtm

MatthiasLohr · 2020-07-10T07:39:49Z

Is there anything one can help to speed up things here?

Miouge1 · 2020-07-13T14:09:34Z

/approve

k8s-ci-robot · 2020-07-13T14:09:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miouge1, MrFreezeex

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:

~~OWNERS~~ [Miouge1]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* 'master' of https://github.com/kubernetes-sigs/kubespray: Add a way to deploy cilium alongside another CNI (kubernetes-sigs#6373) Cleanup old build-cephfs-provisioner.yml playbook (kubernetes-sigs#6418) Always enable GitLab CI artifacts for cluster-dump (kubernetes-sigs#6412) Remove allow-release-candidate-upgrades already include in experimental-upgrades flag (kubernetes-sigs#6349) add calico-node selinux (kubernetes-sigs#6359) Add oomichi to reviwers of MetalLB addon (kubernetes-sigs#6393) Respect kube_override_hostname during removal/upgrade (kubernetes-sigs#6347) Fixed fedora modular repos activation for fcos (kubernetes-sigs#6300) Fix kube-proxy post deployment removal (kubernetes-sigs#5554) Remove old csi-attacher flag and fix RBAC for Cinder CSI (kubernetes-sigs#6358) Update cilium minimum kernel preinstall check (kubernetes-sigs#6376) Add readiness probe to dns-autoscaler (kubernetes-sigs#6382) Add Fedora CoreOS kubevirt image for tests (kubernetes-sigs#6337) allow kubeadm to upgrade etcd (kubernetes-sigs#6345) crio: harden downloads with retry (kubernetes-sigs#6374) Add workaround with include_task for mitogen (kubernetes-sigs#6312)

* Fix kube-proxy removal * Fix unwanted skipped task for kube-proxy * Fix kube_proxy_remove default Signed-off-by: Arthur Outhenin-Chalandre <[email protected]> * Add test for kube-router svc proxy Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 17, 2020

k8s-ci-robot requested review from holmsten and mirwan January 17, 2020 02:09

MrFreezeex changed the title ~~Skip kube-proxy addon phase when needed and fix kube-proxy post deployment~~ Skip kube-proxy addon phase when needed and fix kube-proxy post deployment removal Jan 17, 2020

MrFreezeex force-pushed the fix-kube-proxy-remove branch 2 times, most recently from 70b1e1d to e5ef43f Compare January 17, 2020 02:58

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 17, 2020

MrFreezeex force-pushed the fix-kube-proxy-remove branch 2 times, most recently from 31cb023 to 23993ff Compare January 17, 2020 03:25

k8s-ci-robot assigned ant31 Jan 17, 2020

MrFreezeex force-pushed the fix-kube-proxy-remove branch from 23993ff to f3871c1 Compare March 4, 2020 13:59

MrFreezeex mentioned this pull request Mar 6, 2020

Utilize kubeadm init --skip-phases=addon/kube-proxy for k8s v1.16 (and newer) #5694

Closed

MrFreezeex force-pushed the fix-kube-proxy-remove branch from f3871c1 to 584d5ac Compare March 6, 2020 23:07

mattymo reviewed Mar 17, 2020

View reviewed changes

roles/kubernetes/kubeadm/defaults/main.yml Show resolved Hide resolved

MrFreezeex force-pushed the fix-kube-proxy-remove branch from 584d5ac to fe33d81 Compare May 2, 2020 13:20

MrFreezeex force-pushed the fix-kube-proxy-remove branch from fe33d81 to b5fd6c6 Compare May 28, 2020 11:58

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 28, 2020

MrFreezeex mentioned this pull request Jun 28, 2020

Adding kube-proxy-replacement support in cilium #6334

Merged

MrFreezeex added 2 commits June 28, 2020 19:43

Fix kube-proxy removal

d55af93

* Fix unwanted skipped task for kube-proxy * Fix kube_proxy_remove default Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>

Add test for kube-router svc proxy

dc47ff8

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>

MrFreezeex force-pushed the fix-kube-proxy-remove branch from b5fd6c6 to dc47ff8 Compare June 28, 2020 17:43

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 28, 2020

MrFreezeex changed the title ~~Skip kube-proxy addon phase when needed and fix kube-proxy post deployment removal~~ Fix kube-proxy post deployment removal Jun 28, 2020

MrFreezeex mentioned this pull request Jul 3, 2020

Add a way to deploy cilium alongside another CNI #6373

Merged

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 8, 2020

k8s-ci-robot assigned EppO Jul 8, 2020

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2020

k8s-ci-robot merged commit abfa163 into kubernetes-sigs:master Jul 13, 2020

MrFreezeex mentioned this pull request Mar 14, 2023

REQUEST: New membership for MrFreezeex kubernetes/org#4089

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix kube-proxy post deployment removal #5554

Fix kube-proxy post deployment removal #5554

MrFreezeex commented Jan 17, 2020 •

edited

Loading

MrFreezeex commented Jan 17, 2020

k8s-ci-robot commented Jan 17, 2020

MrFreezeex commented Jan 17, 2020

MrFreezeex commented Mar 4, 2020 •

edited

Loading

MrFreezeex commented Mar 7, 2020 •

edited

Loading

MrFreezeex commented May 2, 2020

Miouge1 commented May 28, 2020

MrFreezeex commented May 28, 2020 •

edited

Loading

EppO commented Jun 2, 2020 •

edited

Loading

MrFreezeex commented Jun 2, 2020 •

edited

Loading

EppO commented Jun 2, 2020 •

edited

Loading

MrFreezeex commented Jun 28, 2020

EppO commented Jul 8, 2020

MrFreezeex commented Jul 8, 2020

EppO commented Jul 8, 2020

EppO commented Jul 8, 2020 •

edited

Loading

MatthiasLohr commented Jul 10, 2020

Miouge1 commented Jul 13, 2020

k8s-ci-robot commented Jul 13, 2020

Fix kube-proxy post deployment removal #5554

Fix kube-proxy post deployment removal #5554

Conversation

MrFreezeex commented Jan 17, 2020 • edited Loading

MrFreezeex commented Jan 17, 2020

k8s-ci-robot commented Jan 17, 2020

MrFreezeex commented Jan 17, 2020

MrFreezeex commented Mar 4, 2020 • edited Loading

MrFreezeex commented Mar 7, 2020 • edited Loading

MrFreezeex commented May 2, 2020

Miouge1 commented May 28, 2020

MrFreezeex commented May 28, 2020 • edited Loading

EppO commented Jun 2, 2020 • edited Loading

MrFreezeex commented Jun 2, 2020 • edited Loading

EppO commented Jun 2, 2020 • edited Loading

MrFreezeex commented Jun 28, 2020

EppO commented Jul 8, 2020

MrFreezeex commented Jul 8, 2020

EppO commented Jul 8, 2020

EppO commented Jul 8, 2020 • edited Loading

MatthiasLohr commented Jul 10, 2020

Miouge1 commented Jul 13, 2020

k8s-ci-robot commented Jul 13, 2020

MrFreezeex commented Jan 17, 2020 •

edited

Loading

MrFreezeex commented Mar 4, 2020 •

edited

Loading

MrFreezeex commented Mar 7, 2020 •

edited

Loading

MrFreezeex commented May 28, 2020 •

edited

Loading

EppO commented Jun 2, 2020 •

edited

Loading

MrFreezeex commented Jun 2, 2020 •

edited

Loading

EppO commented Jun 2, 2020 •

edited

Loading

EppO commented Jul 8, 2020 •

edited

Loading