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

Update Kubernetes Manifests to comment out liveness probe #67080

Merged
merged 2 commits into from
Jul 1, 2021

Conversation

keith-mcclellan
Copy link
Contributor

Liveness probe can cause kubernetes to reboot nodes when the database is busy but functioning properly, so we are commenting out that probe.

Liveness probe can cause kubernetes to reboot nodes when the database is busy but functioning properly, so we are commenting out that probe.
@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Jun 30, 2021

CLA assistant check
All committers have signed the CLA.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@taroface taroface left a comment

Choose a reason for hiding this comment

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

Just a phrasing suggestion!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto and @keith-mcclellan)


cloud/kubernetes/cockroachdb-statefulset.yaml, line 118 at r1 (raw file):

        - containerPort: 8080
          name: http
# We recommend that users don't configure liveness probes for production environments as it can impact availability of production databases.

I suggest this wording in all the manifests:

We recommend that you do not configure a liveness probe on a production environment, as this can impact the availability of production databases.

Do we want to add something like "A liveness probe can be used to verify the health of a test deployment." (if that is true)

I also suggest indenting this at the same level as livenessProbe to be consistent with the other comments.

@keith-mcclellan
Copy link
Contributor Author

Just a phrasing suggestion!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto and @keith-mcclellan)

cloud/kubernetes/cockroachdb-statefulset.yaml, line 118 at r1 (raw file):

        - containerPort: 8080
          name: http
# We recommend that users don't configure liveness probes for production environments as it can impact availability of production databases.

I suggest this wording in all the manifests:

We recommend that you do not configure a liveness probe on a production environment, as this can impact the availability of production databases.

Do we want to add something like "A liveness probe can be used to verify the health of a test deployment." (if that is true)

I also suggest indenting this at the same level as livenessProbe to be consistent with the other comments.

Happy to change the wording. The second one isn't really true - liveness probes are good for "quiet" environments but noisy ones they're bad. This is something we'd probably want to expound on in the docs rather than a yaml comment. What do you think @taroface ?

@taroface
Copy link
Collaborator

Sounds good. I'll create a separate ticket to document the use of livenessProbe in e.g. the K8s monitoring doc.

changed content description and updated commented out format to be in line with the rest of the yaml comments
@keith-mcclellan
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 1, 2021

🔒 Permission denied

Existing reviewers: click here to make keith-mcclellan a reviewer

@taroface
Copy link
Collaborator

taroface commented Jul 1, 2021

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 1, 2021

Build succeeded:

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.

3 participants