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

Improve pod restarts on GKE #10377

Merged
merged 1 commit into from
Mar 4, 2020
Merged

Conversation

ap4y
Copy link
Contributor

@ap4y ap4y commented Feb 28, 2020

Pod restart from node init supposed to happen once kubelet is
reconfigured, but order is opposite in the helm chart. It's also
unnecessary to hold off cilium agent pod till pods are restarted. This
patch moves pod restart to the last operation in node init. This patch
also fixes typo in the GKE docs: 'global.restartPods' ->
'nodeinit.restartPods'.


This change is Reviewable

Pod restart from node init supposed to happen once kubelet is
reconfigured, but order is opposite in the helm chart. It's also
unnecessary to hold off cilium agent pod till pods are restarted. This
patch moves pod restart to the last operation in node init. This patch
also fixes typo in the GKE docs: 'global.restartPods' ->
'nodeinit.restartPods'.

Signed-off-by: Arthur Evstifeev <aevstifeev@gitlab.com>
@ap4y ap4y requested a review from a team as a code owner February 28, 2020 02:31
@ap4y ap4y requested a review from a team February 28, 2020 02:31
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 45.541% when pulling 49b9ebb on ap4y:fix-gke-restart-pods into 64c4792 on cilium:master.

@joestringer joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Feb 28, 2020
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

That's a good point. @raybejjani @nebril are we using this file in gke testing? If yes should we trigger a CI run for GKE?

@aanm
Copy link
Member

aanm commented Feb 28, 2020

test-me-please

@tgraf
Copy link
Member

tgraf commented Feb 28, 2020

test-gke

@joestringer
Copy link
Member

@ap4y
Copy link
Contributor Author

ap4y commented Mar 1, 2020

nodeinit.restartPods is not used in the tests it seems: https://github.com/cilium/cilium/search?q=restartPods&unscoped_q=restartPods . I have manually tested this patch against GKE cluster and it worked as expected.

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Docs changes look right.

@joestringer joestringer merged commit 75749e9 into cilium:master Mar 4, 2020
1.8.0 automation moved this from In progress to Merged Mar 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.1 Mar 4, 2020
@joestringer joestringer removed this from Needs backport from master in 1.7.1 Mar 5, 2020
@joestringer joestringer added this to Needs backport from master in 1.7.2 Mar 5, 2020
@tklauser tklauser moved this from Needs backport from master to Backport done to v1.7 in 1.7.2 Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.7.2
Backport done to v1.7
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants