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

Bump Cilium to 1.7 for k8s 1.12+ #8589

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

olemarkus
Copy link
Member

Cilium 1.7 requires K8s 1.12 minimum. Changed the templates so that we
can have different cilium versions for different k8s versions.

This also mean that this addon will behave similar to other addons wrt
upgrades. Cilium used to add a fixed version to the cluster spec on cluster creation so
upgrades were slightly more manual. Now, for new clusters, upgrades will
happen implicitly with kops updates unless the .Version is added
manually to the cluster spec.

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

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

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 19, 2020
@hakman
Copy link
Member

hakman commented Feb 19, 2020

/ok-to-test

@k8s-ci-robot 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 Feb 19, 2020
@hakman
Copy link
Member

hakman commented Feb 19, 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 Feb 19, 2020
- create
- get
- list
- watch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't removing this adversely affect version 1.6?

@olemarkus
Copy link
Member Author

Yes you may be right there. I'll add it back in

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2020
@johngmyers
Copy link
Member

You could put a version conditional in the template.

Cilium 1.7 requires K8s 1.12 minimum. Changed the templates so that we
can have different cilium versions for different k8s versions.

This also mean that this addon will behave similar to other addons wrt
upgrades. Cilium used to add a fixed version to the cluster spec on cluster creation so
upgrades were slightly more manual. Now, for new clusters, upgrades will
happen implicitly with kops updates unless the .Version is added
manually to the cluster spec.
@olemarkus
Copy link
Member Author

You could put a version conditional in the template.

I would if this was more security sensitive. But this particular change is basically just about dropping the deprecated k8s extensions apis.

@@ -593,9 +593,6 @@ func (c *Cluster) FillDefaults() error {
} else if c.Spec.Networking.AmazonVPC != nil {
// OK
} else if c.Spec.Networking.Cilium != nil {
if c.Spec.Networking.Cilium.Version == "" {
c.Spec.Networking.Cilium.Version = CiliumDefaultVersion
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of baking the logic in the templates, this could have picked the default version by doing check on the Kubernetes version. That would allow other Go code to do Cilium version checks. But we don't have any such Cilium version checks, so not a blocker.

@johngmyers
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2020
version := "1.6.6-kops.0"
version := "1.7.0-kops.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use something like this here?

versions := map[string]string{
"k8s-1.8": "1.5.0-kops.1",
"k8s-1.10": "1.5.0-kops.2",
"k8s-1.12": "1.5.5-kops.1",
"k8s-1.16": "1.6.0-kops.1",
}

Copy link
Member

@rifelpet rifelpet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus, rifelpet

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 Feb 19, 2020
@k8s-ci-robot k8s-ci-robot merged commit ea8c8fb into kubernetes:master Feb 19, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 19, 2020
@hakman
Copy link
Member

hakman commented Feb 20, 2020

Seems that e2e for Cilium is failing after this change. @olemarkus any thoughts?
https://prow.k8s.io/view/gcs/kubernetes-jenkins/logs/ci-kubernetes-e2e-kops-aws-cni-cilium/1230338989827821576

Feb 20 03:57:17 ip-172-20-46-251 kubelet[8295]: E0220 03:57:17.599360    8295 kubelet.go:2183] Container runtime network not ready: NetworkReady=false reason:NetworkPluginNotReady message:docker: network plugin is not ready: cni config uninitialized
Feb 20 03:57:17 ip-172-20-46-251 kubelet[8295]: E0220 03:57:17.730033    8295 remote_image.go:113] PullImage "docker.io/cilium/cilium:v.1.7.0" from image service failed: rpc error: code = Unknown desc = Error response from daemon: manifest for cilium/cilium:v.1.7.0 not found: manifest unknown: manifest unknown
Feb 20 03:57:17 ip-172-20-46-251 kubelet[8295]: E0220 03:57:17.730068    8295 kuberuntime_image.go:50] Pull image "docker.io/cilium/cilium:v.1.7.0" failed: rpc error: code = Unknown desc = Error response from daemon: manifest for cilium/cilium:v.1.7.0 not found: manifest unknown: manifest unknown
Feb 20 03:57:17 ip-172-20-46-251 kubelet[8295]: E0220 03:57:17.730109    8295 kuberuntime_manager.go:801] container start failed: ErrImagePull: rpc error: code = Unknown desc = Error response from daemon: manifest for cilium/cilium:v.1.7.0 not found: manifest unknown: manifest unknown
Feb 20 03:57:17 ip-172-20-46-251 kubelet[8295]: E0220 03:57:17.730135    8295 pod_workers.go:191] Error syncing pod 18f0acff-0eb7-449f-b5fa-c7d6cee008e2 ("cilium-68c4n_kube-system(18f0acff-0eb7-449f-b5fa-c7d6cee008e2)"), skipping: failed to "StartContainer" for "cilium-agent" with ErrImagePull: "rpc error: code = Unknown desc = Error response from daemon: manifest for cilium/cilium:v.1.7.0 not found: manifest unknown: manifest unknown"
Feb 20 03:57:18 ip-172-20-46-251 kubelet[8295]: E0220 03:57:18.268619    8295 pod_workers.go:191] Error syncing pod 18f0acff-0eb7-449f-b5fa-c7d6cee008e2 ("cilium-68c4n_kube-system(18f0acff-0eb7-449f-b5fa-c7d6cee008e2)"), skipping: failed to "StartContainer" for "cilium-agent" with ImagePullBackOff: "Back-off pulling image \"docker.io/cilium/cilium:v.1.7.0\""

My guess is that it's the extra . in the image tag:

image: "docker.io/cilium/cilium:{{- or .Version "v.1.7.0" }}"

@olemarkus
Copy link
Member Author

Yep. I'll have a PR momentarily

k8s-ci-robot added a commit that referenced this pull request Feb 25, 2020
…#8591-origin-release-1.17

Automated cherry pick of #8589: Bump Cilium to 1.7 for k8s 1.12+ #8591: Fix typo in the cilium default version
k8s-ci-robot added a commit that referenced this pull request Feb 29, 2020
…#8591-origin-release-1.16

Automated cherry pick of #8589: Bump Cilium to 1.7 for k8s 1.12+ #8591: Fix typo in the cilium default version
@errordeveloper errordeveloper mentioned this pull request Mar 4, 2020
32 tasks
olemarkus added a commit to olemarkus/kops that referenced this pull request Mar 5, 2020
…for k8s 1.12+ kubernetes#8591: Fix typo in the cilium default version"
k8s-ci-robot added a commit that referenced this pull request Mar 5, 2020
…pick-of-#8589-#8591-origin-release-1.16

Revert "Automated cherry pick of #8589: Bump Cilium to 1.7 for k8s 1.12+ #8591: Fix typo in the cilium default version"
@olemarkus olemarkus deleted the cilium-1-7 branch March 25, 2020 20:02
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. 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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants