Update calico template #3803

itajaja · 2017-11-08T17:34:41Z

changes taken from https://docs.projectcalico.org/v2.6/getting-started/kubernetes/installation/hosted/hosted
just a POC. not sure if I should update the 1.6 template or create a new template for 1.8 (maybe 1.7?) as I did. Do you have any suggestions on how to test this?

k8s-ci-robot · 2017-11-08T17:34:43Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

itajaja · 2017-11-08T17:41:14Z

signed

chrislovecnm

One version thing for you

chrislovecnm · 2017-11-08T18:09:56Z

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

@@ -494,7 +494,22 @@ func (b *BootstrapChannelBuilder) buildManifest() (*channelsapi.Addons, map[stri
 				Version:           fi.String(version),
 				Selector:          networkingSelector,
 				Manifest:          fi.String(location),
-				KubernetesVersion: ">=1.6.0",
+				KubernetesVersion: ">=1.6.0 <1.8.0",


what do you mean? is the selector wrong?

blakebarnett

I have an open branch with the changes I mentioned, and version matching like we're doing for Canal: https://github.com/kubernetes/kops/compare/master...blakebarnett:bdb/update_calico_for_k8s_1-8?expand=1

I didn't realize this PR was already in flight. Feel free to copy/use any of it.

blakebarnett · 2017-11-08T18:14:18Z

upup/models/cloudup/resources/addons/networking.projectcalico.org/k8s-1.8.yaml.template

+  cni_network_config: |-
+    {
+      "name": "k8s-pod-network",
+      "cniVersion": "0.3.0",


This should be 0.1.0 per: projectcalico/calico#742

should I update the 1.6 template as well?

blakebarnett · 2017-11-08T18:14:45Z

upup/models/cloudup/resources/addons/networking.projectcalico.org/k8s-1.8.yaml.template

+  namespace: kube-system
+data:
+  # The calico-etcd PetSet service IP:port
+  etcd_endpoints: "{{ $cluster := index .EtcdClusters 0 -}}


If we do this we should probably handle TLS too since that config can provide it now.

can you expand on this?

The new etcd configuration supports TLS, if someone happens to enable it there, we'll blow up the cluster unless we support it here.

blakebarnett · 2017-11-08T18:15:16Z

upup/models/cloudup/resources/addons/networking.projectcalico.org/k8s-1.8.yaml.template

+              value: "kops,bgp"
+            # Disable file logging so `kubectl logs` works.
+            - name: CALICO_DISABLE_FILE_LOGGING
+              value: "true"


We should add the FELIX defaults from the example configs

added the missing vars 👍

blakebarnett · 2017-11-08T18:16:16Z

upup/models/cloudup/resources/addons/networking.projectcalico.org/k8s-1.8.yaml.template

+
+---
+
+# This manifest deploys the Calico Kubernetes controllers.


You'll need to add a manifest that scales down calico-policy-controller from the old version or they'll conflict.

that's suboptimal tho, because people starting from scratch they will see a weird deployment scaled to 0 for no reason.

Agreed, but we'll have to just add it to to the release notes, or automatically delete it somehow (which seems potentially hairy)

chrislovecnm · 2017-11-08T18:28:10Z

/ok-to-test

itajaja · 2017-11-08T19:27:00Z

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go


 			addons.Spec.Addons = append(addons.Spec.Addons, &channelsapi.AddonSpec{
 				Name:              fi.String(key),
-				Version:           fi.String(version),
+				Version:           fi.String("2.4.1"),


changing this from 2.4.1-kops.1, was there a specific reason the version was different? can't find the code for that specific tag. if so, should it be kept and and are some changes to be done also for 2.6.2?

I don't actually know why that version exists, I got rid of it in my branch but maybe @chrislovecnm or @justinsb can enlighten us.

This should be the version of calico. We have another PR going in, which will force a rebase. @itajaja are you able to update this afternoon?

chrislovecnm · 2017-11-13T20:23:22Z

@itajaja #3844 will force a rebase

itajaja · 2017-11-13T20:24:51Z

no problem @chrislovecnm, I'll rebase, just ping back when the other pr is merged :)

chrislovecnm · 2017-11-13T21:49:29Z

Ping

itajaja · 2017-11-13T22:11:26Z

🙌

chrislovecnm

We need to get another PR in for the older manifests as well. Thanks!

chrislovecnm · 2017-11-13T22:33:48Z

/lgtm

chrislovecnm · 2017-11-13T22:34:39Z

/lgtm cancel

Is this still wip?

itajaja · 2017-11-13T22:36:04Z

We need to get another PR in for the older manifests as well. Thanks!

you mean for 1.6?

blakebarnett

It would be nice to have the clear version mapping like canal has: https://github.com/itajaja/kops/blob/3719d15c6b195aa58b2c4c8ff9e52c47dca71195/upup/pkg/fi/cloudup/bootstrapchannelbuilder.go#L521

Otherwise LGTM

itajaja · 2017-11-13T22:42:16Z

I guess the value there is just readability, since each field is accessed once? I think looking in the branch is pretty readable as well and makes the method shorter, but I can change it if you think the alternative is better 👍

blakebarnett · 2017-11-13T22:47:28Z

Yeah just easier to see what versions map to what at a glance, and to be consistent.

chrislovecnm · 2017-11-13T23:21:34Z

/lgtm

k8s-github-robot · 2017-11-13T23:22:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

itajaja · 2017-11-13T23:26:01Z

great! @chrislovecnm if you want to expand what you mean by "We need to get another PR in for the older manifests as well. Thanks!" I'll be happy to pr that as well

k8s-github-robot · 2017-11-13T23:32:42Z

/test all [submit-queue is verifying that this PR is safe to merge]

chrislovecnm · 2017-11-13T23:43:07Z

@itajaja i assume you tested the PR

Also we need the other calico manifests to have the new version of calico once them. Just bump the version in the manifest and in bootstrap - thanks for your help!

itajaja · 2017-11-13T23:53:52Z

@chrislovecnm I am not sure if there is an easy way to do more integration testing, the way I tested was to apply the template against my local toy cluster to see networking would work as expected.

k8s-github-robot · 2017-11-14T00:04:26Z

Automatic merge from submit-queue.

chrislovecnm · 2017-11-14T00:13:40Z

You able to update the other manifests?

That testing is perfect btw

itajaja · 2017-11-14T00:22:27Z

@chrislovecnm but then why creating a 1.8 if I could just update 1.6? Am I missing anything?

chrislovecnm · 2017-11-14T00:47:28Z

the rbac stuff does not work in 1.6. On my phone so I could be wrong. Does the manifest work on 1.8 and 1.7? I thought we had 1.8 specific stuff

itajaja · 2017-11-14T15:57:22Z

I just followed the canal PR tbh. it might be that the 1.8 template works with 1.6 as is, but it's probably not strictly necessary because pre 1.8 there is no egress policy, which strictly requires the new calico version

chrislovecnm · 2017-11-15T00:45:53Z

@itajaja yah I messed up 😞

I will move the 1.8 manifest to 1.6. Grumble. My mistake. We will need a 1.7+ manifest when we enable Calico to use the k8s api as a datasource

itajaja · 2017-11-15T01:00:33Z

lmk if I can help, I can replace the 1.6 with 1.8 manifest and remove 1.8.

chrislovecnm · 2017-11-15T03:55:57Z

Can you do the pre 1.6 template and test k8s 1.5? I have the branch already moving the 1.8 file around.

chrislovecnm · 2017-11-15T07:22:20Z

@itajaja problem is a bit more complicated - ran into a bug testing. k8s 1.6 is broken in master and in the last beta. The policy controller is not scheduling on the master. I am guessing that we need a toleration that works for ks8 1.6, and thus need a pre-1.6 manifest, a 1.6 manifest, and a 1.7+ manifest.

It is late, so I going to address this tomorrow. If you want to upgrade the pre-1.6 yaml please do.

blakebarnett · 2017-11-15T18:57:52Z

upup/models/cloudup/resources/addons/networking.projectcalico.org/k8s-1.8.yaml.template

+      # it isn't governed by policy that would prevent it from working.
+      hostNetwork: true
+      serviceAccountName: calico
+      tolerations:


So we should probably just change these to match what's in the current calico-node so it works on all k8s versions:

- key: CriticalAddonsOnly operator: Exists - effect: NoExecute operator: Exists - effect: NoSchedule operator: Exists

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2017

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 8, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 8, 2017

chrislovecnm reviewed Nov 8, 2017

View reviewed changes

blakebarnett suggested changes Nov 8, 2017

View reviewed changes

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 Nov 8, 2017

itajaja commented Nov 8, 2017

View reviewed changes

itajaja added 2 commits November 13, 2017 17:11

update calico template

af64c28

update templates and bootstrapper

3719d15

itajaja force-pushed the calico-2.6 branch from a06f779 to 3719d15 Compare November 13, 2017 22:11

chrislovecnm reviewed Nov 13, 2017

View reviewed changes

k8s-ci-robot assigned chrislovecnm Nov 13, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2017

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2017

itajaja changed the title ~~[wip] update calico template~~ Update calico template Nov 13, 2017

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 Nov 13, 2017

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2017

blakebarnett reviewed Nov 13, 2017

View reviewed changes

add version map

b7a16a2

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2017

blakebarnett approved these changes Nov 13, 2017

View reviewed changes

k8s-github-robot merged commit efa0a99 into kubernetes:master Nov 14, 2017

itajaja deleted the calico-2.6 branch November 14, 2017 00:07

blakebarnett reviewed Nov 15, 2017

View reviewed changes


		---

		# This manifest deploys the Calico Kubernetes controllers.

Update calico template #3803

Update calico template #3803

Conversation

itajaja commented Nov 8, 2017

k8s-ci-robot commented Nov 8, 2017

itajaja commented Nov 8, 2017

chrislovecnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakebarnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itajaja Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Nov 13, 2017

itajaja commented Nov 13, 2017 • edited Loading

chrislovecnm commented Nov 13, 2017

itajaja commented Nov 13, 2017

chrislovecnm left a comment

Choose a reason for hiding this comment

chrislovecnm commented Nov 13, 2017

chrislovecnm commented Nov 13, 2017

itajaja commented Nov 13, 2017

blakebarnett left a comment

Choose a reason for hiding this comment

itajaja commented Nov 13, 2017

blakebarnett commented Nov 13, 2017

chrislovecnm commented Nov 13, 2017

k8s-github-robot commented Nov 13, 2017

itajaja commented Nov 13, 2017

k8s-github-robot commented Nov 13, 2017

chrislovecnm commented Nov 13, 2017

itajaja commented Nov 13, 2017

k8s-github-robot commented Nov 14, 2017

chrislovecnm commented Nov 14, 2017

itajaja commented Nov 14, 2017

chrislovecnm commented Nov 14, 2017

itajaja commented Nov 14, 2017

chrislovecnm commented Nov 15, 2017

itajaja commented Nov 15, 2017

chrislovecnm commented Nov 15, 2017

chrislovecnm commented Nov 15, 2017

Choose a reason for hiding this comment

itajaja Nov 8, 2017 •

edited

Loading

itajaja commented Nov 13, 2017 •

edited

Loading