Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add healthz liveness probe #2936

Merged
merged 10 commits into from
May 8, 2020
Merged

Conversation

asmacdo
Copy link
Member

@asmacdo asmacdo commented Apr 28, 2020

Sets up a Ping healthz endpoint (ping)
Adds a liveness probe to ansible operator deployment scaffolding

@asmacdo asmacdo added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2020
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Just few nits which are :

  • Add fragment
  • Add comments to clarifies how/why the tests are made

Otherwise, it is :

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 7, 2020
but to take advantage of it, a liveness probe should be manually added
to the operator manifest. See the
[test operator](https://github.com/operator-framework/operator-sdk/blob/master/test/ansible/deploy/operator.yaml)
for an example.
Copy link
Contributor

Choose a reason for hiding this comment

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

By following the doc standard it would be as;

    migration:
      header: **(Optional for Ansible-base operators)**  Add livenessProbe check
      body: >
        Existing operators will have a healthz endpoint without intervention,
        but to take advantage of it, a liveness probe should be manually added
        to the operator manifest. Add the livenessProbe as the following example in the`/deploy/operator.ymal`

        ```ymal
          livenessProbe:
            httpGet:
              path: /healthz
              port: 6789
            initialDelaySeconds: 5
            periodSeconds: 3

        ```

@asmacdo asmacdo removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2020
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2020
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm
based on previous discussions and quick re-review.

Comment on lines +111 to +118
live_pod=$(kubectl get pod -l name=memcached-operator -o jsonpath="{..metadata.name}")
if kubectl get events --field-selector involvedObject.name=$live_pod | grep Killing
then
error_text "FAIL: Operator pod killed due to failed liveness probe."
kubectl get events --field-selector involvedObject.name=$live_pod,reason=Killing
operator_logs
exit 1
fi
Copy link
Member

Choose a reason for hiding this comment

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

Nice. I remember this discussion :) Looks like it worked.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants