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

Flannel: change default backend type #3190

Merged
merged 2 commits into from
Sep 13, 2017
Merged

Flannel: change default backend type #3190

merged 2 commits into from
Sep 13, 2017

Conversation

justinsb
Copy link
Member

@justinsb justinsb commented Aug 12, 2017 •
edited
Loading

We support udp, which has to the default for backwards-compatibility,
but also new clusters will now use vxlan.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 12, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2017
@justinsb justinsb force-pushed the flannel_vlxan branch 4 times, most recently from b79e000 to cf9abce Compare August 14, 2017 11:39
@justinsb justinsb mentioned this pull request Aug 14, 2017
Closed
@justinsb justinsb changed the title WIP: Flannel: change default backend type Flannel: change default backend type Aug 15, 2017
chrislovecnm
chrislovecnm suggested changes Aug 18, 2017
View reviewed changes
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.

Please update the flannel documentation, and I have a question about the warning message.

Thanks for the PR!

upup/pkg/fi/cloudup/template_functions.go
if tf.cluster.Spec.Networking != nil && tf.cluster.Spec.Networking.Flannel != nil {
flannelBackendType := tf.cluster.Spec.Networking.Flannel.Backend
if flannelBackendType == "" {
glog.Warningf("Defaulting flannel backend to udp (not a recommended configuration)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we tell the user what to do? You need to set this, or you need to migrate to this? We are warning them, but not helping them on what to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a release note. The docs we have for networking are not terribly instructive at the moment... that might have to be another PR though.

Copy link

Choose a reason for hiding this comment

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

Upgrade path I followed today on a running cluster. Be aware that this creates downtime/connectivity issues ~1 minute. Steps followed:

  1. kops edit cluster <- change udp for vxlan
  2. kubectl edit configmap -n kube-system kube-flannel-cfg <- change udp for vxlan
  3. kubectl delete pod -n kube-system -l app=flannel

chrislovecnm
chrislovecnm reviewed Aug 18, 2017
View reviewed changes
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

pkg/apis/kops/networking.go Outdated
@@ -60,6 +60,8 @@ type WeaveNetworkingSpec struct {

// Flannel declares that we want Flannel networking
type FlannelNetworkingSpec struct {
// Backend is the backend overlay type we want to use
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the possible backend values? Should we have them in the comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added values to comment

@justinsb
Copy link
Member Author

Comments addressed - PTAL

@k8s-github-robot
Copy link

@justinsb PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2017
@justinsb
Copy link
Member Author

/retest

@justinsb justinsb force-pushed the flannel_vlxan branch 2 times, most recently from c9fc9a5 to a4cbe67 Compare August 24, 2017 19:26
@justinsb
Copy link
Member Author

Passing tests - PTAL

@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 Aug 30, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, 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:
  • OWNERS [chrislovecnm,justinsb]

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
Copy link

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

@k8s-github-robot
Copy link

@justinsb PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2017
@justinsb
Flannel: support choosing a backend type
15d6834 
We support udp, which has to the default for backwards-compatibility,
but also new clusters will now use vxlan.
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 31, 2017
@k8s-github-robot
Copy link

/lgtm cancel //PR changed after LGTM, removing LGTM. @chrislovecnm @justinsb

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 31, 2017
@justinsb
Copy link
Member Author

/retest

@justinsb
Copy link
Member Author

Reapplying LGTM after rebase

@justinsb justinsb added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 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 26e1cb0 into kubernetes:master Sep 13, 2017
@TattiQ TattiQ mentioned this pull request Aug 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrislovecnm chrislovecnm chrislovecnm requested changes

@mstuparu mstuparu mstuparu left review comments

Assignees

@chrislovecnm chrislovecnm

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. needs-review size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@justinsb @k8s-github-robot @chrislovecnm @mstuparu @k8s-ci-robot