Skip to content

ci: fail the contract suite with a named cause when no disposable node exists - #215

Merged
hellices merged 8 commits into
mainfrom
fix/contract-workload-pool
Aug 7, 2026
Merged

ci: fail the contract suite with a named cause when no disposable node exists#215
hellices merged 8 commits into
mainfrom
fix/contract-workload-pool

Conversation

@hellices

@hellices hellices commented Aug 7, 2026

Copy link
Copy Markdown
Owner

The K8s contract tests have failed on every main commit since 2026-08-05 — 6 of 21 tests red, most recently on c5d14cc2:

FAILED tests/contract/test_node_ops_contract.py::test_preview_cordon_causes_no_persistent_mutation
  Failed: no disposable workload node found — refusing to touch system nodes
FAILED tests/contract/test_resize_contract.py::test_execute_resize_mutates_exactly_once
  Failed: timed out after 180s: pod resize-execute should reach Running

Nothing is wrong with the tests or the product. The cluster had no node labelled korvid.dev/disposable=true, and that label is the only place the suite may schedule — the node-operation tests fail closed rather than fall back to a system node. That guard is working as designed; it is why the symptom is a loud failure instead of a destructive fallback onto system nodes.

The gap is that the workflow started the cluster but never checked it had somewhere to schedule, so the suite stayed red for three days and eight commits behind six failures spread across four files, none of which names the cause.

What this does

Waits for a Ready node carrying korvid.dev/disposable=true before installing anything, and fails with a named cause and the repair command if none appears within five minutes.

It checks; it does not repair. The workflow identity holds agentPools/read and no write, so it cannot scale the pool — and granting write so it could self-heal would also let this job reshape the system pool that every other guard here exists to protect. Not worth it to save one command.

It asks the cluster, not Azure. This started as a check on agentPools[].count, which turned out not to be evidence: a stopped cluster reports that count as 0 for the workload pool even when it is populated (observed directly — stopped reads 0, starting the same cluster reads 1, with no scale operation in between), and az aks start returns before nodes finish registering. A count-based guard would have failed healthy runs. kubectl wait on the label asks for exactly what the tests need.

Verification

The contract workflow cannot run from a PR branch — Branch "fix/contract-workload-pool" is not allowed to deploy to aks-contract-test due to environment protection rules, which is the protection working as designed. I did not work around it, so the CI-side proof waits for merge.

Locally, against the real cluster:

  1. Restored the missing node, then ran the suite exactly as CI does (pytest -p no:randomly -m contract tests/contract/): 21/21 passed, including the 6 red on main.
  2. Guard passes with the disposable node Ready; fails with the named error when the selector matches nothing.
  3. The repair command printed in the error message restores the pool as written.

The cluster is left Running with a workload node so the post-merge contract run has somewhere to schedule; it stops itself at the end of that run as usual.

The K8s contract tests have failed on every main commit since 2026-08-05,
6 of 21 tests red:

  no disposable workload node found — refusing to touch system nodes
  timed out after 180s: pod should be scheduled

Nothing was wrong with the tests or the product. The `workload` node pool
was at zero nodes, and that pool is the only place the suite may schedule:
its nodes carry korvid.dev/disposable=true, and the node-operation tests
fail closed rather than fall back to a system node — correct behaviour, and
the reason the symptom is a hard failure rather than a dangerous fallback.

The workflow started and stopped the cluster but never checked that it had
capacity, so once something scaled the pool to zero (a perf run, a cost
sweep, a cleanup) nothing put it back and the suite stayed red for days.

Scale the pool to one node when it is found empty, before fetching
credentials. The pool name is a fixed literal rather than a repository
variable so drifting settings cannot aim the scale-up at the system pool.

Verified against the real cluster: scaled the pool to zero, ran the new
step verbatim, watched it report `workload was at 0` and restore one node;
re-ran with the node present and confirmed it is a no-op. Full contract
suite then passed 21/21 locally against the restored cluster.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings August 7, 2026 03:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Attempts to restore AKS contract-test capacity before running the suite.

Changes:

  • Adds a fixed workload node-pool identifier.
  • Scales an empty workload pool to one node.
  • Documents the recovery behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
.github/workflows/k8s-contract.yml Adds workload-pool restoration.
docs/dev/contract-tests.md Documents automatic restoration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/k8s-contract.yml Outdated
@hellices

hellices commented Aug 7, 2026

Copy link
Copy Markdown
Owner Author

Tried to prove this by dispatching the contract workflow on this branch. It was rejected, correctly:

Branch "fix/contract-workload-pool" is not allowed to deploy to aks-contract-test
due to environment protection rules.

That is the protection working as designed — only main may reach the cluster identity — so the CI-side proof has to wait for merge. I did not work around it.

Instead I reproduced the exact failing sequence locally against the real cluster, in workflow order:

  1. Scaled workload to 0 — the state main has been in since 2026-08-05.
  2. Ran the new step verbatim: ::notice title=Scaling workload pool::workload was at 0; scaling to 1 for the contract suite. → pool back to 1, node Ready with korvid.dev/disposable=true.
  3. Ran the janitor step: deleted 0 stale namespace(s), uncordoned 0 node(s).
  4. Ran the suite exactly as CI does (pytest -p no:randomly -m contract tests/contract/): 21 passed in 35.21s, including the 6 that have been red on main.

Also confirmed the step is a no-op when the node is already there, so it does not churn the pool on every run.

Review caught that the scale-up could never work: the workflow identity holds
a deliberately minimal role with agentPools/read and no write, so
`az aks nodepool scale` returns 403 in CI. My local verification ran under my
own credentials and so proved nothing about the OIDC identity — confirmed by
reading the deployed role, which lists read/start/stop/listClusterUserCredential
/agentPools/read and nothing else.

Granting write would fix the symptom and widen the blast radius: the same
permission that scales the workload pool also reshapes the system pool, which
every guard in this workflow and every node-operation test exists to keep out
of reach. Not worth it to save one command.

Check and fail instead, with the pool named and the repair command spelled
out. An empty pool is drift, not a setting — cost at rest is handled by
stopping the cluster, and nothing in the repo scales this pool down — so the
right response is a loud, diagnosable failure rather than silent repair.

Verified both paths against the real cluster: at one node the guard passes; at
zero it emits the error and would exit 1; and the command in the error message
restores the pool as written.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@my-reviewer-agent my-reviewer-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

리뷰 요약

workload 풀이 0일 때 contract suite가 no disposable workload node found / 파드 스케줄링 타임아웃으로 원인을 알 수 없게 red로 남는다는 진단은 타당하고, 검사 위치(클러스터 start 직후, 크리덴셜 취득·의존성 설치 전)도 적절합니다. 특히 최신 커밋 51eba57에서 자동 스케일업을 포기하고 fail-fast 가드로 바꾼 결정은 정확합니다 — 워크플로 아이덴티티는 agentPools/read 만 보유하므로 이전 커밋(4535f75)의 az aks nodepool scale 은 정작 복구가 필요한 0-노드 경로에서 AuthorizationFailed 로 죽었을 것이고, 이를 위해 역할에 write 를 부여하면 같은 워크플로가 system 풀까지 재구성할 수 있게 됩니다. 이 변경으로 Copilot 리뷰(#4879746123)의 권한 지적과 문서-역할 불일치는 모두 해소되었습니다. 에러 메시지에 정확한 복구 명령을 담은 것도 좋습니다.

남은 지적 두 가지는 아래 인라인 코멘트를 참고해 주세요. 둘 다 blocker 는 아닙니다.

검토 항목별 정리

  • 보안: 새 단계는 read-only 이고 데이터 플레인 크리덴셜 취득 이전에 실행되며, 대상 풀 이름이 고정 리터럴이라 destructive 범위가 넓어지지 않습니다. 기존 EXPECTED_* 가드 스타일과 일관됩니다.
  • 버그: 노드가 1 이상이면 완전한 no-op 이며 부작용이 없습니다. 실패 시 contract 잡이 죽어도 stop-cluster 잡이 if: always() 로 클러스터를 정지시키므로 과금 회귀는 없습니다.
  • 성능: az aks nodepool show 1회 호출로, 실패 케이스에서는 오히려 uv sync/helm 설치/30분 스위트 실행 전에 중단되어 러너 시간을 절약합니다.
  • 에러 처리: count 가 숫자가 아닐 때의 경로만 미흡합니다(아래 finding 1).
  • 테스트: 워크플로 단계라 유닛 테스트 대상이 아니며, 문서에 재현 증상과 복구 절차가 기록되어 근거로 충분합니다.

COMMENT

Comment thread .github/workflows/k8s-contract.yml Outdated
Comment thread .github/workflows/k8s-contract.yml Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

.github/workflows/k8s-contract.yml:43

  • The workflow no longer performs the “scale-up” described here, and the PR title, Fix section, and verification steps still promise that an empty pool is restored automatically. The implementation now intentionally checks and fails with a manual repair command. Please rename/update the PR metadata and this comment so operators are not told that merging this change will self-heal the recurring zero-node condition.
      # The only pool the suite may schedule onto; its nodes carry
      # korvid.dev/disposable=true. Fixed rather than a variable so a drifting
      # setting cannot point the scale-up at the system pool.

Chasing the review notes about the unquoted count comparison turned up
something worse: the count itself is not evidence. A stopped cluster reports
agentPools[].count as 0 for the workload pool even when the pool is populated
— observed directly, stopped reads 0 and starting the same cluster reads 1
with no scale operation in between. `az aks start` also returns before nodes
finish registering, so a count read just after it can be 0 for a healthy
cluster. The guard I wrote would have failed good runs.

Ask the cluster for the thing the tests actually need instead: a Ready node
labelled korvid.dev/disposable=true, polled with kubectl wait for up to five
minutes so a slow start is tolerated. That is immune to the reporting quirk,
needs no extra Azure permission, and dumps node labels on failure so the next
person can see what the cluster really had.

Also fixes the review findings that led here: the empty-string comparison
that died with "integer expression expected" is gone with the count check,
the step now runs under set -euo pipefail, and the WORKLOAD_NODEPOOL comment
no longer describes a scale-up this workflow never performs.

Verified on the real cluster: passes with the disposable node Ready, and
fails with the named error when the selector matches nothing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@hellices

hellices commented Aug 7, 2026

Copy link
Copy Markdown
Owner Author

Updated the PR description to match what the code now does — it checks and fails, it does not restore the pool automatically. The original title and "Fix" section promised auto-repair, which was correct for 4535f75 and wrong for everything after it.

@hellices hellices changed the title ci: restore the disposable workload pool before the contract suite ci: fail the contract suite with a named cause when no disposable node exists Aug 7, 2026
@hellices
hellices requested a balanced review from Copilot August 7, 2026 04:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

.github/workflows/k8s-contract.yml:118

  • All kubectl wait failures are discarded and treated as “no Ready node.” An expired login, RBAC denial, or API connectivity failure will therefore retry and end with the new empty-pool diagnosis and scale hint, even though scaling cannot fix it. Capture/query the node list separately and fail with the underlying error; retry only a genuine no-match or readiness timeout.
            if kubectl wait --for=condition=Ready node \
              -l korvid.dev/disposable=true --timeout=10s >/dev/null 2>&1; then

.github/workflows/k8s-contract.yml:119

  • This selector differs from the scheduler constraint used by the workload and resize contracts: tests/contract/conftest.py:153,177 selects korvid.dev/pool=workload, while node operations discover korvid.dev/disposable=true. A Ready node carrying only the latter lets this guard pass while test pods still time out. Require both labels on the same Ready node (and align the success/error diagnostics), or make the contracts use one canonical label.
            if kubectl wait --for=condition=Ready node \
              -l korvid.dev/disposable=true --timeout=10s >/dev/null 2>&1; then
              kubectl get nodes -l korvid.dev/disposable=true

.github/workflows/k8s-contract.yml:123

  • This loop does not enforce the advertised five-minute deadline. Once a matching node has registered but remains NotReady, each iteration can spend 10 seconds in kubectl wait and then another 10 seconds sleeping, so the failure may take roughly ten minutes while the annotation reports five. Use a wall-clock deadline so both the no-node and NotReady paths have the same bound.

This issue also appears in the following locations of the same file:

  • line 117
  • line 117
          for _ in $(seq 1 30); do
            if kubectl wait --for=condition=Ready node \
              -l korvid.dev/disposable=true --timeout=10s >/dev/null 2>&1; then

@my-reviewer-agent my-reviewer-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

리뷰 요약

51eba57 대비 이번 커밋(7e265a4)은 이전 리뷰에서 지적한 두 가지를 정확히 해결했습니다. az aks nodepool show --query count 기반 판정을 완전히 제거해 [ "" -lt 1 ] 정수 비교 크래시 경로가 사라졌고, WORKLOAD_NODEPOOL 주석도 실제 동작(복구 힌트 출력 전용)과 일치하게 갱신되었습니다. 또한 판정 근거를 컨트롤 플레인의 count 에서 클러스터가 실제로 스케줄 가능하다고 보고하는 Ready 노드로 옮긴 것은 더 나은 설계입니다 — az aks start 가 노드 등록 완료 전에 반환하는 레이스를 대기 루프가 흡수하고, 가드 위치도 크리덴셜 취득 직후·uv sync/helm 설치 이전이라 실패 시 러너 시간을 절약합니다. set -euo pipefail 추가도 좋습니다.

다만 새로 들어온 대기 루프 자체에 세 가지 지적이 있습니다. 아래 인라인 코멘트를 참고해 주세요. 1번은 가드의 목적 자체를 무력화할 수 있어 가장 중요합니다.

검토 항목별 정리

  • 버그: 가드가 검사하는 라벨(korvid.dev/disposable=true)과 테스트 파드가 실제로 요구하는 라벨(korvid.dev/pool=workload)이 다릅니다(finding 1). 두 라벨이 항상 같은 노드에 붙는다는 보장이 코드에 없어, 가드는 통과하는데 파드는 스케줄 타임아웃으로 죽는 — 이 PR 이 없애려던 바로 그 "원인 없는 red" 가 재현될 수 있습니다.
  • 에러 처리: kubectl wait 의 stderr 를 전부 버려 모든 실패를 "노드 없음" 으로 단정합니다(finding 3). 토큰 만료·RBAC 거부·API 서버 접속 불가도 5분 대기 후 "pool 을 scale 하세요" 라는 틀린 복구 안내로 끝납니다.
  • 성능: 실패 경로의 실제 소요가 문서/에러 메시지가 약속한 5분이 아니라 최대 약 10분입니다(finding 2).
  • 보안: 새 단계는 read-only 이며, 스케일 권한을 요구하지 않는 결정과 고정 리터럴 풀 이름 사용은 그대로 타당합니다. 회귀 없음.
  • 테스트: 워크플로 스텝이라 유닛 테스트 대상은 아니고, docs/dev/contract-tests.md 갱신으로 증상·복구 절차가 문서화되어 근거는 충분합니다. 다만 finding 1 을 반영하면 문서의 라벨 서술도 함께 맞춰 주세요.

COMMENT

Comment thread .github/workflows/k8s-contract.yml Outdated
Comment thread .github/workflows/k8s-contract.yml Outdated
Comment thread .github/workflows/k8s-contract.yml Outdated
Three review findings on the wait loop, all of which left a way for the guard
to be green while the suite is red, or to blame the wrong thing:

The selector only asked for korvid.dev/disposable=true, but every test pod in
tests/contract/conftest.py carries a korvid.dev/pool=workload nodeSelector.
Those labels sharing a node is a convention of the pool setup, not something
this workflow enforced, so label drift would pass the guard and leave pods
Pending — exactly the undiagnosable failure being removed. Both are now
required on the same node.

The budget was a loop count, so a registered-but-NotReady node spent each
attempt on its own 10s timeout plus a 10s sleep: 10 minutes against an error
message promising 5. Replaced with a wall-clock deadline, which holds for both
paths.

Every kubectl failure was swallowed and read as "no node", so an expired
token, an RBAC denial, or an unreachable API server would burn the deadline
and then advise a scale command that cannot help. The node list is now proven
reachable once up front, and its stderr is reported.

Verified on the real cluster: passes with both labels present; fails within
the deadline when one label drifts; and an unreachable API server fails in one
second naming the connection error rather than the pool.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

.github/workflows/k8s-contract.yml:143

  • The repair command will normally be run after this failed job completes, but the stop-cluster job runs with if: always() and leaves AKS stopped. Azure permits only start/delete operations on a stopped cluster, so az aks nodepool scale then fails. Make the hint start the cluster before scaling and stop it afterward (ideally saying to wait for the cleanup job first).
          echo "::error title=No schedulable disposable node::$AKS_CLUSTER_NAME has no Ready node matching $SELECTOR after 5 minutes, so the node-operation and resize contracts have nowhere to schedule. If the $WORKLOAD_NODEPOOL pool is empty, restore it with: az aks nodepool scale -g $AKS_RESOURCE_GROUP --cluster-name $AKS_CLUSTER_NAME -n $WORKLOAD_NODEPOOL --node-count 1"

.github/workflows/k8s-contract.yml:137

  • This waits for every matching node to become Ready, not for any one matching node. If the workload pool has one healthy node and one NotReady node, the guard fails after five minutes even though the suite has the Ready disposable node it requires. Query the matching nodes' Ready conditions and succeed when at least one is True.

This issue also appears on line 143 of the same file.

            if kubectl wait --for=condition=Ready node -l "$SELECTOR" \
              --timeout=10s >/dev/null 2>&1; then

docs/dev/contract-tests.md:64

  • This documented repair command is incomplete because every workflow run stops the cluster, including a preflight failure. AKS does not allow node-pool scaling while stopped, so following these instructions after the run fails will not repair the pool. Document the required start and final stop around the scale operation.
az aks nodepool scale -g rg-korvid-contract-test \
  --cluster-name aks-korvid-contract-test -n workload --node-count 1

@my-reviewer-agent my-reviewer-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

리뷰 요약

7e265a4 대비 이번 커밋(e94823a)은 직전 리뷰의 세 지적을 모두 정확히 해결했습니다. 셀렉터가 korvid.dev/disposable=true,korvid.dev/pool=workload 로 합쳐져 "가드는 통과하는데 파드는 Pending" 경로가 닫혔고(.github/workflows/k8s-contract.yml:120), kubectl get nodes -o name 사전 확인이 추가되어 토큰 만료·RBAC 거부·API 서버 접속 불가를 "빈 풀" 로 오진하지 않게 되었으며(:126-129), 반복 횟수 대신 deadline=$(( SECONDS + 300 )) 벽시계 예산을 써서 NotReady 노드가 있을 때 실제 소요가 약속한 5분을 넘어 두 배로 늘어나던 문제도 없어졌습니다(:134-135). 가드 위치(크리덴셜 취득 직후, uv sync/helm 설치 이전)와 read-only 유지 결정도 그대로 타당합니다.

남은 지적 두 가지는 아래 인라인 코멘트를 참고해 주세요. 둘 다 blocker 는 아니지만, 2번은 이 PR 이 제공하려는 "이름 붙은 원인 + 복구 명령" 의 후반부가 실제로는 그대로 실행되지 않는다는 점에서 실용적 가치에 직접 영향을 줍니다.

검토 항목별 정리

  • 버그: kubectl wait 는 셀렉터에 매칭되는 모든 노드가 Ready 가 될 때까지 기다립니다. 워크로드 풀에 Ready 1대 + NotReady 1대가 있으면 스위트가 필요한 노드는 이미 있는데도 5분 뒤 실패합니다(finding 1).
  • 에러 처리: 실패 메시지가 안내하는 복구 명령이 그 시점 클러스터 상태에서 실행 불가입니다(finding 2). stop-cluster 잡이 if: always()(:198-200)라 가드 실패 직후에도 클러스터가 정지되고, 정지된 AKS 에서는 nodepool scale 이 거부됩니다.
  • 보안: 새 단계는 read-only 이고, 스케일 권한을 요구하지 않는 대신 수동 복구를 안내하는 결정과 고정 리터럴 풀 이름 사용은 그대로 옳습니다. 회귀 없음.
  • 성능: 실패 경로가 uv sync/helm 설치/30분 스위트 이전에 5분 안에 끊기므로 러너 시간을 절약합니다. 성공 경로는 대개 첫 kubectl wait 한 번으로 끝나는 no-op 입니다.
  • 테스트: 워크플로 스텝이라 유닛 테스트 대상이 아니며, docs/dev/contract-tests.md 에 증상·판정 근거·복구 절차가 기록되어 근거로 충분합니다. 다만 finding 2 를 반영하면 문서의 복구 스니펫(docs/dev/contract-tests.md:63-66)도 함께 맞춰 주세요.

COMMENT

Comment thread .github/workflows/k8s-contract.yml Outdated
Comment thread .github/workflows/k8s-contract.yml Outdated
Two more ways the guard could report the wrong cause.

`kubectl wait` requires *every* node matching the selector to satisfy the
condition, but the suite needs only one somewhere to schedule. A second pool
node, or one cycling through an image upgrade, would have burned the whole
deadline and announced "no schedulable disposable node" with a healthy one
sitting right there. Count Ready nodes among the matches instead.

The repair command also could not run as printed. The stop-cluster job is
`if: always()`, so it stops the cluster even when this guard fails, and Azure
rejects everything except start on a stopped cluster — verified: `az aks
nodepool scale` on the stopped cluster returns "Managed Cluster is in stopped
state, no operations except for start are allowed". An operator pasting the
hint got an error instead of a fix, which is the same dead end this step
exists to remove. The hint is now start, scale, stop, and says why.

Also verified that `az aks start` refuses on a running cluster, so the
sequence is documented as the post-cleanup recovery rather than something to
run mid-flight.

Verified on the real cluster: passes in 3s with the node Ready, honours the
deadline on label drift, and the printed sequence executes end to end.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@my-reviewer-agent my-reviewer-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

리뷰 요약

e94823a 대비 이번 커밋(db2881c)은 직전 리뷰의 finding 1·2 를 정확히 해결했습니다. kubectl wait 가 셀렉터에 매칭되는 모든 노드의 Ready 를 요구하던 문제는 kubectl get nodes -o jsonpath + grep -q '=True$' 로 바뀌면서 "하나라도 Ready 면 통과" 의미가 되었고(.github/workflows/k8s-contract.yml:138-140), 실패 메시지와 문서의 복구 절차에 az aks start / az aks stop 이 앞뒤로 붙어 정지된 클러스터에서 실행 불가하던 안내도 실행 가능해졌습니다(:143, docs/dev/contract-tests.md:60-70). 루프 종료 조건을 본문 뒤로 옮겨 최소 1회는 반드시 검사하도록 한 것도 맞는 변경입니다.

남은 지적은 아래 인라인 코멘트 1건입니다. blocker 는 아니지만, 이 PR 이 없애려는 "원인 없는 red" 가 잔존 경로로 재현될 수 있다는 점에서 확인이 필요합니다.

검토 항목별 정리

  • 버그: Ready=True 는 스케줄 가능을 뜻하지 않습니다(finding 1). 스텝 이름은 a schedulable disposable node must exist 인데 판정은 Ready 조건만 봅니다.
  • 에러 처리: 루프 진입 전 kubectl get nodes -o name 사전 확인(:126-129)으로 토큰 만료·RBAC 거부·API 접속 불가를 "빈 풀" 로 오진하지 않게 한 설계는 그대로 유효합니다. 루프 내부의 2>/dev/null 은 사전 확인이 통과한 뒤라 잔여 위험이 작습니다.
  • 성능: 실패 경로가 uv sync/helm 설치/30분 스위트 이전에 5분 벽시계 예산으로 끊깁니다. 성공 경로는 API 1회 호출 no-op 입니다.
  • 보안: read-only 유지, 고정 리터럴 풀 이름, 크리덴셜 취득 직후 배치 — 회귀 없음.
  • 테스트: 워크플로 스텝이라 유닛 테스트 대상이 아니며, docs/dev/contract-tests.md 에 증상·판정 근거·복구 절차가 일치하게 갱신되어 근거로 충분합니다.

COMMENT

Comment thread .github/workflows/k8s-contract.yml Outdated
The step promised "schedulable" but only checked Ready, so a cordoned node
passed it. That is not hypothetical: the janitor sweeps cordons precisely
because a crashed node-operation test leaves one behind, and the janitor runs
*after* this guard. A run following a crashed one would have gone green here
and then timed out with every test pod Pending — the unnamed red this step
exists to remove, reintroduced by the step itself.

Reproduced it: cordoned the workload node, and the previous check reported the
node as fine.

Require spec.unschedulable to be unset alongside Ready, and say so in the
failure message so an operator knows to uncordon rather than reach for the
pool. Failure output is now `-o wide --show-labels` as well.

Verified on the real cluster: cordoned node rejected, same node accepted after
uncordon.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

.github/workflows/k8s-contract.yml:148

  • The polling query still collapses API/authentication failures into “no schedulable node”: 2>/dev/null hides the diagnostic, and a nonzero kubectl status merely makes the if false even with pipefail. The one preflight request only proves reachability at that instant, so a later failure waits out the deadline and prints the misleading pool-scaling repair. Capture and validate each kubectl result before grepping it.
            if kubectl get nodes -l "$SELECTOR" \
              -o jsonpath='{range .items[*]}{.metadata.name}{"=ready:"}{range .status.conditions[?(@.type=="Ready")]}{.status}{end}{" unschedulable:"}{.spec.unschedulable}{"\n"}{end}' \
              2>/dev/null | grep -q '=ready:True unschedulable:$'; then

Comment thread .github/workflows/k8s-contract.yml Outdated

@my-reviewer-agent my-reviewer-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

리뷰 요약

db2881c 대비 이번 커밋(6d8a26d)은 판정 조건에 unschedulable 을 추가해 "Ready = 스케줄 가능" 이 아니라는 직전 리뷰의 finding 1 을 정확히 해결했습니다. jsonpath 를 =ready:True unschedulable: 형태로 확장하고 grep -q '=ready:True unschedulable:$'cordon 되지 않은 Ready 노드가 하나라도 있으면 통과 하는 의미를 유지한 점, 실패 시 진단 출력을 kubectl get nodes -o wide --show-labels 로 넓힌 점, 에러 메시지와 docs/dev/contract-tests.md 서술을 동시에 갱신한 점 모두 적절합니다. 스텝 이름(a schedulable disposable node must exist)과 실제 판정이 이제 일치합니다.

다만 이 변경이 새로 만들어내는 운영상 회귀가 1건 있습니다. 아래 인라인 코멘트를 참고해 주세요.

검토 항목별 정리

  • 버그: cordon 판정을 추가했지만, cordon 을 실제로 해제하는 janitor(tests/contract/janitor.py:46 _sweep_cordons)는 이 가드보다 (.github/workflows/k8s-contract.yml:170-171)에서 실행됩니다. 지금까지 자동 복구되던 상태가 5분 대기 후 수동 개입 요구로 바뀝니다(finding 1).
  • 에러 처리: 루프 진입 전 kubectl get nodes -o name 사전 확인(:126-129)으로 토큰 만료·RBAC 거부를 "빈 풀" 로 오진하지 않게 한 설계는 그대로 유효합니다. 루프 내부 2>/dev/null 의 잔여 위험(Copilot 리뷰 지적)은 사전 확인 이후라 낮게 봅니다.
  • 성능: 성공 경로는 API 1회 호출 no-op, 실패 경로는 uv sync/helm 설치/30분 스위트 이전에 5분 벽시계 예산으로 끊깁니다. 회귀 없음.
  • 보안: read-only 유지, 고정 리터럴 풀 이름(WORKLOAD_NODEPOOL: workload), 크리덴셜 취득 직후 배치 — 회귀 없음.
  • 테스트: 워크플로 스텝이라 유닛 테스트 대상이 아니며, 문서에 unschedulable 검사 이유까지 기록되어 근거는 충분합니다.

Comment thread .github/workflows/k8s-contract.yml
Both reviewers landed on the same flaw in my last commit from opposite
directions, and they were right: adding the cordon check in front of the
janitor converted a self-healing state into a hard failure.

`_sweep_cordons` exists to uncordon disposable nodes left behind by a crashed
node-ops test, and it did fix those runs silently. With the guard ahead of it,
the same situation now spent five minutes and exited, killing the job before
the janitor could run — and every later run stayed blocked until a human
intervened. The goal is removing unnamed reds, not reds that used to fix
themselves.

Moved the guard after the janitor. It cannot move earlier instead: the janitor
is a Python entry point that needs `uv sync` first. Cost is that a genuinely
broken cluster now wastes the dependency install before failing, which is
worth it — and a cluster too broken to reach fails at the janitor with its own
error anyway.

Verified end to end: cordoned the node, ran the janitor, watched it report
`uncordoned 1 node(s)`, and the guard then passed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

docs/dev/contract-tests.md:48

  • This timing claim contradicts the workflow: uv sync, Helm installation, and the janitor all run before this guard (.github/workflows/k8s-contract.yml:96-113). Describe it as running before the contract suite rather than before installing anything.
`korvid.dev/disposable=true` and `korvid.dev/pool=workload` before installing
anything, and fails with a named cause and the repair sequence if none appears

@my-reviewer-agent my-reviewer-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

리뷰 요약

6d8a26d 대비 이번 커밋(ec9799c)은 직전 리뷰의 finding 1(cordon 판정과 janitor 실행 순서의 충돌)을 실행 순서 자체를 바꿔서 해결했습니다. 가드가 Janitor — remove stale fixtures from interrupted runs(.github/workflows/k8s-contract.yml:110-111) 뒤인 :113 으로 이동해, janitor 의 _sweep_cordons 가 고칠 수 있는 cordon 은 그대로 자동 복구되고 가드에 남는 것은 janitor 가 고치지 못한 상태뿐이라는 의미가 성립합니다. 주석(:132-136)과 문서(docs/dev/contract-tests.md:53-55)에 그 근거까지 명시한 점도 좋습니다. 판정 로직(=ready:True unschedulable:$ 로 "하나라도 스케줄 가능하면 통과"), 5분 벽시계 예산, 루프 진입 전 kubectl get nodes -o name 도달성 사전 확인, read-only 유지 및 고정 리터럴 WORKLOAD_NODEPOOL: workload 는 모두 그대로 유지되어 회귀가 없습니다.

다만 가드를 뒤로 옮기면서 문서의 타이밍 서술이 실제 워크플로와 어긋나게 되었습니다. 아래 인라인 코멘트 1건을 참고해 주세요. blocker 는 아닙니다.

검토 항목별 정리

  • 버그: 판정 로직에 새 결함은 없습니다. deadline=$(( SECONDS + 300 )) 후 본문 검사 → 종료 조건 → sleep 5 순서라 최소 1회 검사가 보장되고, 성공 시 exit 0 으로 즉시 빠져나옵니다.
  • 에러 처리: 루프 진입 전 도달성 확인(:140-143)이 토큰 만료·RBAC 거부·API 접속 불가를 "빈 풀" 로 오진하지 않게 막아 줍니다. 루프 내부 2>/dev/null(:150)은 이 사전 확인 이후라 잔여 위험이 낮다는 이전 판단을 유지합니다.
  • 성능: 가드가 janitor 뒤로 이동해 실패 시 절약되는 러너 시간이 줄었지만(uv sync/helm 설치가 앞서 실행됨), 30분짜리 contract suite 이전에 5분 안에 끊는다는 핵심 이득은 그대로이고 cordon 자동 복구를 살리기 위한 합리적 교환입니다. 성공 경로는 API 1회 호출 no-op 입니다.
  • 보안: read-only 유지, 스케일 권한 요구 없음, 대상 풀 이름 고정 리터럴 — 회귀 없음.
  • 테스트: 워크플로 스텝이라 유닛 테스트 대상이 아니고, docs/dev/contract-tests.md 에 증상·판정 근거·복구 절차가 기록되어 근거는 충분합니다.

COMMENT

Comment thread docs/dev/contract-tests.md Outdated
Moving the guard behind the janitor made this sentence false, and it
contradicted the next sentence in its own paragraph, which correctly says the
guard runs after the janitor sweep. The real order is setup-uv, uv sync, helm,
janitor, guard.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@my-reviewer-agent my-reviewer-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

리뷰 요약

ec9799c 대비 이번 커밋(a35ca2b)은 문서 한 문장만 수정한 변경으로, 직전 리뷰에서 남긴 유일한 지적(가드를 janitor 뒤로 옮기면서 문서의 타이밍 서술이 실제 워크플로와 어긋난 문제)을 정확히 해결했습니다.

  • docs/dev/contract-tests.md:45-46before installing anythingbefore running the suite. 실제 워크플로에서 uv sync·Helm 설치·janitor 는 가드(.github/workflows/k8s-contract.yml:113)보다 에서 실행되므로, 새 문구가 사실과 일치합니다.

워크플로 파일에는 변경이 없고, 이전에 확인한 설계는 그대로 유지됩니다.

검토 항목별 정리

  • 버그: 신규 결함 없음. 판정 로직(=ready:True unschedulable:$ 로 "하나라도 스케줄 가능하면 통과"), deadline=$(( SECONDS + 300 )) 벽시계 예산, 최소 1회 검사 보장 순서 모두 이번 커밋에서 손대지 않았습니다.
  • 보안: read-only 유지, 스케일 권한 요구 없음, 대상 풀 이름 고정 리터럴(WORKLOAD_NODEPOOL: workload) — 회귀 없음.
  • 성능: 문서 전용 변경으로 런타임 영향 없음.
  • 에러 처리: 루프 진입 전 kubectl get nodes -o name 도달성 사전 확인과 실패 시 az aks start → nodepool scale → az aks stop 복구 시퀀스 안내가 그대로 유지되고, 문서의 복구 스니펫과도 일치합니다.
  • 테스트: 워크플로 스텝이라 유닛 테스트 대상이 아니며, 문서에 증상·판정 근거(양쪽 라벨 동시 요구, unschedulable 검사, janitor 이후 실행)·복구 절차가 모두 일치하게 기록되어 근거로 충분합니다.

미해결 finding 없음.


APPROVE

@hellices
hellices merged commit ec19f4d into main Aug 7, 2026
10 checks passed
@hellices
hellices deleted the fix/contract-workload-pool branch August 7, 2026 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants