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

clustermesh-apiserver deployment: support lifecycle and terminationGr… #26945

Merged
merged 1 commit into from Jul 31, 2023
Merged

clustermesh-apiserver deployment: support lifecycle and terminationGr… #26945

merged 1 commit into from Jul 31, 2023

Conversation

acgs771126
Copy link

clustermesh-apiserver deployment: support lifecycle and terminationGracePeriodSeconds

clustermesh-apiserver deployment support lifecycle and terminationGracePeriodSeconds, because we need a while time(by lifecycle.preStop hook) to wait AWS NLB draining target for prevent connection issue when clustermesh-apiserver pod terminating

Please note: this change I hope can backported to 1.13 version

Fixes: #26900

clustermesh-apiserver deployment support lifecycle and terminationGracePeriodSeconds.

@acgs771126 acgs771126 requested review from a team as code owners July 20, 2023 06:22
@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 Jul 20, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jul 20, 2023
@giorio94 giorio94 self-requested a review July 20, 2023 07:50
@giorio94
Copy link
Member

Related discussion in #26900.

kaworu
kaworu previously requested changes Jul 20, 2023
View reviewed changes
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @acgs771126!

CI failed with:

HINT: to fix this, run 'make -C Documentation update-helm-values'

Also please first report the changes from install/kubernetes/cilium/values.yaml to install/kubernetes/cilium/values.yaml.tmpl instead and then run:

make -C install/kubernetes cilium/values.yaml

install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved
@kaworu kaworu added kind/feature This introduces new functionality. area/clustermesh Relates to multi-cluster routing functionality in Cilium. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jul 20, 2023
@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 Jul 20, 2023
giorio94
giorio94 requested changes Jul 21, 2023
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks @acgs771126 for the PR!

One minor request inline on top of the comments from Alexandre.

install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved
@acgs771126 acgs771126 force-pushed the pr/acgs771126/clustermesh-deployment-support-lifecycle-and-terminationGracePeriodSeconds branch from b9c4f66 to 7a9fe62 Compare July 21, 2023 16:12
@acgs771126 acgs771126 force-pushed the pr/acgs771126/clustermesh-deployment-support-lifecycle-and-terminationGracePeriodSeconds branch from 7a9fe62 to 2a7e710 Compare July 24, 2023 06:45
giorio94
giorio94 requested changes Jul 24, 2023
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment •
edited

Choose a reason for hiding this comment

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

Changes look good to me.

CI is failing with the following errors, which shall be fixed:

=========================================================
[1/1] Running on 6f0006779a9ecc757e06931cbe5facee053c0e43
clustermesh-apiserver deployment: support lifecycle and terminationGracePeriodSeconds
=========================================================
ERROR:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'andychuang <andy.chuang@shoplineapp.com>'

"[PATCH] clustermesh-apiserver deployment: support lifecycle and" has style problems, please review.

ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 85)

Similarly, please also rerun make -C install/kubernetes and commit the changes (mending the current commit).

(Edit) You should also run Documentation/update-spelling_wordlist.sh terminationGracePeriodSeconds and commit the changes.

@acgs771126 acgs771126 force-pushed the pr/acgs771126/clustermesh-deployment-support-lifecycle-and-terminationGracePeriodSeconds branch from 2a7e710 to b54244e Compare July 24, 2023 07:16
@acgs771126 acgs771126 requested a review from a team as a code owner July 24, 2023 07:16
@acgs771126
Copy link
Author

acgs771126 commented Jul 24, 2023 •
edited

@giorio94 seems resolved all CI failure, please help review the PR again, many thanks.

@giorio94
Copy link
Member

@giorio94 seems resolved all CI failure, please help review the PR again, many thanks.

One of the checks is still failing because the commit title is too long: https://github.com/cilium/cilium/actions/runs/5641947820/job/15282283916?pr=26945

@acgs771126 acgs771126 force-pushed the pr/acgs771126/clustermesh-deployment-support-lifecycle-and-terminationGracePeriodSeconds branch from b54244e to a4ed903 Compare July 24, 2023 08:45
@acgs771126
Copy link
Author

acgs771126 commented Jul 24, 2023 •
edited

One of the checks is still failing because the commit title is too long: https://github.com/cilium/cilium/actions/runs/5641947820/job/15282283916?pr=26945

@giorio94
updated the PR, but got Travis-CI failed...
seems like in ipam testing step, but I don't update any about ipam changes, so maybe you can help re-run the CI testing? or any suggestions.

@giorio94
Copy link
Member

/test

@giorio94
Copy link
Member

@acgs771126 Tests are not happy yet :-(. Could you please rebase your branch onto main?

zacharysarah
zacharysarah requested changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@acgs771126 Some grammar nits, otherwise LGTM from a docs perspective.

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved
install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved
install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved
@acgs771126 acgs771126 force-pushed the pr/acgs771126/clustermesh-deployment-support-lifecycle-and-terminationGracePeriodSeconds branch from a4ed903 to deac4e2 Compare July 25, 2023 02:51
helm: apiserver support lifecycle and terminationGracePeriodSeconds
ddf76d1 
clustermesh-apiserver deploment support set lifecycle and terminationGracePeriodSeconds, because we need a while time(by lifecycle.preStop hook) to wait AWS NLB draining target for prevent connection issue when clustermesh-apiserver pod terminating

Fixes: cilium#26900

Signed-off-by: andychuang <andy.chuang@shoplineapp.com>
@acgs771126 acgs771126 force-pushed the pr/acgs771126/clustermesh-deployment-support-lifecycle-and-terminationGracePeriodSeconds branch from deac4e2 to ddf76d1 Compare July 25, 2023 02:56
@giorio94
Copy link
Member

/test

@acgs771126
Copy link
Author

@acgs771126 Some grammar nits, otherwise LGTM from a docs perspective.

@zacharysarah updated the PR, please help review again, thanks :)

zacharysarah
zacharysarah approved these changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

LGTM

@acgs771126
Copy link
Author

@kaworu Hi Alexandre
Sorry to trouble you, hope you have a good vacation.
If your come back to work please help review the PR, thank you :)

@giorio94
Copy link
Member

/test

tommyp1ckles
tommyp1ckles approved these changes Jul 29, 2023
View reviewed changes
Copy link
Contributor

@tommyp1ckles tommyp1ckles left a comment

Choose a reason for hiding this comment

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

looks good

@giorio94 giorio94 dismissed kaworu’s stale review July 31, 2023 06:51

Comments have been addressed

@giorio94
Copy link
Member

Reviews are in, and CI is green. Marking as ready to merge.

@giorio94 giorio94 added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 31, 2023
@dylandreimerink dylandreimerink merged commit 47a6bd0 into cilium:main Jul 31, 2023
69 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zacharysarah zacharysarah zacharysarah approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

@giorio94 giorio94 giorio94 approved these changes

@kaworu kaworu Awaiting requested review from kaworu

Assignees
No one assigned
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. kind/community-contribution This was a contribution made by a community member. kind/feature This introduces new functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CFP: clustermesh-apiserver deployment support lifecycle and terminationGracePeriodSeconds
6 participants
@acgs771126 @giorio94 @kaworu @zacharysarah @tommyp1ckles @dylandreimerink