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

Enhance liveness probes for manila-operator #49

Merged

Conversation

silvacarloss
Copy link
Contributor

The manila operator needs to do checks on services status to ensure they are actually up. This change enhances the liveness probes for the manila-share and manila-scheduler services by implementing a HTTP probe using a custom HTTP server. The HTTP server will take advantage of the manila-manage service list command, which is an interface manila provides to get the services that are current registered in the database, then allowing the probe to check the services actual state.

@openshift-ci openshift-ci bot requested review from dprince and viroel April 5, 2023 17:25
Copy link
Contributor

@gouthampacha gouthampacha left a comment

Choose a reason for hiding this comment

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

Thanks @silvacarloss. Please see comments inline

templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
@silvacarloss silvacarloss force-pushed the enhance-probes branch 2 times, most recently from 3bef483 to 90275d7 Compare June 30, 2023 11:47
@silvacarloss silvacarloss force-pushed the enhance-probes branch 3 times, most recently from ab8bf90 to 2b919ce Compare July 12, 2023 12:07
@silvacarloss
Copy link
Contributor Author

/test manila-operator-build-deploy-tempest

@gouthampacha
Copy link
Contributor

/test manila-operator-build-deploy-tempest

think we need to harden the ceph deployment a bit

@silvacarloss silvacarloss force-pushed the enhance-probes branch 3 times, most recently from 12f8127 to 549fd85 Compare September 27, 2023 16:23
@silvacarloss silvacarloss marked this pull request as draft September 27, 2023 16:23
@silvacarloss silvacarloss force-pushed the enhance-probes branch 2 times, most recently from a113688 to 7fbe449 Compare September 27, 2023 21:11
Copy link
Contributor

@gouthampacha gouthampacha left a comment

Choose a reason for hiding this comment

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

o/ carlos - just some thoughts inline

templates/manila/bin/healthcheck.py Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@silvacarloss silvacarloss 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 review, please take a look at the most recent changes

templates/manila/bin/healthcheck.py Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
@silvacarloss silvacarloss force-pushed the enhance-probes branch 4 times, most recently from f1de144 to 2b71578 Compare December 8, 2023 14:42
@silvacarloss silvacarloss force-pushed the enhance-probes branch 3 times, most recently from dda7261 to c44cbe0 Compare December 8, 2023 21:12
@silvacarloss
Copy link
Contributor Author

/retest

@silvacarloss
Copy link
Contributor Author

The kuttl job isn't logging to stdout, and all debug messages are being picked up when we do manila-manage service list. That made me add a workaround to remove the DEBUG info from the manila-manage command output, and I think it could still be relevant, considering that we must treat it in case a customer wants to enable debugging and we don't want the code to break.
The code will look cleaner when the enhancement for manila-manage output format merges upstream.

Copy link
Contributor

@gouthampacha gouthampacha left a comment

Choose a reason for hiding this comment

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

Thanks @silvacarloss ; this looks good.. some minor comments inline.

pkg/manilashare/statefulset.go Outdated Show resolved Hide resolved
templates/manila/bin/healthcheck.py Outdated Show resolved Hide resolved
Manila-operator is currently doing limited actions when it comes
to doing healthchecks. This change enhances the probes so we can
do actual checks on the status of the manila services, by
implementing a simple HTTP server (as being done by the
cinder operator)

The HTTP server though will do the healthchecks using a command we
offer in Manila: manila-manage service list. The HTTP server will
take as arguments the type of service you are trying to ensure is
alive (share or scheduler) and the location of the manila
configuration file.

Manila-manage service list command does not require any kind of
additional client to be run, nor keystone tokens or additional
authentication steps are required. The command will simply get the
services that are already registered in the database.

Then, the HTTP server will check on the state of the services and
respond to the operator according to the state and filters
matching.
@silvacarloss
Copy link
Contributor Author

/retest

@silvacarloss silvacarloss marked this pull request as ready for review December 11, 2023 13:00
@openshift-ci openshift-ci bot requested a review from stuggi December 11, 2023 13:00
# configuration directory (defaults to /etc/manila/manila.conf.d)

from http import server
import logging
Copy link
Contributor

Choose a reason for hiding this comment

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

unused import

Copy link
Contributor

@gouthampacha gouthampacha left a comment

Choose a reason for hiding this comment

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

Thanks @silvacarloss
a minor comment inline; you can drop the unused import in a follow up patch

@openshift-ci openshift-ci bot added the lgtm label Dec 12, 2023
Copy link
Contributor

openshift-ci bot commented Dec 12, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gouthampacha, silvacarloss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 5e9945a into openstack-k8s-operators:main Dec 12, 2023
9 checks passed
openshift-merge-bot bot added a commit that referenced this pull request Jan 16, 2024
Follow-up for pull request #49 (healthchecks)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants