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

Add startupProbe for Cilium-agent #14518

Merged
merged 1 commit into from
Jan 8, 2021
Merged

Add startupProbe for Cilium-agent #14518

merged 1 commit into from
Jan 8, 2021

Conversation

youssefazrak
Copy link
Contributor

@youssefazrak youssefazrak commented Jan 4, 2021 •
edited by aanm

As of K8s 1.20, a new feature called startupProbe has been added.
This allows getting better startup times especially for slow starting
containers.

Fixes: #14342

Signed-off-by: Youssef Azrak yazrak.tech@gmail.com

Add startupProbe for Cilium-agent for faster readiness in Kubernetes >= 1.20

@youssefazrak youssefazrak requested review from a team as code owners January 4, 2021 23:38
@youssefazrak youssefazrak requested review from a team, qmonnet, kkourt and brb January 4, 2021 23:38
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 4, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Jan 4, 2021
@youssefazrak
Copy link
Contributor Author

youssefazrak commented Jan 4, 2021 •
edited

@christarazi let me know if anything else is needed.

Btw I didn't touch install/kubernetes/quick-install.yaml and install/kubernetes/experimental-install.yaml as this features is only available starting from k8s 1.20.

Also, as I have no "real-life" data of what could be a good timer, I fixed it to an arbitrary 300s (5 minutes) for the startupProbe. On the other hand, I removed the initialDelaySeconds for the livenessProbe.

Regarding the startupProbe:
Offical docs
KEP

sayboras
sayboras reviewed Jan 5, 2021
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Few comments as per below.

I am wondering if the same can be done for preflight and hubble pods as well ? :thinking"

install/kubernetes/cilium/templates/cilium-agent-daemonset.yaml Outdated Show resolved Hide resolved
install/kubernetes/cilium/templates/cilium-agent-daemonset.yaml Show resolved Hide resolved
install/kubernetes/cilium/templates/cilium-agent-daemonset.yaml Outdated Show resolved Hide resolved
@sayboras sayboras added area/helm Impacts helm charts and user deployment experience release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jan 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 5, 2021
@sayboras
Copy link
Member

sayboras commented Jan 5, 2021

Also, as I have no "real-life" data of what could be a good timer, I fixed it to an arbitrary 300s (5 minutes) for the startupProbe. On the other hand, I removed the initialDelaySeconds for the livenessProbe.

Hope you don't mind if I chime in.

Currently, I think that as long as the value is greater or equal to current initialDelaySeconds of livenessProbe, it should be fine :).

@youssefazrak
Copy link
Contributor Author

@sayboras Thanks for the review! I addressed the points.
The only thing left is the Hubble and Preflights (what are those?) pods. Well, if they have the same behavior as the agent, I guess it would make sense to introduce this startupProbe too.

@sayboras
Copy link
Member

sayboras commented Jan 6, 2021

The only thing left is the Hubble and Preflights (what are those?) pods. Well, if they have the same behavior as the agent, I guess it would make sense to introduce this startupProbe too.

I think the delay is not that much for hubble or preflight, this can be addressed in separate PR as well. Feel free to remove WIP in PR description once you are ready. The current changes are LGTM 💯

@youssefazrak youssefazrak changed the title WIP - Add startupProbe for Cilium-agent Add startupProbe for Cilium-agent Jan 6, 2021
@aanm
Copy link
Member

aanm commented Jan 6, 2021

test-me-please

fristonio
fristonio approved these changes Jan 6, 2021
View reviewed changes
Copy link
Member

@fristonio fristonio left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

christarazi
christarazi approved these changes Jan 6, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM. Minor non-blocking nit

install/kubernetes/cilium/templates/cilium-agent-daemonset.yaml Outdated
Comment on lines 132 to 133
# Starting from Kubernetes 1.20, we are using startupProbe instead
# of this field
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Starting from Kubernetes 1.20, we are using startupProbe instead
# of this field
# Starting from Kubernetes 1.20, we are using startupProbe instead
# of this field.

Just to match with the rest of the formatting in the comment. A few occurrences of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, fixed now.

sayboras
sayboras approved these changes Jan 7, 2021
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@youssefazrak
Add startupProbe for Cilium-agent
e2d1038 
As of K8s 1.20, a new feature called startupProbe has been added.
This allows to get better startup times especially for slow starting
containers.

Fixes cilium#14342

Signed-off-by: Youssef Azrak <yazrak.tech@gmail.com>
@aanm
Copy link
Member

aanm commented Jan 7, 2021

test-me-please

@aanm aanm unassigned brb, kkourt and qmonnet Jan 7, 2021
@aanm aanm removed request for brb and kkourt January 7, 2021 12:21
@qmonnet
Copy link
Member

qmonnet commented Jan 7, 2021

test-gke

1 similar comment
@qmonnet
Copy link
Member

qmonnet commented Jan 7, 2021

test-gke

@qmonnet
Copy link
Member

qmonnet commented Jan 7, 2021 •
edited

retest-net-next
(hit #13275)

@aanm aanm merged commit 173b1bc into cilium:master Jan 8, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 8, 2021
@youssefazrak youssefazrak deleted the pod_startup branch January 8, 2021 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi approved these changes

@aanm aanm aanm approved these changes

@sayboras sayboras sayboras approved these changes

@qmonnet qmonnet qmonnet approved these changes

@fristonio fristonio fristonio approved these changes

Assignees

@qmonnet qmonnet

Labels
area/helm Impacts helm charts and user deployment experience ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pod startup liveness-probe hold-off
8 participants
@youssefazrak @sayboras @aanm @qmonnet @christarazi @fristonio @brb @kkourt