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

Switch etcd mode to use an init container for CNI #2300

Merged
merged 1 commit into from
Nov 19, 2018
Merged

Switch etcd mode to use an init container for CNI #2300

merged 1 commit into from
Nov 19, 2018

Conversation

caseydavenport
Copy link
Member

@caseydavenport caseydavenport commented Nov 18, 2018 •
edited
Loading

Description

We did this for KDD a while back and it seems to be working well. Let's do it for etcd in the same release.

#2231

Todos

  • Tests
  • Documentation
  • Release note

Release Note

etcd manifests now install CNI plugins and config using an init container

@caseydavenport caseydavenport requested a review from a team as a code owner November 18, 2018 02:43
@CLAassistant
Copy link

CLAassistant commented Nov 18, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@caseydavenport
Copy link
Member Author

caseydavenport commented Nov 18, 2018 •
edited
Loading

Deploy preview for calico ready!

Built with commit 567d982

https://deploy-preview-2300--calico.netlify.com

@nelljerram nelljerram changed the title Switch etcd mode to use an init contianer for CNI Switch etcd mode to use an init container for CNI Nov 19, 2018
nelljerram
nelljerram approved these changes Nov 19, 2018
View reviewed changes
Copy link
Member

@nelljerram nelljerram left a comment

Choose a reason for hiding this comment

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

LGTM. I'm not really a manifest expert, but I did a fairly careful check against what we had before for etcd CNI setup as a non-init container, and I don't think anything has been lost.

@fasaxc
Copy link
Member

fasaxc commented Nov 19, 2018

ISTR that we used to keep the container running so it monitored the etcd certs, or something? What are we losing here?

@caseydavenport
Copy link
Member Author

@fasaxc yeah, we're losing that bit of logic where if the cert changed we'd copy it across to the host.

I don't think it's useful - you need to restart felix / confd / etc anyway if the certs change, so my thinking was you'll need to restart this pod anyway.

WDYT?

@caseydavenport caseydavenport added the release-note-required Change has user-facing impact (no matter how small) label Nov 19, 2018
@caseydavenport caseydavenport added this to the Calico v3.4.0 milestone Nov 19, 2018
@caseydavenport caseydavenport merged commit a4fa5b3 into projectcalico:master Nov 19, 2018
@caseydavenport caseydavenport deleted the init-container branch November 19, 2018 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nelljerram nelljerram nelljerram approved these changes

@fasaxc fasaxc Awaiting requested review from fasaxc

Assignees
No one assigned
Labels
release-note-required Change has user-facing impact (no matter how small)
Projects
None yet
Milestone
Calico v3.4.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@caseydavenport @CLAassistant @fasaxc @nelljerram