Add shared dev environment for medik8s operators#21
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mpryc The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds a shared medik8s development environment under ChangesDev Environment Tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant devmk as dev.mk
participant setupsh as setup.sh
participant certsh as enable-certmanager.sh
participant nhcsh as create-nhc.sh
participant Kind
participant Kubernetes as Kubernetes API
Developer->>devmk: make dev-setup
devmk->>setupsh: invoke setup.sh
setupsh->>Kind: create cluster / verify existing
setupsh->>Kubernetes: create namespaces, install cert-manager/OLM
Developer->>devmk: make dev-deploy
devmk->>Kubernetes: apply kustomize manifests
devmk->>certsh: enable-certmanager.sh (if webhook config present)
certsh->>Kubernetes: apply Issuer/Certificate, patch deployment
devmk->>Kubernetes: wait for deployment Available
devmk->>nhcsh: create-nhc.sh (if remediation template found)
nhcsh->>Kubernetes: apply NodeHealthCheck CR
sequenceDiagram
participant Developer
participant devmk as dev.mk
participant simsh as simulate-failure.sh
participant Worker as Kind worker node
participant Kubernetes as Kubernetes API
Developer->>devmk: make dev-simulate-failure SCENARIO=kubelet-stop
devmk->>simsh: invoke simulate-failure.sh
simsh->>Worker: stop kubelet / drop iptables / concurrent stops
simsh->>Kubernetes: monitor node status and remediation CRs
Developer->>devmk: make dev-recover
devmk->>simsh: invoke simulate-failure.sh recover
simsh->>Worker: start kubelet / remove iptables rule
simsh->>Kubernetes: wait for nodes Ready and CRs to clean up
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Shared Kind-based development environment for all medik8s operators. Provides consistent cluster setup, operator deployment, failure simulation, and observability across NHC, SNR, FAR, MDR, NMO, and SBR. Features: - Kind cluster with 1 CP + 3 workers, cert-manager, OLM, softdog - Build/deploy operators with automatic cert-manager webhook configuration - Failure simulations: kubelet stop, network partition, storm - Observability: logs, describe, summary, events - External cluster support (OCP, etc.) with SKIP_KIND=true - ttl.sh ephemeral registry support (DEV_REGISTRY=ttl.sh) - Rootless podman support with cpuset cgroup detection - Shellcheck CI via GitHub Actions Signed-off-by: Michal Pryc <mpryc@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
razo7
left a comment
razo7
left a comment
There was a problem hiding this comment.
I tested dev-* targets with all 6 Medik8s operators (SNR, NHC, FAR, MDR, NMO, SBR) on Kind v0.32.0 + K8s v1.34.8. Ran 6 solo cycles + retries, 1 non-NHC pairwise, and 5 NHC pairwise. Below are my results.
Solo Test Results
| Operator | Result | Notes |
|---|---|---|
| SNR | PASS (8/8) | All dev targets work correctly |
| NHC | PASS* | Manual deploy needed (controller-gen Go 1.26 incompatibility + CRD $ref fix). Label mismatch breaks dev-wait/dev-logs |
| FAR | PASS* | Manual cert-manager workaround (dev-deploy Error 2 prevents enable-certmanager.sh) |
| MDR | PASS (8/8) | No webhooks, clean deployment |
| NMO | PASS* | Same cert-manager workaround as FAR |
| SBR | PASS* | Needs CONTAINER_TOOL=docker override |
Pairwise Test Results
| Pair | Result | Notes |
|---|---|---|
| SNR + MDR | PASS | Multi-operator deployment verified, dev-wait iterates both |
| NHC + SNR | PASS | Both run, dev-wait finds SNR only (NHC label mismatch) |
| NHC + FAR | PASS | Both run, no CRD/RBAC conflicts |
| NHC + MDR | PASS | Both run cleanly |
| NHC + NMO | PARTIAL | NHC runs, NMO CrashLoopBackOff (cert-manager timing) |
| NHC + SBR | PASS | Both run cleanly |
The design is solid and works automatically for some operators, and for some we need some adjustments (see below). This is a very good first step towards shared kind cluster targets :)
Findings (sorted by priority)
-
dev-deploy Error 2 aborts cert-manager setup — The kustomize build/apply shell block returns Error 2 to make despite all resources being created and the shell exiting 0. This prevents
enable-certmanager.shfrom running. Affects FAR, NMO, and any operator with webhooks. Runningenable-certmanager.shmanually works perfectly. -
NHC label mismatch breaks dev-wait/dev-logs/dev-describe/dev-redeploy — NHC uses
app.kubernetes.io/component=controller-managerwhile all dev targets hardcodecontrol-plane=controller-manager. NHC pods are invisible to all label-based dev targets. Suggest adding aDEV_POD_LABELvariable with fallback: trycontrol-plane=controller-managerfirst, thenapp.kubernetes.io/component=controller-manager. Tracked in RHWA-1312. -
dev-wait exits 0 when no deployments found — Returns success with "No operator deployments found" message. Should exit non-zero so CI/scripting detects the problem.
-
CONTAINER_TOOL mismatch —
setup.shauto-detects Docker, but operator Makefiles may default to podman. SBR'sCONTAINER_TOOL ?= podmancauses build failures. Document that CONTAINER_TOOL should be passed explicitly, or have dev.mk's detection take precedence. We could maybe include SBR fix at medik8s/storage-based-remediation#63 -
kustomize
commonLabelsdeprecation warnings — All operators emit "Warning: 'commonLabels' is deprecated". Not blocking but noisy. -
NHC CRD uses
$refrejected by K8s 1.34 — NHC's CRD has$refconstructs in validation schemas. Modern K8s rejects these. Fixed for testing by removing$refand addingtype: object+x-kubernetes-preserve-unknown-fields: trueto 3 fields. This is an NHC upstream issue, not tools PR. -
dev-describe misses NHC pods in "Operator Pods" section. Same label mismatch as finding 2 from above. NHC events appear in the events section.
-
enable-certmanager.sh reports "patched (no change)" for some operators — NMO's deployment already has the TLS volume mount in its default config, so the patch is a no-op. Not harmful but confusing output.
| else \ | ||
| $(KUSTOMIZE) build config/default | $(KUBECTL) apply -f -; \ | ||
| fi | ||
| cd config/manager && $(KUSTOMIZE) edit set image controller=$(IMG) |
There was a problem hiding this comment.
This restore step (kustomize edit set image controller=$(IMG)) fails with Error 2 for FAR, NMO, and SBR. Since the entire dev-deploy recipe is a single make rule, this failure prevents enable-certmanager.sh from running.
Impact: Operators with webhooks (FAR, NMO) deploy but crash on startup with open /tmp/k8s-webhook-server/serving-certs/tls.crt: no such file or directory.
Tested on: FAR, NMO, and SBR when all hit the same error.
Suggested fix: Add || true to this line, or move cert-manager setup before the restore:
cd config/manager && $(KUSTOMIZE) edit set image controller=$(IMG) || trueThe restore is a courtesy to avoid dirtying the operator's git state — it should never block deployment.
There was a problem hiding this comment.
Fixed. Replaced kustomize edit set image controller=$(IMG) restore with cp/mv backup+trap pattern. The kustomization.yaml is now backed up before modification and restored via trap on EXIT - even if kustomize build or kubectl apply fails, cert-manager setup still runs. Also wrapped grep calls in { ... || true; } to prevent pipefail from killing the recipe when config/patches/ doesn't exist (FAR).
| @# Files are restored after build (even on failure) via trap. | ||
| ifeq ($(DEV_REGISTRY),local) | ||
| @patched=""; \ | ||
| for f in $$(find install/ -name '*.yaml' 2>/dev/null); do \ |
There was a problem hiding this comment.
setup.sh uses common.sh's detect_container_tool() which prefers Docker. But dev-build inherits $(CONTAINER_TOOL) from the operator's Makefile. SBR sets CONTAINER_TOOL ?= podman, so:
setup.shcreates cluster with Dockerdev-buildtriesKIND_EXPERIMENTAL_PROVIDER=podman kind load image-archive- kind can't find the cluster's Docker-managed nodes
Tested on: SBR (cycle 06) — kind load fails with "ERROR: no nodes found for cluster".
Suggested fix: dev.mk should source CONTAINER_TOOL from common.sh instead of inheriting from the operator:
CONTAINER_TOOL := $(shell . $(DEV_DIR)/common.sh && echo $$CONTAINER_TOOL)There was a problem hiding this comment.
Fixed. Changed from CONTAINER_TOOL ?= to override CONTAINER_TOOL := in dev.mk. This ensures dev targets always use the same auto-detected container tool as setup.sh, regardless of what the operator's Makefile sets. The override directive takes precedence over both ?= and command-line overrides from the operator Makefile.
| echo "Could not find deployment to restart. Run 'make dev-deploy' first."; \ | ||
| fi | ||
|
|
||
| .PHONY: dev-logs |
There was a problem hiding this comment.
When dev-deploy fails partway through (no deployment created), dev-wait reports "No operator deployments found" but exits 0. This masks deployment failures in scripts.
Tested on: NHC as dev-deploy failed at manifests target, no deployment created, dev-wait exited 0.
Suggested fix: Exit non-zero when no deployments are found.
There was a problem hiding this comment.
Fixed. dev-wait now exits 1 when no operator deployments are found.
| # Check inotify limits — Kind nodes inherit host limits and operators need many watchers. | ||
| INOTIFY_INSTANCES=$(cat /proc/sys/fs/inotify/max_user_instances 2>/dev/null || echo 0) | ||
| INOTIFY_WATCHES=$(cat /proc/sys/fs/inotify/max_user_watches 2>/dev/null || echo 0) | ||
| if [ "${INOTIFY_INSTANCES}" -lt 512 ] || [ "${INOTIFY_WATCHES}" -lt 524288 ]; then |
There was a problem hiding this comment.
The hard exit at max_user_instances < 512 blocked my machine (default: 128). Kind works fine with 128 for single-operator testing.
Suggested fix: Add a --skip-inotify-check flag or lower the threshold for single-operator use.
There was a problem hiding this comment.
Fixed. Added --skip-inotify-check flag to setup.sh. The dev-setup target passes it through when invoked. Users can skip the check with: make dev-setup SKIP_INOTIFY_CHECK=true or directly: ./setup.sh --skip-inotify-check
The check message also now tells users about the skip flag.
Without the skip flag there is a hard exit as I want to make it explicit - normally full e2e tests is not passing with standard limits on 2 test boxes.
| @@ -0,0 +1,15 @@ | |||
| # Kind cluster configuration for medik8s development | |||
There was a problem hiding this comment.
Without image: on node entries, Kind uses its compiled-in default. Kind v0.20.0 defaults to K8s v1.27.3, incompatible with cert-manager latest (selectableFields requires K8s 1.29+).
I worked around this by adding KIND_NODE_IMAGE env var support to setup.sh, but new users will hit a cryptic CRD validation error.
Suggested fix: Pin the node image in kind-config.yaml to at least v1.30.0, or pin cert-manager to a compatible version.
There was a problem hiding this comment.
Fixed differently. Instead of pinning a node image (which would need constant updating and this is just dev environment so rarely people do update), we now enforce Kind >= 0.22.0 at startup. Kind 0.22.0+ defaults to K8s 1.29+, which is compatible with cert-manager latest. The README also documents the minimum Kind version (v0.22.0+).
This will allow us to catch API changes in the latest K8s versions much quicker rather than manually updating pinned versions.
| echo "Waiting for SNR controller to finish remediation cleanup..." | ||
| # The SNR controller needs to observe the node is healthy, remove taints, | ||
| # and strip its finalizer before the CR can be deleted. Give it time. | ||
| REMAINING=0 | ||
| WAITED=0 | ||
| while [ $WAITED -lt 120 ]; do | ||
| REMAINING=$(${KUBECTL} get selfnoderemediation -A --no-headers 2>/dev/null | wc -l) | ||
| if [ "$REMAINING" -eq 0 ]; then | ||
| break | ||
| fi | ||
| echo " $REMAINING remediation CR(s) still being processed..." | ||
| sleep 5 | ||
| WAITED=$((WAITED + 5)) | ||
| done |
There was a problem hiding this comment.
Can be any other remediator CRs...
|
|
||
| ```bash | ||
| # 1. Create the Kind cluster (from any operator directory) | ||
| pushd self-node-remediation |
| ## Limitations | ||
|
|
||
| - **FAR fence agent execution** — no IPMI/BMC or cloud APIs | ||
| - **MDR Machine API** — Kind has no Machine objects | ||
| - **SBR shared storage** — no ODF | ||
| - **Hardware watchdog** — only softdog (software) |
There was a problem hiding this comment.
Is there a way to overcome these limitations?
There was a problem hiding this comment.
We need to check this, but IMO not within first version of this dev-env?
For now I've added guidance in the Limitations section:
"For full-fidelity testing of these scenarios, use an OpenShift cluster with real hardware (BMC/IPMI for FAR, Machine API for MDR, ODF for SBR, hardware watchdog for SNR). The SKIP_KIND=true workflow supports deploying to external clusters."
| @echo " Image will expire after $(TTL_SH_TTL)." | ||
| endif | ||
|
|
||
| .PHONY: dev-deploy |
There was a problem hiding this comment.
To deploy/install an operator, we can use bundle-run target to install the bundle.
There was a problem hiding this comment.
Done. Added dev-bundle-run and dev-bundle-cleanup targets.
| fi | ||
|
|
||
| .PHONY: dev-undeploy | ||
| dev-undeploy: ## Remove operator from dev cluster |
There was a problem hiding this comment.
To undeploy/uninstall an operator, we can use bundle-cleanup target to uninstall the bundle.
…obustness fixes
Key changes addressing Or Raz's review:
- Fix dev-deploy Error 2: backup/restore kustomization.yaml with cp/mv+trap
instead of kustomize edit restore; wrap grep in { || true } for pipefail safety
- Fix CONTAINER_TOOL mismatch: use `override CONTAINER_TOOL :=` so dev targets
always use the same tool as setup.sh regardless of operator Makefile defaults
- Fix NHC label mismatch: add `app.kubernetes.io/component=controller-manager`
fallback in _dev_find_ns, dev-deploy, dev-wait, dev-logs, dev-redeploy, dev-describe
- Fix dev-wait exit code: exit 1 when no deployments found
- Fix inotify check: add --skip-inotify-check flag for single-operator use
- Fix Kind version: enforce >= 0.22.0 (K8s 1.29+ for cert-manager selectableFields)
- Fix cert-manager DNS names: read namePrefix from all kustomization locations
- Fix enable-certmanager.sh: skip patch when deployment already has TLS volume
- Support multiple remediators: create-nhc.sh auto-detects SNR/FAR/MDR templates
- Add NHC_UNHEALTHY_DURATION config (default: 300s, matching downstream docs)
- Add KIND_HA=true support with kind-config-ha.yaml (3 CP + 3 workers)
- Add dev-bundle-run / dev-bundle-cleanup targets for OLM bundle deployment
- Filter kustomize commonLabels deprecation warnings from output
- Merge dev-build recipe lines so trap covers container build failures
- describe.sh: show pod node placement and leader election info
- simulate-failure.sh: show monitor hints for all installed remediator CRDs
- Update README: lazy clone snippet, Kind version, OLM v0 note, SNR envsubst
limitation, operator coverage known issues, external cluster workflow
Signed-off-by: Michal Pryc <mpryc@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@razo7 Than you for such detailed review. I think all of the comments were addressed. Also tested the full update against:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
dev/README.md (2)
79-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block.
The code block at line 79 (sysctl config) lacks a language specifier, triggering markdownlint MD040. Use
iniorconf:+```ini
fs.inotify.max_user_instances=8192
fs.inotify.max_user_watches=524288🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/README.md` at line 79, Add a language specifier to the fenced sysctl configuration block in the README to satisfy markdownlint MD040; update the markdown fence near the sysctl example to use a config language such as ini or conf, keeping the block content unchanged.Source: Linters/SAST tools
130-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block.
The code block at line 130 (echo command) lacks a language specifier, triggering markdownlint MD040. Use
bash:+```bash
echo '.tools/' >> .gitignore🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/README.md` at line 130, The fenced code block for the echo command is missing a language specifier, which triggers markdownlint MD040. Update the markdown snippet in the README so the block is labeled with bash, and ensure the fenced block around the echo command uses the proper language hint.Source: Linters/SAST tools
dev/summary.sh (1)
54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOnly checks
control-plane=controller-managerlabel, missing the fallback used indescribe.sh.
describe.shiterates over bothcontrol-plane=controller-managerandapp.kubernetes.io/component=controller-managerlabels (line 22), butsummary.shonly uses the first. If an operator uses only theapp.kubernetes.io/componentlabel, its pods won't appear here. Consider matching the same label set for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/summary.sh` around lines 54 - 56, The controller-manager pod listing in summary.sh only filters on the control-plane=controller-manager label, so it can miss pods labeled with app.kubernetes.io/component=controller-manager. Update the pod query in the summary script to use the same label matching logic as describe.sh, so both label variants are covered consistently. Keep the change in the get pods command that produces the controller-manager summary output..github/workflows/shellcheck.yml (1)
15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBump
actions/checkouttov7.v6is still a valid tag, butv7.0.0is the current release;ludeeus/action-shellcheck@2.0.0is already current.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/shellcheck.yml around lines 15 - 18, Update the workflow to use the current release of actions/checkout by bumping the checkout step from v6 to v7 in the shellcheck job; keep ludeeus/action-shellcheck@2.0.0 unchanged, and adjust the action reference in the workflow definition accordingly.dev/setup.sh (1)
43-46: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
--namewithout a value produces a cryptic error underset -e.If
--nameis passed without a following argument,shift 2fails silently underset -eand the script exits without a helpful message. Consider validating$# -ge 2before consuming$2.🛡️ Proposed fix
--name) + if [[ $# -lt 2 ]]; then + echo "Error: --name requires a cluster name argument." + exit 1 + fi CLUSTER_NAME="$2" shift 2 ;;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/setup.sh` around lines 43 - 46, The `--name` option handling in `setup.sh` can fail cryptically when no value follows it, so add an explicit argument-count check before consuming `$2` in the `--name)` case. Update the option parsing logic to validate that at least two arguments remain, emit a clear error message if the value is missing, and only then assign `CLUSTER_NAME` and `shift 2` in the same branch.dev/common.sh (1)
30-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider lazy detection to avoid coupling scripts to tools they don't use.
Both
detect_kubectlanddetect_container_toolexecute at source time, so any script sourcingcommon.shwill hard-exit if either tool family is missing — even scripts that only need one. For example,teardown.shonly usesKUBECTLbut will fail if neitherdockernorpodmanis installed.This is a reasonable tradeoff for a dev environment where both tool families are expected to coexist, but if you later add scripts that only need
kubectl(e.g., a pure observability script run against an external cluster without container tooling), they'll be blocked unnecessarily. Consider deferring detection to first use, or splitting into separatesourcelines per script.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/common.sh` around lines 30 - 31, Lazy-load tool detection in common.sh so sourcing it does not fail for scripts that only need one dependency. The current KUBECTL and CONTAINER_TOOL assignments eagerly call detect_kubectl and detect_container_tool at source time; refactor this so detection happens on first use or is split behind script-specific sourcing, keeping the existing symbols available without coupling every consumer to both tool families.dev/enable-certmanager.sh (1)
76-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCA-injection annotation loop uses a fragile substring match.
grep -q "namespace: ${NAMESPACE}"against the full YAML of every webhookconfiguration in the cluster can match unrelated fields (labels, annotations, ownerReferences) rather thanclientConfig.service.namespace. In a shared dev cluster with multiple operators/cert-manager/OLM webhooks, this could annotate an unrelated webhook for CA injection and disrupt its TLS trust chain.♻️ Suggested fix: query the specific field
for wh_type in mutatingwebhookconfigurations validatingwebhookconfigurations; do for wh in $(${KUBECTL} get "${wh_type}" -o name 2>/dev/null); do - # Only annotate webhooks that reference services in our namespace - if ${KUBECTL} get "${wh}" -o yaml 2>/dev/null | grep -q "namespace: ${NAMESPACE}"; then + # Only annotate webhooks whose clientConfig actually targets our namespace + if ${KUBECTL} get "${wh}" -o jsonpath='{.webhooks[*].clientConfig.service.namespace}' 2>/dev/null | grep -qw "${NAMESPACE}"; then ${KUBECTL} annotate "${wh}" cert-manager.io/inject-ca-from="${NAMESPACE}/serving-cert" --overwrite 2>/dev/null || true fi done done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/enable-certmanager.sh` around lines 76 - 83, The webhook CA-injection loop is matching on a broad YAML substring instead of the service namespace field. Update the logic in the webhook iteration around the ${KUBECTL} get/annotate flow to inspect only clientConfig.service.namespace for each mutatingwebhookconfigurations/validatingwebhookconfigurations entry, and then apply cert-manager.io/inject-ca-from only when that exact field equals ${NAMESPACE}. Keep the existing wh_type/wh iteration but replace the fragile grep-based check with a precise field query.dev/create-nhc.sh (1)
16-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
--durationwithout a value crashes with an unhelpful error.If
--durationis the last argument,shift 2(line 20) errors with "shift count out of range" instead of a clear usage message, becauseset -u(line 6) also makes the preceding$2reference an unbound-variable error first.🛡️ Suggested fix
case $1 in --duration) + if [ $# -lt 2 ]; then + echo "Error: --duration requires a value" >&2 + exit 1 + fi NHC_UNHEALTHY_DURATION="$2" shift 2 ;;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/create-nhc.sh` around lines 16 - 29, The --duration option handling in create-nhc.sh should validate that a value is present before accessing $2 or calling shift 2. Update the argument parsing in the while/case block so the --duration branch checks for a missing next argument and prints the existing usage message with a clean exit instead of triggering set -u or “shift count out of range”; keep the fix localized around the --duration case and the script’s usage output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/shellcheck.yml:
- Around line 14-15: The Checkout step in the shellcheck workflow uses
actions/checkout without disabling stored GitHub credentials. Update the
checkout configuration in the workflow to set persist-credentials to false on
the actions/checkout invocation so this job does not keep the token in
.git/config.
In `@dev/dev.mk`:
- Around line 144-197: The dev-deploy recipe is swallowing failures from
critical steps, so make it stop on errors and return non-zero when setup or
readiness checks fail. In dev-deploy, ensure the kustomize edit set image step
and enable-certmanager.sh are chained with explicit failure propagation, and
change the kubectl wait / warning path so a failed deployment readiness check
sets an error status instead of only logging. Use the dev-deploy target plus the
kustomize edit set image, enable-certmanager.sh, and kubectl wait blocks to
locate the fix.
- Around line 9-56: Move the CONTAINER_TOOL auto-detection block above the
DEV_CLUSTER_TYPE selection in dev.mk so the Kind probe has a valid provider when
it expands KIND_EXPERIMENTAL_PROVIDER. Update the ordering around
DEV_CLUSTER_TYPE, DEV_REGISTRY, and DEV_IMG so $(CONTAINER_TOOL) is defined
before it is referenced, preserving the intended kind vs external detection and
the correct image branch for podman-backed setups.
In `@dev/setup.sh`:
- Around line 106-135: The inotify validation in the setup script always runs
even on non-Linux hosts, causing macOS to fail because /proc/sys/fs/inotify does
not exist. Update the logic in the inotify check block of setup.sh to detect the
platform first (for example via uname or a Linux-only guard) and automatically
skip the inotify limits check on non-Linux systems while preserving the existing
threshold check on Linux.
In `@dev/summary.sh`:
- Line 17: The node status lookup in the kubectl command is using the last entry
in .status.conditions, which may not be the Ready condition. Update the logic in
the summary.sh node listing to explicitly select the Ready condition (or switch
to plain kubectl get nodes, which reports readiness correctly) so the
STATUS/READY columns reflect the actual node readiness rather than an arbitrary
condition.
---
Nitpick comments:
In @.github/workflows/shellcheck.yml:
- Around line 15-18: Update the workflow to use the current release of
actions/checkout by bumping the checkout step from v6 to v7 in the shellcheck
job; keep ludeeus/action-shellcheck@2.0.0 unchanged, and adjust the action
reference in the workflow definition accordingly.
In `@dev/common.sh`:
- Around line 30-31: Lazy-load tool detection in common.sh so sourcing it does
not fail for scripts that only need one dependency. The current KUBECTL and
CONTAINER_TOOL assignments eagerly call detect_kubectl and detect_container_tool
at source time; refactor this so detection happens on first use or is split
behind script-specific sourcing, keeping the existing symbols available without
coupling every consumer to both tool families.
In `@dev/create-nhc.sh`:
- Around line 16-29: The --duration option handling in create-nhc.sh should
validate that a value is present before accessing $2 or calling shift 2. Update
the argument parsing in the while/case block so the --duration branch checks for
a missing next argument and prints the existing usage message with a clean exit
instead of triggering set -u or “shift count out of range”; keep the fix
localized around the --duration case and the script’s usage output.
In `@dev/enable-certmanager.sh`:
- Around line 76-83: The webhook CA-injection loop is matching on a broad YAML
substring instead of the service namespace field. Update the logic in the
webhook iteration around the ${KUBECTL} get/annotate flow to inspect only
clientConfig.service.namespace for each
mutatingwebhookconfigurations/validatingwebhookconfigurations entry, and then
apply cert-manager.io/inject-ca-from only when that exact field equals
${NAMESPACE}. Keep the existing wh_type/wh iteration but replace the fragile
grep-based check with a precise field query.
In `@dev/README.md`:
- Line 79: Add a language specifier to the fenced sysctl configuration block in
the README to satisfy markdownlint MD040; update the markdown fence near the
sysctl example to use a config language such as ini or conf, keeping the block
content unchanged.
- Line 130: The fenced code block for the echo command is missing a language
specifier, which triggers markdownlint MD040. Update the markdown snippet in the
README so the block is labeled with bash, and ensure the fenced block around the
echo command uses the proper language hint.
In `@dev/setup.sh`:
- Around line 43-46: The `--name` option handling in `setup.sh` can fail
cryptically when no value follows it, so add an explicit argument-count check
before consuming `$2` in the `--name)` case. Update the option parsing logic to
validate that at least two arguments remain, emit a clear error message if the
value is missing, and only then assign `CLUSTER_NAME` and `shift 2` in the same
branch.
In `@dev/summary.sh`:
- Around line 54-56: The controller-manager pod listing in summary.sh only
filters on the control-plane=controller-manager label, so it can miss pods
labeled with app.kubernetes.io/component=controller-manager. Update the pod
query in the summary script to use the same label matching logic as describe.sh,
so both label variants are covered consistently. Keep the change in the get pods
command that produces the controller-manager summary output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22c52222-9b9a-4f02-94fb-841d1ae27ac4
📒 Files selected for processing (14)
.github/workflows/shellcheck.ymlREADME.mddev/README.mddev/common.shdev/create-nhc.shdev/describe.shdev/dev.mkdev/enable-certmanager.shdev/kind-config-ha.yamldev/kind-config.yamldev/setup.shdev/simulate-failure.shdev/summary.shdev/teardown.sh
…pagation - Move CONTAINER_TOOL auto-detection above DEV_CLUSTER_TYPE in dev.mk so KIND_EXPERIMENTAL_PROVIDER has a valid value during Kind cluster probe - Skip inotify limits check on non-Linux hosts (macOS has no /proc/sys/fs/inotify) - Change dev-deploy kubectl wait from warning to error exit on readiness failure - Add persist-credentials: false to shellcheck workflow checkout step - Fix summary.sh node status: use kubectl get nodes -o wide instead of conditions[-1:] which may not be the Ready condition - Add external cluster support to simulate-failure.sh: prints oc debug / SSH commands by default, DEV_FORCE_SIMULATE=true to auto-execute via oc debug - Add Kind vs OpenShift recovery documentation to README with examples - Make cleanup steps consistent: dev-undeploy (both) + dev-teardown (Kind only) Signed-off-by: Michal Pryc <mpryc@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Add shared dev environment for medik8s operators
Why we need this PR
Every medik8s operator today requires its own manual cluster setup, deployment steps, and debugging workflow. There is no standard way to:
This leads to inconsistent setups, wasted onboarding time, and "works on my machine" issues. New contributors have no clear path to run operators locally end-to-end.
This PR adds a shared
dev/directory in thetoolsrepo that any operator can include with a one-line Makefile snippet. All operators get the samemake dev-*targets with zero per-repo configuration.Changes made
Files added under
dev/:dev.mkdev-setup,dev-deploy,dev-describe, etc.) included via-includecommon.shsetup.shKIND_HA=true), installs cert-manager, OLM, labels workers, loads softdogenable-certmanager.shcreate-nhc.shsimulate-failure.shoc debug/ SSH commands (safe by default) or auto-executes withDEV_FORCE_SIMULATE=truedescribe.shsummary.shteardown.shkind-config.yamlkind-config-ha.yamlCI:
.github/workflows/shellcheck.ymldev/**/*.shon PRs and pushesOperator integration — add this snippet to any operator's Makefile:
This uses a local sibling checkout (
../tools) if available, otherwise shallow-clones the repo on firstdev-*target use. The clone is lazy — it only runs when a dev target is invoked, not on everymakeinvocation.How it works
Kind cluster workflow:
External cluster (OCP, etc.):
Images are pushed to ttl.sh — an anonymous, ephemeral registry that requires no auth. Images expire after 2h by default (configurable via
TTL_SH_TTL). You can also force ttl.sh on a Kind cluster withDEV_REGISTRY=ttl.sh make dev-deploy.Kind vs OpenShift recovery:
On Kind,
dev-simulate-failurestops kubelet viadocker exec, which also kills the SNR agent pod on that node. Since a Kind container "reboot" doesn't restart kubelet, the automatic recovery can't complete —dev-recoversimulates what a real reboot would do. The dev environment tests the detection/decision flow (NHC detects unhealthy node → creates remediation CR), not the full reboot cycle.On OpenShift, SNR automatically reboots the unhealthy node, kubelet restarts on boot, and the node rejoins — full self-healing, no manual
dev-recoverneeded. On external clusters,dev-simulate-failureprintsoc debug/ SSH commands by default; setDEV_FORCE_SIMULATE=trueto auto-execute.Multi-operator flow (NHC + any remediator):
Key design decisions:
SKIP_KINDcontrols cluster lifecycle (Kind vs external),DEV_REGISTRYcontrols image delivery (local vs ttl.sh) — these are independent concernsoverride CONTAINER_TOOL :=ensures dev targets use the same container tool assetup.sh, regardless of operator Makefile defaultsdev-setupbecausedev-deploycreates cert-manager Certificate resources for webhook TLSenable-certmanager.shretries Issuer creation for up to 30s to handle cert-manager webhook startup delay, and skips the TLS volume patch if already presentconfig/webhook/service.yaml+namePrefixfrom all kustomization locations (handles NHC's non-default namePrefix location)kustomization.yamlis backed up/restored viacp/mv+ trap,imagePullPolicysed patches are restored even on build failurecreate-nhc.shauto-detects deployed remediator templates (SNR → FAR → MDR), with configurable unhealthy duration (NHC_UNHEALTHY_DURATION, default 300s matching downstream docs)control-plane=controller-managerandapp.kubernetes.io/component=controller-managerlabels (NHC uses the latter)dev-waitexits non-zero when no deployments are found, so CI/scripting can detect failuresdev-%fallback rule for git clone — eager$(shell git clone ...)would trigger on everymakeinvocation--skip-inotify-checkCompanion PRs
The following draft PRs add the Makefile snippet to individual operators:
Remaining operators that need the same change (identical snippet +
.tools/in.gitignore):fence-agents-remediationmachine-deletion-remediationnode-maintenance-operatorcustomized-user-remediationstorage-based-remediationWhich issue(s) this PR fixes
N/A — new functionality.
Test plan
Tested manually with all 6 medik8s operators on Kind (podman, Kind v0.32.0, K8s v1.36.1) and external OCP (IBM Cloud ROKS 4.21):
Solo operator tests (Kind):
app.kubernetes.io/component), namePrefix fromconfig/patches/override CONTAINER_TOOLprevents mismatch. Namespace lookup via kustomization file. Leader election RBAC error (SBR bug).Multi-operator tests (Kind):
External cluster tests (OCP 4.21):
dev-setup(SKIP_KIND=true)dev-deploy(SNR, FAR, NHC via ttl.sh)dev-wait/dev-describe/dev-logsdev-undeploySource file integrity: Verified all 6 operator repos have no modified source files (
git diff) after deploy/undeploy cycles. The backup/restore mechanism (cp/mv + trap) correctly restoreskustomization.yamlandimagePullPolicypatches even on failure.CI: Shellcheck passes at all severity levels (including info).