🌱 Add health probes to test-operator bundles #2311
Conversation
Enhance test-operator v1.0.0 and v1.2.0 bundles with HTTP-based health probes
to enable testing of startup, liveness, and readiness probe behavior. Replaces
the simple sleep container with a busybox httpd server that exposes probe
endpoints via a ConfigMap-mounted script.
The probes can be flipped from true to false by executing `rm /var/www{stared,ready,live}`
and back to true by performin `touch /var/www{stared,ready,live}`
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the test operator bundles (v1.0.0 and v1.2.0) by replacing a simple sleep-based busybox container with an httpd-based container that includes health probes. The changes add ConfigMaps with startup scripts and configure Kubernetes health checks for better container lifecycle management.
- Replaces the sleep command with an httpd server implementation
- Adds startup, liveness, and readiness probes with HTTP endpoints
- Introduces ConfigMaps to mount executable scripts into containers
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| testdata/images/bundles/test-operator/v1.2.0/manifests/testoperator.clusterserviceversion.yaml | Updates container configuration with httpd server, health probes, and volume mounts for v1.2.0 |
| testdata/images/bundles/test-operator/v1.2.0/manifests/script.configmap.yaml | Adds ConfigMap with httpd startup script for v1.2.0 |
| testdata/images/bundles/test-operator/v1.0.0/manifests/testoperator.clusterserviceversion.yaml | Updates container configuration with httpd server, health probes, and volume mounts for v1.0.0 |
| testdata/images/bundles/test-operator/v1.0.0/manifests/script.configmap.yaml | Adds ConfigMap with httpd startup script for v1.0.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| port: 80 | ||
| initialDelaySeconds: 1 | ||
| periodSeconds: 1 | ||
| serviceAccountName: simple-bundle-manager |
There was a problem hiding this comment.
The serviceAccountName field is incorrectly placed inside the container spec. It should be at the pod spec level (after the containers array, not inside it). This will cause the deployment to fail.
| port: 80 | ||
| initialDelaySeconds: 1 | ||
| periodSeconds: 1 | ||
| serviceAccountName: simple-bundle-manager |
There was a problem hiding this comment.
The serviceAccountName field is incorrectly placed inside the container spec. It should be at the pod spec level (after the containers array, not inside it). This will cause the deployment to fail.
There was a problem hiding this comment.
Incorrect, all e2e tests are passing.
There was a problem hiding this comment.
The fact that the tests are passing doesn’t necessarily mean the implementation is correct.
It depends on what the end-to-end (E2E) tests are actually verifying with respect to this test data.
Also, note that the serviceAccountName should not be defined inside the container specification.
You can refer to this example for the correct structure:
https://github.com/operator-framework/operator-sdk/blob/master/testdata/go/v4/memcached-operator/bundle/manifests/memcached-operator.clusterserviceversion.yaml#L198
There was a problem hiding this comment.
I'll check this out and poke at it. It seems weird that the e2es would pass. But, I guess they are only checking for the installed status and maybe the underlying helm operation succeeded while the operator is itself it bung?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2311 +/- ##
==========================================
+ Coverage 74.18% 74.29% +0.11%
==========================================
Files 91 91
Lines 7046 7046
==========================================
+ Hits 5227 5235 +8
+ Misses 1404 1400 -4
+ Partials 415 411 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hi @pedjak 👋
Thank you for the contribution!
I’m not sure what problem this change solves — could you please clarify?
- How does it help us test OLM features?
Otherwise, I’d prefer to keep the samples simple and focused only on what’s needed to test OLM.
| echo true > /var/www/started | ||
| echo true > /var/www/ready | ||
| echo true > /var/www/live | ||
| exec httpd -f -h /var/www -p 80 |
There was a problem hiding this comment.
TL;DR (just to share):
- Common case of content managed by OLM is scaffolded with Operator SDK/Kubebuilder, which use controller-runtime.
- We already have a project used in downstream tests that we maintain — test-operator — which includes health and ready probes. See the default implementation: https://github.com/operator-framework/test-operator/blob/main/cmd/main.go#L238-L245 ( controller runtime )
If there’s a strong reason to add probe implementations here (which doesn’t seem to be the case right now), I’d prefer to rely on the existing setup — meaning we can create a bundle using test-operator.
Otherwise, adding probes here feels unnecessary. I’d suggest using the existing test-operator instead.
There was a problem hiding this comment.
I don't think we ever reached a steady state on the test-operator or use it within our e2es. I'm actually wondering if we should nuke it for now.
The nice thing about having the health and readiness probes defined in this way is that we can very easily change their state at runtime by creating/deleting the files in /var/www, which is great for e2e testing. This is the real value added here by this PR.
There was a problem hiding this comment.
That might be a true point. However, unless we add tests that actually take advantage of it, I don’t see a strong reason to include it.
Could we add some unit tests or use case scenarios that demonstrate where this feature would be helpful? If so, I’d be completely on board with it — that would show not only that this change is useful, but also that it works as expected.
3 participants
Description
Enhance test-operator v1.0.0 and v1.2.0 bundles with HTTP-based health probes
to enable testing of startup, liveness, and readiness probe behavior. Replaces
the simple sleep container with a busybox httpd server that exposes probe
endpoints via a ConfigMap-mounted script.
The probes can be flipped from true to false by executing
rm /var/www{stared,ready,live}and back to true by performin
touch /var/www{stared,ready,live}Reviewer Checklist