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

Fix Flannel nonMasqueradeCIDR #3952

Merged

Conversation

mikesplain
Copy link
Contributor

@mikesplain mikesplain commented Nov 28, 2017

Fixes #3950

And looks like we forgot to bump the version in bootstrapchannelbuilder.go(moved to #3953) (thanks @jkemp101)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 28, 2017
@chrislovecnm
Copy link
Contributor

@mikesplain so this needs to be two PRs. First of this can cause breaking changes with upgrades using nonMasqCIDR properly.

We are going to need boostrapchannelbuilder.go bumped for 1.8.0, if we can, and we need to figure out migration for the CIDR.

@mikesplain mikesplain mentioned this pull request Nov 28, 2017
k8s-github-robot pushed a commit that referenced this pull request Nov 28, 2017
Automatic merge from submit-queue.

Fix flannel version

Breaking out from #3952 since this is needed for Kops 1.8
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 29, 2017
@chrislovecnm
Copy link
Contributor

/hold

As we chatted about - how do we upgrade flannel between cidrs

@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 Nov 30, 2017
@justinsb justinsb added this to the 1.9 milestone Dec 1, 2017
@KashifSaadat
Copy link
Contributor

Hey @chrislovecnm, we should probably get this in pretty soon.

I'm not sure this would have ever worked correctly unless a user's Cluster Spec had the samenonMasqueradeCIDR defined, because nodes fail to provision otherwise (clusterDNS is set incorrectly, causing DNS lookups to fail and you get the placeholder address 203.0.113.123 in logs).

@chrislovecnm
Copy link
Contributor

How does a migration work between cidrs with flannel? Before we get this in we need to figure out how to transition users.

@KashifSaadat
Copy link
Contributor

KashifSaadat commented Dec 12, 2017

I'm not sure whether there would be a migration for existing users on flannel. Might be wrong but I think these are the current two scenarios for existing users:

  • Kops ClusterSpec configured with different nonMasqueradeCIDR: Provisioning cluster will fail because clusterDNS etc will be set to addresses different to what flannel would provision (based on the value hardcoded in the manifest file) - I had failures on the kubelet services across nodes, failing to resolve
  • Kops ClusterSpec configured with nonMasqueradeCIDR: 100.64.0.0/10 (matching manifest file): Cluster would be in working condition and the PR should have no impact as the addresses will stay the same

So in the second scenario there wouldn't be any migration to perform, or am I missing something here? I don't know much about flannel :(

@justinsb
Copy link
Member

Ah - good point @KashifSaadat . So nobody could have a working cluster anyway with flannel + a different non-masquerade cidr.

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2017
justinsb added a commit to justinsb/kops that referenced this pull request Dec 14, 2017
@justinsb
Copy link
Member

Send a bootstrapchannelbuilder bump in #4064

@chrislovecnm
Copy link
Contributor

/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 Dec 14, 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 3fc8ddd into kubernetes:master Dec 14, 2017
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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants