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

fix(chart): set ttlSecondsAfterFinished on certgen job to 30 by default #3156

Merged
merged 1 commit into from
Apr 16, 2024
Merged

fix(chart): set ttlSecondsAfterFinished on certgen job to 30 by default #3156

merged 1 commit into from
Apr 16, 2024

Conversation

JuniorJPDJ
Copy link
Contributor

This is painless workaround for ArgoCD bug: argoproj/argo-cd#6880

@JuniorJPDJ JuniorJPDJ requested a review from a team as a code owner April 9, 2024 15:57
zhaohuabing
zhaohuabing previously approved these changes Apr 12, 2024
Copy link
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

@JuniorJPDJ Thanks for coming up with the workaround for using EG with ArgoCD. Is it because ArgoCD requires the job to exist for a while after it is finished?

@arkodg
Copy link
Contributor

arkodg commented Apr 12, 2024

thanks for raising this ! I think 30s also works here.
weighing the pros and cons of setting this as a default - I believe the con of this is slower clean up (pressure on the k8s API server) ?

@JuniorJPDJ
Copy link
Contributor Author

@zhaohuabing probably. It's not specified anywhere in the ArgoCD docs, but ArgoCD freezes forever on sync phase when job is deleted too quickly.

60s also allows to read logs without much timing pressure ;)

@arkodg you are probably right, but I'm not sure if it creates any pressure, and if it does, its so small and done only when upgrading/installing the chart, as it's just a hook

@arkodg
Copy link
Contributor

arkodg commented Apr 12, 2024

@zhaohuabing probably. It's not specified anywhere in the ArgoCD docs, but ArgoCD freezes forever on sync phase when job is deleted too quickly.

60s also allows to read logs without much timing pressure ;)

@arkodg you are probably right, but I'm not sure if it creates any pressure, and if it does, its so small and done only when upgrading/installing the chart, as it's just a hook

cool, lets get this in, can we reduce this to 30 ? have you noticed cases where this is not enough, afaik 30s seems to work for users here

@JuniorJPDJ
Copy link
Contributor Author

I've not seen any problems, but let me try in my setup again first.
Give me a few minutes :D

@JuniorJPDJ JuniorJPDJ changed the title fix(chart): set ttlSecondsAfterFinished on certgen job to 60 by default fix(chart): set ttlSecondsAfterFinished on certgen job to 30 by default Apr 12, 2024
@JuniorJPDJ
Copy link
Contributor Author

It worked without any problems - ready to merge. I already changed value, commit message and the PR name.

@arkodg
Copy link
Contributor

arkodg commented Apr 15, 2024

@JuniorJPDJ can you run make generate and commit the changes

This is painless workaround for ArgoCD bug:
argoproj/argo-cd#6880

Signed-off-by: JuniorJPDJ <git@juniorjpdj.pl>
@JuniorJPDJ
Copy link
Contributor Author

rebased and make generated

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested a review from zhaohuabing April 15, 2024 12:02
@zirain
Copy link
Contributor

zirain commented Apr 15, 2024

/retest

@arkodg arkodg merged commit e537033 into envoyproxy:main Apr 16, 2024
19 of 20 checks passed
@JuniorJPDJ JuniorJPDJ deleted the patch-2 branch April 16, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants