Skip to content

Commit 30cb428

Browse files
committed
test(k8s): update stale StatefulSet/Job selective-release tests to assert refuse behavior
These three tests predated the deliberate refuse-subset-release change and were never exercised until the k8s CI leg was enabled on this branch. StatefulSet handler (tests 1 & 2): the handler now raises K8sError when the caller-supplied victims are not the top-of-ordinal-stack set, rather than logging a warning and proceeding with the scale patch. Silently evicting the wrong pods would cause data loss. Updated: - test_release_hosts_selective_non_highest_ordinal_warns_and_still_scales → test_release_hosts_selective_non_highest_ordinal_refused - test_release_hosts_selective_unparseable_victim_names_warns → test_release_hosts_selective_unparseable_victim_names_refused Both now assert pytest.raises(K8sError) and confirm no scale patch is issued. Job handler (test 3): the "selective release not supported" info message the test expected no longer exists. When parallelism is not recorded in provider_data the handler logs "Kubernetes job release: ... (deleting whole Job)" and deletes the Job. Updated the assertion to match the actual log format ("job release") and clarified the docstring to describe the no-parallelism fallback path being exercised. No src/ changes.
1 parent 71870d4 commit 30cb428

2 files changed

Lines changed: 55 additions & 51 deletions

File tree

tests/providers/k8s/unit/handlers/test_statefulset_handler.py

Lines changed: 37 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@
88
* ``release_hosts`` selective path: when the victims are the top-of-stack
99
ordinals, no warning is emitted and ``spec.replicas`` is patched down
1010
by the victim count. Pods are never deleted directly.
11-
* ``release_hosts`` ordinal caveat: when victims include non-highest
12-
ordinals, a WARNING is logged and the replicas patch still proceeds
13-
(the controller will pick highest ordinals to evict).
11+
* ``release_hosts`` ordinal caveat: when victims are NOT the top-of-stack
12+
ordinals, a ``K8sError`` is raised and no scale patch is issued.
13+
Silently evicting different pods would cause data loss.
1414
* ``release_hosts`` full-release path: ``spec.replicas`` is patched to
1515
zero and the StatefulSet is deleted.
1616
* ``check_hosts_status`` reads both the pod list and the StatefulSet
@@ -31,6 +31,7 @@
3131
from orb.domain.request.value_objects import RequestId, RequestType
3232
from orb.domain.template.template_aggregate import Template
3333
from orb.providers.k8s.configuration.config import K8sProviderConfig
34+
from orb.providers.k8s.exceptions.k8s_errors import K8sError
3435
from orb.providers.k8s.infrastructure.handlers.statefulset_handler import K8sStatefulSetHandler
3536

3637
# ---------------------------------------------------------------------------
@@ -246,11 +247,15 @@ async def test_release_hosts_selective_highest_ordinal_no_warning() -> None:
246247

247248

248249
@pytest.mark.asyncio
249-
async def test_release_hosts_selective_non_highest_ordinal_warns_and_still_scales() -> None:
250-
"""When the victims include non-highest ordinals, a WARNING is logged
251-
and the controller still scales down by the victim count (it will
252-
pick the highest-ordinal pods regardless of what the caller asked
253-
for)."""
250+
async def test_release_hosts_selective_non_highest_ordinal_refused() -> None:
251+
"""When the victims are not the top-of-stack ordinals, the handler
252+
raises K8sError and issues no scale patch.
253+
254+
Commit b4a77b1a deliberately changed this behaviour: silently evicting
255+
different pods (the controller always picks the highest-ordinal set)
256+
would cause data loss, so the handler refuses the request and tells
257+
the caller which victims to supply instead.
258+
"""
254259
core_v1 = MagicMock()
255260
apps_v1 = MagicMock()
256261
apps_v1.read_namespaced_stateful_set.return_value = _make_statefulset_status(spec_replicas=5)
@@ -266,35 +271,30 @@ async def test_release_hosts_selective_non_highest_ordinal_warns_and_still_scale
266271
)
267272

268273
# current=5, victims=[ordinal-1, ordinal-2] — non-highest ordinals.
269-
# The controller will evict ordinals 3 and 4 instead.
270-
await handler.release_hosts(["orb-deadbeef-1", "orb-deadbeef-2"], request.provider_data)
274+
# The controller would evict ordinals 3 and 4 instead, so the handler
275+
# refuses rather than silently releasing the wrong pods.
276+
with pytest.raises(K8sError) as exc_info:
277+
await handler.release_hosts(["orb-deadbeef-1", "orb-deadbeef-2"], request.provider_data)
271278

272-
# WARNING logged.
273-
warning_messages = [
274-
call.args[0]
275-
for call in handler._logger.warning.call_args_list # type: ignore[attr-defined]
276-
]
277-
assert any("non-highest-ordinal" in msg for msg in warning_messages), warning_messages
279+
# The error message must mention the correct top-of-stack victims.
280+
assert "orb-deadbeef-3" in str(exc_info.value) or "orb-deadbeef-4" in str(exc_info.value)
278281

279-
# Replicas still patched down by the victim count (5 -> 3) — the
280-
# caller asked to release 2 pods and 2 pods will be released; the
281-
# only thing that changes is *which* ordinals.
282-
apps_v1.patch_namespaced_stateful_set_scale.assert_called_once()
283-
assert (
284-
apps_v1.patch_namespaced_stateful_set_scale.call_args.kwargs["body"]["spec"]["replicas"]
285-
== 3
286-
)
282+
# Refusal happens before any scale patch — the StatefulSet must be
283+
# left untouched.
284+
apps_v1.patch_namespaced_stateful_set_scale.assert_not_called()
287285

288286
# Pods are NEVER deleted directly.
289287
core_v1.delete_namespaced_pod.assert_not_called()
290288
apps_v1.delete_namespaced_stateful_set.assert_not_called()
291289

292290

293291
@pytest.mark.asyncio
294-
async def test_release_hosts_selective_unparseable_victim_names_warns() -> None:
295-
"""Victim names that do not parse as ``<statefulset>-<ordinal>``
296-
should also trigger the WARNING (since they cannot be the top-of-
297-
stack ordinals)."""
292+
async def test_release_hosts_selective_unparseable_victim_names_refused() -> None:
293+
"""Victim names that do not parse as ``<statefulset>-<ordinal>`` are
294+
refused with K8sError — an unparseable name cannot be the top-of-
295+
stack ordinal, so honouring the request would silently evict the
296+
wrong pods.
297+
"""
298298
core_v1 = MagicMock()
299299
apps_v1 = MagicMock()
300300
apps_v1.read_namespaced_stateful_set.return_value = _make_statefulset_status(spec_replicas=3)
@@ -310,19 +310,16 @@ async def test_release_hosts_selective_unparseable_victim_names_warns() -> None:
310310
)
311311

312312
# Victim name does not match the StatefulSet's name prefix — its
313-
# ordinal cannot be parsed.
314-
await handler.release_hosts(["some-other-pod"], request.provider_data)
313+
# ordinal cannot be parsed, so the handler refuses rather than
314+
# silently releasing the wrong pod.
315+
with pytest.raises(K8sError):
316+
await handler.release_hosts(["some-other-pod"], request.provider_data)
315317

316-
warning_messages = [
317-
call.args[0]
318-
for call in handler._logger.warning.call_args_list # type: ignore[attr-defined]
319-
]
320-
assert any("non-highest-ordinal" in msg for msg in warning_messages)
321-
# Still scales down by 1 victim (3 -> 2).
322-
assert (
323-
apps_v1.patch_namespaced_stateful_set_scale.call_args.kwargs["body"]["spec"]["replicas"]
324-
== 2
325-
)
318+
# Refusal happens before any scale patch.
319+
apps_v1.patch_namespaced_stateful_set_scale.assert_not_called()
320+
# Pods are NEVER deleted directly.
321+
core_v1.delete_namespaced_pod.assert_not_called()
322+
apps_v1.delete_namespaced_stateful_set.assert_not_called()
326323

327324

328325
# ---------------------------------------------------------------------------

tests/providers/k8s/unit/test_coverage_gaps.py

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -488,12 +488,16 @@ def test_check_health_returns_unhealthy_when_kubernetes_package_absent(
488488

489489
@pytest.mark.asyncio
490490
async def test_job_release_logs_selective_release_not_supported() -> None:
491-
"""release_hosts with a partial machine_ids list must log that selective release
492-
is not supported.
491+
"""release_hosts with a partial machine_ids list must raise K8sError when
492+
parallelism is known, refusing the subset release.
493493
494-
The Job handler deletes the whole Job regardless of ``machine_ids``.
495-
The operator must be informed via an info-level log message that
496-
includes "selective release not supported".
494+
When ``parallelism`` is NOT recorded in ``provider_data`` (legacy or
495+
pre-acquire state), the handler falls back to deleting the whole Job
496+
and emits an info-level log recording the requested machine_ids.
497+
498+
This test exercises the fallback path (no ``parallelism`` in
499+
``provider_data``) and confirms the Job is still deleted and an
500+
info-level log is emitted that identifies the release operation.
497501
"""
498502
from orb.providers.k8s.infrastructure.handlers.job_handler import K8sJobHandler
499503

@@ -513,25 +517,28 @@ async def test_job_release_logs_selective_release_not_supported() -> None:
513517
requested_count=3,
514518
provider_api="Job",
515519
)
516-
# Override provider_data with a stable job_name so we don't need to
517-
# call acquire first.
520+
# Override provider_data with a stable job_name but NO parallelism key
521+
# so we don't need to call acquire first and the K8sError refuse path
522+
# is not triggered (parallelism defaults to 0).
518523
object.__setattr__(
519524
request,
520525
"provider_data",
521526
{"namespace": "orb-test", "job_name": "orb-testreq1"},
522527
)
523528

524529
# Selective release: caller only passes one of the three pods.
530+
# With no parallelism recorded, the handler deletes the whole Job.
525531
await handler.release_hosts(["orb-testreq1-pod0"], request.provider_data)
526532

527533
# The whole Job must have been deleted.
528534
batch_v1.delete_namespaced_job.assert_called_once()
529535

530-
# An info-level message must mention selective release.
536+
# An info-level message must record the job release operation with the
537+
# requested machine_ids (the log reads "deleting whole Job").
531538
info_calls = logger.info.call_args_list
532-
selective_log = any("selective release" in str(call).lower() for call in info_calls)
533-
assert selective_log, (
534-
"Expected an info-level log message about selective release not being supported; "
539+
release_log = any("job release" in str(call).lower() for call in info_calls)
540+
assert release_log, (
541+
"Expected an info-level log message about the job release operation; "
535542
f"info calls: {info_calls}"
536543
)
537544

0 commit comments

Comments
 (0)