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

CAPI: remove kubernetes-cni install #259

Conversation

CecileRobertMichon
Copy link
Contributor

As discussed in https://kubernetes.slack.com/archives/CD6U2V71N/p1592508705375700 and #258, and in response to kubernetes/kubernetes#92242, this PR removes the kubernetes-cni package install from all CAPI builders.

The current thinking is that "we don't actually need kubernetes-cni, as in general CNI providers do not consume the binaries provided by it, but vendor/use their own copies (or alternatives)" - @detiber

/hold for discussion and testing
/cc @detiber @vincepri @codenrhoden @justaugustus

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 18, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 18, 2020
@CecileRobertMichon CecileRobertMichon changed the title [WIP] CAPI: remove kubernetes-cni install CAPI: remove kubernetes-cni install Jun 18, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2020
@detiber
Copy link
Member

detiber commented Jun 19, 2020

Changes look good to me

@CecileRobertMichon
Copy link
Contributor Author

Everything looks good on the Azure side. Built an image with these changes and built a cluster with CAPZ using that image.

/hold cancel

@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 Jun 22, 2020
@vincepri
Copy link
Member

@detiber Should we test on AWS side as well?

@detiber
Copy link
Member

detiber commented Jun 22, 2020

@detiber Should we test on AWS side as well?

Yes.

@CecileRobertMichon out of curiosity, did you also verify conformance against the test image as well?

@CecileRobertMichon
Copy link
Contributor Author

@detiber running conformance right now. It will take an hour or two before I get a signal. I'll hold again until we get a green light from all providers.

/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 Jun 22, 2020
@justaugustus
Copy link

Explicit hold for @kubernetes-sigs/release-engineering
/hold

We're still discussing approaches and may end up re-introducing the kubernetes-cni package.

@CecileRobertMichon
Copy link
Contributor Author

@justaugustus can you please expand on

We're still discussing approaches and may end up re-introducing the kubernetes-cni package

?

If this package is not needed in CAPI images then it makes sense to remove it regardless, even if sig-release fixes the failures we are seeing because of the version incompatibility, no? Or would there be an explicit dependency on kubernetes-cni which would cause installing new k8s versions to fail without the cni there? Also, do you have an ETA for a potential fix?

This is the only workaround we have that successfully builds v1.16.11, v1.17.7 and v1.18.4 CAPI images at the moment and we are currently blocked on building those images without it.

@justaugustus
Copy link

Also, do you have an ETA for a potential fix?

@CecileRobertMichon -- The best I have is "soon", at the moment (hopefully well before week's end).
I have an idea, that will become a PR which will include more details. Will keep you posted.

@justaugustus
Copy link

Opened kubernetes/release#1375 as a potential fix, which has more details in the PR description.

@justaugustus
Copy link

^^ That said, closing this in favor of #258.
/close

@k8s-ci-robot
Copy link
Contributor

@justaugustus: Closed this PR.

In response to this:

^^ That said, closing this in favor of #258.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants