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.

Sign up for GitHub

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

Jump to bottom

Remove workaround with kube_proxy_remove #6512

Merged
merged 1 commit into from
Sep 17, 2020
Merged

Remove workaround with kube_proxy_remove #6512

merged 1 commit into from
Sep 17, 2020

Conversation

hafe
Copy link
Contributor

@hafe hafe commented Aug 8, 2020 •
edited
Loading

  • kube-proxy never gets deployed so need to remove it

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 8, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @hafe. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 8, 2020
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 8, 2020
@hafe
Copy link
Contributor Author

hafe commented Aug 8, 2020

I will remove the new exposed var and make it internal instead. kube-router users will then not be affected

@hafe hafe changed the title Make kubeadm init phases to skip configurable Remove workaround with kube_proxy_remove Aug 8, 2020
@hafe
Copy link
Contributor Author

hafe commented Aug 8, 2020

/cc @jjo

@k8s-ci-robot
Copy link
Contributor

@hafe: GitHub didn't allow me to request PR reviews from the following users: jjo.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @jjo

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
Copy link
Member

MrFreezeex commented Aug 8, 2020 •
edited
Loading

The upgrade path for kubeadm is only available with kubernetes 1.19 kubernetes/kubernetes#89593.

I would also prefer keeping the kube_proxy_remove and having the kubeadm_init_phases_skip variable be an array (with a join filter on the kubeadm command).
I don't know why you prevented users from overriding these variables by putting them into vars. AFAIK there is no internal variables in kubespray at the moment but maybe I am wrong

@hafe
Copy link
Contributor Author

hafe commented Aug 8, 2020

The upgrade path for kubeadm is only available with kubernetes 1.19 kubernetes/kubernetes#89593.

I would also prefer keeping the kube_proxy_remove and having the kubeadm_init_phases_skip variable be an array (with a join filter on the kubeadm command).
I don't know why you prevented users from overriding these variables by putting them into vars. AFAIK there is no internal variables in kubespray at the moment but maybe I am wrong

Thanks for the review! Will get back once I analyzed this

@hafe
Copy link
Contributor Author

hafe commented Aug 10, 2020

The upgrade path for kubeadm is only available with kubernetes 1.19 kubernetes/kubernetes#89593.

Thanks I was looking for that! I guess that means this should not be merged until master is on 1.19.x

I would also prefer keeping the kube_proxy_remove and

I think the name of this variable reflects that it configures a workaround. If the workaround goes, so should the variable. This cannot be a big deal since it is only for new deployments.

I think features should be exposed via configuration variables and this is not a feature. Selecting cillium/kube-router as a network solution is a feature and that (as before) can be used to modify the value of a new variable.

having the kubeadm_init_phases_skip variable be an array (with a join filter on the kubeadm command).

Fine, let me change that.

I don't know why you prevented users from overriding these variables by putting them into vars. AFAIK there is no internal variables in kubespray at the moment but maybe I am wrong

To minimize the exposed configuration interface. The value of the "remove" is today calculated based on net plugin, kind of internal to me. And yes there are vars files for OS dependent things.

I can make it configurable by putting it into defaults.

@hafe
Copy link
Contributor Author

hafe commented Aug 10, 2020

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 10, 2020
@woopstar
Copy link
Member

@hafe can you look into this again? :)

@hafe
Copy link
Contributor Author

hafe commented Aug 21, 2020

Yes I was thinking to get back to it once master is 1.19. It does not make sense before that

@hafe
Remove workaround with kube_proxy_remove
1fb62e2 
* kube-proxy never gets deployed so need to remove it
@hafe
Copy link
Contributor Author

hafe commented Sep 14, 2020

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2020
@EppO
Copy link
Contributor

EppO commented Sep 14, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2020
@EppO
Copy link
Contributor

EppO commented Sep 14, 2020

packet_fedora32-kube-ovn-containerd passed but CI status was never reported to Github

@floryut
Copy link
Member

floryut commented Sep 15, 2020

packet_fedora32-kube-ovn-containerd passed but CI status was never reported to Github

All good, I've retried it.
/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Sep 15, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 15, 2020
@EppO
Copy link
Contributor

EppO commented Sep 16, 2020

packet_fedora32-kube-ovn-containerd passed but CI status was never reported to Github

All good, I've retried it.
/ok-to-test

I did too couple of times but didn't help (at least yesterday)

@hafe hafe closed this Sep 16, 2020
@hafe hafe reopened this Sep 16, 2020
@woopstar
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hafe, woopstar

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0cc5e3e into kubernetes-sigs:master Sep 17, 2020
erulabs added a commit to kubesail/kubespray that referenced this pull request Sep 19, 2020
@erulabs
Merge branch 'master' of https://github.com/kubernetes-sigs/kubespray
f7cbc45 
* 'master' of https://github.com/kubernetes-sigs/kubespray: (21 commits)
  Make sure node_ip is set if node is in etcd group (kubernetes-sigs#6719)
  Fix order of OS CI cleanup (kubernetes-sigs#6714)
  Remove vagrant.deb from docker image (kubernetes-sigs#6717)
  Move floruyt to approver (kubernetes-sigs#6713)
  Add support for periodic CI (kubernetes-sigs#6715)
  Ignore pause from kubeadm config images list (kubernetes-sigs#6689)
  Ignore error in check mode when disabling swap (kubernetes-sigs#6703)
  flannel image arch specific tag (kubernetes-sigs#6685)
  Added missing permissions for operator. (kubernetes-sigs#6683)
  Add Kubernetes hashes 1.19.2/1.18.9/1.17.12 and set default (kubernetes-sigs#6698)
  Cleanup virsh volumes in Vagrant CI (kubernetes-sigs#6688)
  Use "kubeadm join" to join masters to control plane (kubernetes-sigs#6661)
  Remove workaround with kube_proxy_remove (kubernetes-sigs#6512)
  fix incorrect documentation of use_access_ip (kubernetes-sigs#6674)
  make metallb image repos configurable (kubernetes-sigs#6671) (kubernetes-sigs#6672)
  fix remove node (kubernetes-sigs#6666)
  Allow configuration of nodelabels in local_volume_provisioner (kubernetes-sigs#6620)
  Add support for Calico CNI host-local IPAM plugin (kubernetes-sigs#6580)
  Update third party librairies and tools (kubernetes-sigs#6669)
  Updated KataContainers version to 1.11.3 (kubernetes-sigs#6694)
  ...
@floryut floryut mentioned this pull request Nov 18, 2020
Closed
@EDGsheryl EDGsheryl mentioned this pull request Nov 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bozzo bozzo Awaiting requested review from bozzo

@floryut floryut Awaiting requested review from floryut

Assignees

@EppO EppO

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hafe @k8s-ci-robot @MrFreezeex @woopstar @EppO @floryut