Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Give time for injector to come up #885

Merged
merged 3 commits into from
Mar 30, 2021
Merged

Give time for injector to come up #885

merged 3 commits into from
Mar 30, 2021

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Mar 25, 2021

Getting certificates can take a couple of seconds

Fixes #610, may fix hashicorp/consul-k8s#451

How I've tested this PR:

  • installed on kind and saw how container would be ready in ~10s

How I expect reviewers to test this PR:

  • code, can test locally as well

Checklist:

  • Bats tests added (not applicable)
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

I think that the ideal fix here is to implement a startup probe and possibly leave the existing probes as is, or loosen them just a little. What are your thoughts?
I think this will have an added effect of delaying connect's startup for 30s .. which is possibly 29s too long if this issue only creeps up in certain environments.
What do you think?

@lkysow
Copy link
Member Author

lkysow commented Mar 25, 2021

I think that the ideal fix here is to implement a startup probe and possibly leave the existing probes as is, or loosen them just a little. What are your thoughts?
I think this will have an added effect of delaying connect's startup for 30s .. which is possibly 29s too long if this issue only creeps up in certain environments.
What do you think?

Oh you're totally right, will fix!!

@lkysow lkysow requested a review from kschoche March 25, 2021 19:43
@lkysow
Copy link
Member Author

lkysow commented Mar 25, 2021

@kschoche updated!

BUG FIXES:
* Add startup probe to connect-inject deployment to give time for certificates to be available.
Previously, the deployment could be killed by Kubernetes and crash loop because certificates would take a couple
of seconds. [[GH-885](https://github.com/hashicorp/consul-helm/pull/885)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
of seconds. [[GH-885](https://github.com/hashicorp/consul-helm/pull/885)]
of seconds to be available. [[GH-885](https://github.com/hashicorp/consul-helm/pull/885)]

Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

Looks great!! 🥇

@lkysow lkysow force-pushed the inject-liveness-probes branch from cc41c8c to c492ecc Compare March 25, 2021 20:20
@ndhanushkodi ndhanushkodi self-requested a review March 30, 2021 17:16
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

🎉 Great find to use the startup probe @lkysow and @kschoche!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants