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

Ensure manifests always auto-detect BGP IP addresses #1588

Merged
merged 2 commits into from
Jan 12, 2018

Conversation

caseydavenport
Copy link
Member

@caseydavenport caseydavenport commented Jan 9, 2018

Description

Fixes #1470

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Kubernetes self-hosted manifests enabled BGP IP address auto-detection by default

@caseydavenport caseydavenport added the release-note-required Change has user-facing impact (no matter how small) label Jan 9, 2018
@projectcalico projectcalico deleted a comment from CLAassistant Jan 9, 2018
@projectcalico projectcalico deleted a comment from CLAassistant Jan 9, 2018
@caseydavenport caseydavenport requested a review from fasaxc January 9, 2018 15:24
@caseydavenport
Copy link
Member Author

I think we want to port this back to v3.0 and potentially even v2.6 as well. Input welcome :)

@caseydavenport caseydavenport added this to the Calico v3.0.2 milestone Jan 9, 2018
@fasaxc
Copy link
Member

fasaxc commented Jan 10, 2018

Only thought is that autodetect exacerbates #1471, so maybe we need to only backport if we also backport the deadlock fix?

@@ -243,9 +243,6 @@ spec:
valueFrom:
fieldRef:
fieldPath: spec.nodeName
# No IP address needed.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use autodetect here? If we don't then we won't benefit from the fix in #1589 when IPs change (since we deliberately exclude the IP="" case in that patch). Looks like, if IP is not set then we do a single autodetection anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, this is the policy-only manifest, which shouldn't care about the BGP IP address anyway.

The code in #1589 won't even get hit because we set CALICO_NETWORKING_BACKEND=none in this manifest.

Copy link
Member

Choose a reason for hiding this comment

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

Ah OK, that makes sense, was a bit tunnel-visioned on the code I was reviewing.

@caseydavenport
Copy link
Member Author

@fasaxc it doesn't exacerbate it really, just makes it more visible to the user, right?

Without this, IPs will change and nodes will try to bind to the wrong (old) one. With this, IPs will change and calico will notice, but be unable to set the new IP.

I think this should go into master and v3.0, and the fix will go into v3.0.2

@fasaxc
Copy link
Member

fasaxc commented Jan 11, 2018

I guess that makes sense, would be nice to have the fix available though if we're going to make the problem more visible.

@caseydavenport caseydavenport merged commit 129a3fa into projectcalico:master Jan 12, 2018
@caseydavenport caseydavenport deleted the fix-autodetect branch January 12, 2018 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-required Change has user-facing impact (no matter how small)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

master manifests not configured for IP autodetection
2 participants