test(breakfix): implement BFX01-04 cordon validation - #570
Conversation
Signed-off-by: Hasan Khan <hasank@nvidia.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
isvctl/tests/test_my_isv_cordon_node.py (1)
193-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the untested failure branches.
The suite covers the uncordon failure but not these branches:
_wait_for_unschedulablereturningFalseon timeout, which produces the "New probe pod was not confirmed unschedulable" error.- The delete-pod failure branch in
_cleanup, which appends adelete pod ...entry tocleanup_errors._kubectl_commandwith a blank or unbalanced-quoteKUBECTLvalue.If you add the
ISVCTL_DEMO_MODEgate requested on the script, also add a demo-mode test that asserts no kubectl call occurs. I can draft these tests if you want.🤖 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 `@isvctl/tests/test_my_isv_cordon_node.py` around lines 193 - 207, Add tests covering the missing failure branches: make _wait_for_unschedulable return False and assert the “New probe pod was not confirmed unschedulable” error, make pod deletion fail in _cleanup and assert cleanup_errors includes a “delete pod …” entry, and verify _kubectl_command rejects blank and unbalanced-quote KUBECTL values. If ISVCTL_DEMO_MODE is added, also test that the workflow makes no kubectl calls in demo mode.isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.py (1)
142-147: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the probe pod lifetime with
activeDeadlineSeconds.Cleanup runs only in the
finallyblock ofmain. If the process is killed, the probe pods stay in the namespace. A pod-level deadline lets Kubernetes remove them without the script.♻️ Proposed change
"spec": { "restartPolicy": "Never", + "activeDeadlineSeconds": 3600, "nodeSelector": {"kubernetes.io/hostname": hostname}, "tolerations": tolerations, "containers": [{"name": "probe", "image": image}], },🤖 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 `@isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.py` around lines 142 - 147, Update the probe pod template in the relevant pod-creation flow to include a pod-level activeDeadlineSeconds value, using an appropriate finite timeout for the probe lifetime. Keep the existing restartPolicy, nodeSelector, tolerations, and container configuration unchanged.
🤖 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 `@isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.py`:
- Around line 238-254: Restore the ISVCTL_DEMO_MODE=1 check at the start of
main, before _kubectl_command() is called, and return the expected dummy
successful provider-neutral result without performing cluster operations.
Preserve the existing validation and cordon workflow for non-demo runs.
---
Nitpick comments:
In `@isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.py`:
- Around line 142-147: Update the probe pod template in the relevant
pod-creation flow to include a pod-level activeDeadlineSeconds value, using an
appropriate finite timeout for the probe lifetime. Keep the existing
restartPolicy, nodeSelector, tolerations, and container configuration unchanged.
In `@isvctl/tests/test_my_isv_cordon_node.py`:
- Around line 193-207: Add tests covering the missing failure branches: make
_wait_for_unschedulable return False and assert the “New probe pod was not
confirmed unschedulable” error, make pod deletion fail in _cleanup and assert
cleanup_errors includes a “delete pod …” entry, and verify _kubectl_command
rejects blank and unbalanced-quote KUBECTL values. If ISVCTL_DEMO_MODE is added,
also test that the workflow makes no kubectl calls in demo mode.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6ac58286-9545-4069-9971-742327eaff9d
📒 Files selected for processing (2)
isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.pyisvctl/tests/test_my_isv_cordon_node.py
| def main() -> int: | ||
| """Run the reversible cordon test and emit its provider-neutral JSON result.""" | ||
| args = _parser().parse_args() | ||
| operation: dict[str, Any] = { | ||
| "cordoned": False, | ||
| "new_workloads_blocked": False, | ||
| "existing_workloads_running": False, | ||
| } | ||
| result: dict[str, Any] = {"success": False, "platform": "my-isv", "test_name": "cordon_node"} | ||
| kubectl: list[str] = [] | ||
| created_pods: list[str] = [] | ||
| cordoned_node: str | None = None | ||
|
|
||
| try: | ||
| if args.timeout_seconds <= 0 or args.poll_interval_seconds <= 0: | ||
| raise CordonTestError("Timeout and poll interval must be greater than zero") | ||
| kubectl = _kubectl_command() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the DEMO_MODE gate convention across my-isv provider scripts.
fd -e py . isvctl/configs/providers/my-isv/scripts --exec rg -n 'ISVCTL_DEMO_MODE|^DEMO_MODE|if DEMO_MODE' {} \;Repository: NVIDIA/ai-cloud-validation
Length of output: 16092
🏁 Script executed:
#!/bin/bash
set -eu
file="isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.py"
printf '%s\n' '--- imports and demo references ---'
rg -n '^(import|from) |DEMO_MODE|ISVCTL_DEMO_MODE|kubectl|def main' "$file"
printf '%s\n' '--- main body ---'
sed -n '230,330p' "$file"
printf '%s\n' '--- sibling convention with filenames ---'
fd -e py . isvctl/configs/providers/my-isv/scripts -0 |
xargs -0 rg -l 'ISVCTL_DEMO_MODE' |
head -n 8 |
while IFS= read -r sibling; do
printf '\n%s\n' "$sibling"
rg -n -A8 -B2 'DEMO_MODE = os\.environ\.get\("ISVCTL_DEMO_MODE"\) == "1"|if DEMO_MODE' "$sibling" | head -n 24
done
printf '%s\n' '--- static control-flow check ---'
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.py")
tree = ast.parse(path.read_text())
main = next(node for node in tree.body if isinstance(node, ast.FunctionDef) and node.name == "main")
names = {
node.id
for node in ast.walk(main)
if isinstance(node, ast.Name)
}
calls = [
ast.unparse(node.func)
for node in ast.walk(main)
if isinstance(node, ast.Call)
]
print("main references DEMO_MODE:", "DEMO_MODE" in names)
print("main calls kubectl-related helpers:", [call for call in calls if "kubectl" in call.lower()])
print("main first statement:", ast.unparse(main.body[0]))
PYRepository: NVIDIA/ai-cloud-validation
Length of output: 14944
Restore the ISVCTL_DEMO_MODE gate.
When ISVCTL_DEMO_MODE=1, return dummy success before _kubectl_command(). Without this gate, demo mode can cordon a real node and create probe pods in a live cluster. Keep the implemented workflow for real runs.
🤖 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 `@isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.py` around lines
238 - 254, Restore the ISVCTL_DEMO_MODE=1 check at the start of main, before
_kubectl_command() is called, and return the expected dummy successful
provider-neutral result without performing cluster operations. Preserve the
existing validation and cordon workflow for non-demo runs.
Source: Coding guidelines
2 participants
Summary
my-isvBFX01-04 placeholder with a real, reversible Kubernetes cordon workflowWhy
PR #562 introduces the BFX01-04 validation contract but leaves the provider script as a dummy-success scaffold. This implements the actual Kubernetes behavior required by #209.
This PR is stacked on #562 because the break-fix validation and provider wiring do not exist on
mainyet.Impact
Providers using the
my-isvKubernetes configuration now get evidence-backed BFX01-04 results instead of a placeholder. The workflow requires permission to create/delete pods and cordon/uncordon nodes.Validation
true18 passedmake test:1505 passed,58 passed,1323 passedwith175 deselected, and124 passedCloses #209
Summary by CodeRabbit
New Features
Bug Fixes
Tests