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

Add Calico v2.5 support for Kubernetes v1.8+ #3623

Merged
merged 1 commit into from
Oct 17, 2017

Conversation

KashifSaadat
Copy link
Contributor

@KashifSaadat KashifSaadat commented Oct 13, 2017

Added support for Canal (Calico) v2.5.1, which is required to work with Kubernetes v1.8.0+.

Older versions of Calico relied on ThirdPartyResources API to store it's config data, however this is now fully deprecated in Kubernetes v1.8 and has moved over to CustomResourceDefinitions (CRD). Calico v2.5+ has been updated to use CRD, however there is a manual upgrade process involved to migrate the configuration data across: https://github.com/projectcalico/calico/blob/master/upgrade/v2.5/README.md

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 13, 2017
@KashifSaadat
Copy link
Contributor Author

KashifSaadat commented Oct 13, 2017

/assign @chrislovecnm
/assign @justinsb

Some Notes:

  • I've tested with Kubernetes v1.7.2 and v1.8.0, both successful
  • I've included the changes from other recent PRs to add configurable options into the Canal manifest file (enabling Prometheus metrics, chain insert mode, etc)
  • On raising this PR I noticed there are failing integration and bootstrapchannelbuilder tests related to a recent Weave upgrade (PR Update Weave Net to version 2.0.5 #3614). I'm not sure how this has happened as the Travis CI tests should have picked this up and failed? I was looking to fix the tests in this PR but got a bit puzzled as it's failing to find a weave manifest for K8s v1.7, but the file exists. Any ideas?
  • Attempted to update the Calico networking manifest also but couldn't get it working. Would it be ok to raise an issue for this and add support separately?
  • We will need to include within the release notes about migration steps to Kubernetes v1.8+ when using Canal/Calico networking. Is there anywhere in the repo that you'd like me to update for a reference to this document: https://github.com/gunjan5/calico/blob/master/upgrade/v2.5/README.md

@chrislovecnm
Copy link
Contributor

chrislovecnm commented Oct 13, 2017

On raising this PR I noticed there are failing integration and bootstrapchannelbuilder tests related to a recent Weave upgrade (PR #3614). I'm not sure how this has happened as the Travis CI tests should have picked this up and failed? I was looking to fix the tests in this PR but got a bit puzzled as it's failing to find a weave manifest for K8s v1.7, but the file exists. Any ideas?

Ick ... checking now

@chrislovecnm
Copy link
Contributor

@KashifSaadat which test?

@KashifSaadat
Copy link
Contributor Author

Sorry ignore that!! It seems my git checkout must have screwed up a little, I had the bootstrapchannelbuilder changes but I think it was actually missing the 1.7 manifest file somehow. I've cleared up and done a fresh checkout and the tests are passing now. 👍

@chrislovecnm
Copy link
Contributor

I am guessing that the makefile did not regenerate go-bindata :)

@KashifSaadat
Copy link
Contributor Author

Ah yes that could have been it!

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Question about resources. I think we want QOS on these pods guaranteed :)

securityContext:
privileged: true
resources:
requests:
Copy link
Contributor

Choose a reason for hiding this comment

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

did we want to set limits and request the same to get QOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure that sounds sensible, will update.

name: cni-net-dir
# This container runs flannel using the kube-subnet-mgr backend
# for allocating subnets.
- name: kube-flannel
Copy link
Contributor

Choose a reason for hiding this comment

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

missing resources section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't defined in the existing canal templates. I'm not sure exactly what the limits should be set to here, so I'll use what was defined for the flannel networking templates.

Copy link
Contributor

Choose a reason for hiding this comment

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

May want to talk with @justinsb about it. Can we file an issue and revisit?

Copy link
Contributor

Choose a reason for hiding this comment

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

The canal / calico guys should tell us what to use ;)


# Calico Roles
# Pulled from https://docs.projectcalico.org/v2.5/getting-started/kubernetes/installation/hosted/rbac-kdd.yaml
kind: ClusterRole
Copy link
Contributor

Choose a reason for hiding this comment

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

@caseydavenport / @liggitt you mind reviewing?

@chrislovecnm
Copy link
Contributor

Attempted to update the Calico networking manifest also but couldn't get it working. Would it be ok to raise an issue for this and add support separately?

Just push another WIP PR, we can ping @caseydavenport and his amigos :)

@chrislovecnm
Copy link
Contributor

Fixes #3352

resources:
- pods/status
verbs:
- update
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be using patch to avoid dropping unknown fields (see flannel use of patch for nodes/status)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Casey mentioned I'm unable to change this currently and would need to wait for it to be resolved upstream in Calico. Tested just to make sure and my deployments were failing.

verbs:
- get
- list
- update
Copy link
Member

Choose a reason for hiding this comment

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

Calico updates node specs?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it does (though like you mention for pods/status it should really be using patch - there's an open issue against Calico to do that).

Calico stores some of its per-node configuration, etc. as annotations on the node.

@KashifSaadat
Copy link
Contributor Author

Should be ready for another review now :)

@chrislovecnm
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2017
@k8s-github-robot
Copy link

[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:

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

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2017
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit 1d5b281 into kubernetes:master Oct 17, 2017
@while1eq1 while1eq1 mentioned this pull request Aug 21, 2019
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants