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

Update Calico and Canal to use calico node v2.6.7 #4447

Merged
merged 3 commits into from
Mar 1, 2018
Merged

Update Calico and Canal to use calico node v2.6.7 #4447

merged 3 commits into from
Mar 1, 2018

Conversation

tmjd
Copy link
Contributor

@tmjd tmjd commented Feb 15, 2018

  • Canal updated cni to v1.11.2

@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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 15, 2018
@tmjd tmjd mentioned this pull request Feb 15, 2018
Closed
@chrislovecnm
Copy link
Contributor

/assign @robinpercy

robinpercy
robinpercy reviewed Feb 20, 2018
View reviewed changes
upup/pkg/fi/cloudup/bootstrapchannelbuilder.go Outdated
@@ -476,8 +476,8 @@ func (b *BootstrapChannelBuilder) buildManifest() (*channelsapi.Addons, map[stri
key := "networking.projectcalico.org"
versions := map[string]string{
"pre-k8s-1.6": "2.4.1",
"k8s-1.6": "2.6.6",
"k8s-1.7": "2.6.6",
"k8s-1.6": "2.6.8",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR @tmjd.

Should these versions not be 2.6.7?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, I had duplicated what was done with the pre-k8s-1.6 as it has +1 on the minor. I think maybe that one was bumped to 2.4.1 when it should have been 2.4.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be safe to change the pre-k8s-1.6 to 2.4.0? I'm not sure if that would break anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the minor +1 convention was specific to (148a65e). So we should definitely leave the pre-k8s-1.6 version as-is.

If I'm understanding the original intention correctly, the new versions should be 2.6.7-kops.1 for both canal and calico. (As a preemptive step for the latter). Then if future kops-specific patches are necessary, we'll just bump the suffix.

@justinsb can you please confirm this is the versioning convention you had in mind in 148a65e?

@tmjd tmjd force-pushed the canal-n-calico-to-2-6-7 branch from 22c1041 to 7629ff3 Compare February 20, 2018 16:11
@justinsb justinsb added this to the 1.9 milestone Feb 21, 2018
blakebarnett
blakebarnett approved these changes Feb 21, 2018
View reviewed changes
Copy link

@blakebarnett blakebarnett left a comment

Choose a reason for hiding this comment

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

LGTM aside from the potential issues with terminationGracePeriodSeconds: 0

upup/models/cloudup/resources/addons/networking.projectcalico.org.canal/k8s-1.8.yaml.template
@@ -98,12 +98,15 @@ spec:
# for cluster communication
- effect: NoSchedule
operator: Exists
# Minimize downtime during a rolling upgrade or deletion; tell Kubernetes to do a "force
# deletion": https://kubernetes.io/docs/concepts/workloads/pods/pod/#termination-of-pods.
terminationGracePeriodSeconds: 0
Copy link

Choose a reason for hiding this comment

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

Hmm, have you tested this behavior? Does calico handle being killed like this gracefully, I imagine there's some cleanup on shutdown...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the manifests (if not all, we missed updating them) on the Calico docs site have been updated to include terminationGracePeriodSeconds: 0 so this being set has seen a decent amount of testing.
There is no cleanup I'm aware of on calico-node shutdown.

robinpercy
robinpercy reviewed Feb 21, 2018
View reviewed changes
upup/pkg/fi/cloudup/bootstrapchannelbuilder.go Outdated
@@ -532,7 +532,7 @@ func (b *BootstrapChannelBuilder) buildManifest() (*channelsapi.Addons, map[stri
versions := map[string]string{
"pre-k8s-1.6": "2.4.2-kops.1",
"k8s-1.6": "2.4.2-kops.1",
"k8s-1.8": "2.6.3-kops.2",
"k8s-1.8": "2.6.8-kops.2",
Copy link
Contributor

@robinpercy robinpercy Feb 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I believe hte version should be 2.6.7-kops.1 but would like to get @justinsb's confirmation. If so, I suspect we should add the suffix to the calico addons above as well. (Based on 148a65e)

Copy link
Member

Choose a reason for hiding this comment

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

That's correct, yes. So the idea is that calico releases their version 2.6.7, and then kops releases a version. We have to give our version a semver.

Suppose we gave it 2.6.7, but then we realized that there was a problem with our packaging. We have nowhere to go, that is higher than 2.6.7. This has happened in the past, and we've had to do ugly things like 2.6.8-kops.pre.1 or something equally bad.

Instead we label it 2.6.7-kops.1. So it's the first version of the kops packaging of calico 2.6.7. Then if we have a kops packaging issue, we can do 2.6.7-kops.2, 2.6.7-kops.3 etc. And when calico releases 2.6.8 we go back to 2.6.8-kops.1 again (at least at the moment - if we maintained packages of multiple versions of calico we might have a different strategy)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the confirmation. I've pushed an update fixing this.

@tmjd tmjd force-pushed the canal-n-calico-to-2-6-7 branch from 7629ff3 to 3484feb Compare February 28, 2018 15:54
@chrislovecnm
Copy link
Contributor

ci is not happy, if you run make ci locally you will see the problem. A rebase may help!

@justinsb
Copy link
Member

justinsb commented Mar 1, 2018

/ok-to-test

/lgtm

(Tried fixing the merge using github's online functionality)

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 1, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, tmjd

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 Mar 1, 2018
@k8s-ci-robot k8s-ci-robot merged commit 70e09ad into kubernetes:master Mar 1, 2018
@tmjd tmjd deleted the canal-n-calico-to-2-6-7 branch March 7, 2018 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@robinpercy robinpercy robinpercy left review comments

@blakebarnett blakebarnett blakebarnett approved these changes

Assignees

@justinsb justinsb

@robinpercy robinpercy

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@tmjd @chrislovecnm @justinsb @k8s-ci-robot @robinpercy @blakebarnett