Repository navigation
feat: add single Codeflare entrypoint with handler pattern and remove legacy auth - #1091
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1091 +/- ##
==========================================
- Coverage 96.73% 96.50% -0.24%
==========================================
Files 23 24 +1
Lines 2391 2319 -72
==========================================
- Hits 2313 2238 -75
- Misses 78 81 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/hold |
Introduces the design for a unified SDK entrypoint with SDKConfig, ClusterHandler, and JobHandler using kube-authkit for authentication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ean set_api_client, update E2E/notebooks - Remove print and verification call from set_api_client (now internal-only) - Make namespace fallback consistent: all handler methods fall back to "default" - Fix E2E tests: replace TokenAuthentication with Codeflare/SDKConfig - Fix demo notebooks: use Codeflare(config=SDKConfig(auth=...)) instead of set_api_client - Fix unused import (patch) and non-deterministic set ordering in error msg - Add tests for namespace fallback in create() and submit() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
7fc63d9 to
1aa4a17
Compare
The `list` method on ClusterHandler shadows the built-in `list` type, causing mypy to interpret `-> list` on `list_queued` as a reference to the method rather than the type. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
Signed-off-by: Saad Zaher <szaher@redhat.com>
|
/unhold |
pawelpaszki
left a comment
There was a problem hiding this comment.
OpenShift / E2E status
E2E against OpenShift is passing on this branch: the kubernetes and kubernetes-rayjob workflows completed successfully, and the updated E2E paths (tests/e2e_v2/..., tests/e2e/support.py) use Codeflare + kube_authkit.AuthConfig instead of legacy TokenAuthentication. That gives good confidence for RHOAI/OpenShift token flows.
Unit tests, mypy, ruff, pre-commit, and codecov project checks are also green.
Summary
This PR introduces a solid Codeflare + SDKConfig entrypoint with cf.clusters / cf.jobs handlers, removes legacy auth in favor of kube-authkit, and consolidates RayJob embedded-cluster config onto ClusterConfiguration (RHAIENG-2063). The direction is right and the new test_codeflare.py coverage is strong.
Because this bundles multiple breaking API changes with subtle RayJob CR spec differences, I am requesting a few changes before merge (below). Non-blocking nits are called out separately.
What works well
- Thin handlers delegating to existing
Cluster/RayJob/ list helpers — good separation without reimplementing lifecycle logic. SDKConfigvalidation and handler tests are clear;TestLegacyAuthRemoveddocuments the new public contract.- Demo notebooks and E2E largely migrated to
Codeflare(config=SDKConfig(auth=...)). builtins.listreturn annotations onClusterHandler.list/list_queuedcorrectly avoid mypy shadowing issues.
Must change (blocking)
1. Flagship demo notebook imports removed ManagedClusterConfig
6_single_entrypoint.ipynb Step 4 still uses ManagedClusterConfig, which this PR removes. Users following the primary onboarding notebook will hit ImportError. Use ClusterConfiguration (and updated field names, e.g. head_extended_resource_requests instead of head_accelerators).
See inline comment on the notebook.
2. SDKConfig.retries / SDKConfig.timeout are documented but not applied
Both fields are validated and tested on the dataclass, but Codeflare.__init__ never consumes them. Either wire them through (K8s client / wait helpers) or remove from the public API in this PR to avoid misleading configuration.
See inline comment on codeflare.py.
3. Migration / upgrade documentation for breaking changes
This PR removes in one step:
| Removed | Replacement |
|---|---|
TokenAuthentication, KubeConfigFileAuthentication, … |
kube_authkit.AuthConfig + Codeflare(SDKConfig(auth=...)) |
Top-level set_api_client, AuthConfig, get_k8s_client |
Codeflare init or codeflare_sdk.common.kubernetes_cluster.auth |
ManagedClusterConfig |
ClusterConfiguration (+ field renames) |
Please add a CHANGELOG entry and/or short migration section (before/after for token + kubeconfig, RayJob cluster_config, and export changes). The PR template verification / upgrade sections are still largely empty.
4. Document RayJob embedded-cluster spec deltas (RHAIENG-2063)
Consolidating on ClusterConfiguration + build_ray_cluster_spec() changes runtime specs vs old ManagedClusterConfig, including:
- Different resource defaults (e.g. head CPU/memory).
enableInTreeAutoscalingnow followsenable_autoscaling(defaultFalse— OK for Kueue today, but behavior changes if autoscaling is enabled).autoscalerOptionsalways present in the built spec.- ODH CA volumes always merged for job-managed clusters.
- Image pull policy
IfNotPresent→Alwaysfor head/worker. - Worker container / group naming aligned with RayCluster builder (
machine-learning,small-group-{name}).
Please call these out in release notes (and reference RHAIENG-2063). A golden-spec regression test would help if you have a prior baseline.
See inline comment on ray/rayjobs/config.py.
Non-blocking nits
SDKConfig.namespacedocstring says "auto-detected if not set", but handlers fall back to the literal"default". Align docs or implement detection (e.g. from kube context). Inline oncodeflare.py.ClusterHandler.listshadows the builtinlist; considerlist_clusters()/list_all()in a follow-up for readability.- Global
api_client: eachCodeflare()callsset_api_client()— last instance wins in-process. Worth a one-line doc warning. Inline onCodeflare.__init__. set_api_clientno longer probes the API viaget_api_group()— failures may surface later; acceptable if intentional. Inline onauth.py.- PR hygiene: contributor checkboxes still unchecked; codecov patch reports ~9 uncovered lines — quick pass welcome.
- Scope: entrypoint + auth removal + config consolidation in one PR increases review/rollback cost; fine if intentional for one release, but migration docs become more important.
Verdict
Request changes — fix the demo notebook, resolve retries/timeout API surface, and add migration + RayJob spec notes. Happy to re-review once those are addressed.
Signed-off-by: Saad Zaher <szaher@redhat.com>
Follow-up: RayJob config migration docThanks for the updates in One small docs item remains: This can land post-merge as a follow-up PR (or a quick docs-only commit right after merge) — I would not block merge on it, but it would be good to track so release notes and the auth guide stay accurate. |
Signed-off-by: Saad Zaher <szaher@redhat.com>
|
/hold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pawelpaszki The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…nt (#1176) * RHOAIENG-98754: bind Codeflare instances to their own K8s client Post-merge hardening of the Codeflare single entrypoint added in #1091. This is PR 1 of a planned series; it covers client isolation and the facade's job-submission contract. P1 client isolation ------------------- Codeflare.__init__ stored its client in module-level global state via set_api_client(), while Cluster, RayJob and the list/get helpers all read the global get_api_client(). Constructing a second Codeflare instance therefore silently repointed objects created from the first one, so operations could run against the wrong Kubernetes cluster. Objects now carry their own client. auth.py gains a ContextVar plus a _use_api_client() context manager and a _bound_to_api_client decorator; get_api_client() prefers the context-scoped client and falls back to the module global. Cluster, RayJob, get_cluster, list_all_clusters and list_all_queued take an additional api_client argument, and the facade handlers pass the instance's own client explicitly. A ContextVar is used rather than threading a client parameter through every call site because the client is resolved in ~32 places across build_ray_cluster, the Kueue helpers and cert generation. This keeps the change contained and leaves those helpers untouched. P1 cf.jobs.submit() contract ---------------------------- The advertised cf.jobs.submit(name=..., entrypoint=...) could never work: RayJob requires exactly one of cluster_name/cluster_config. Existing tests missed this because they mock the RayJob class wholesale. cluster_name and cluster_config are now explicit keyword-only parameters on both submit() and create(), so the requirement is visible to type checkers and IDEs, and the error raised when neither is supplied no longer says "but not both". P2 namespace resolution ----------------------- The facade resolved namespaces with `or "default"`, silently targeting the default namespace and defeating RayJob's own context detection. A single _resolve_namespace() helper now applies: explicit argument, then SDKConfig.namespace, then the detected current namespace, then a clear error. Backward compatibility ---------------------- api_client is additive and defaults to None, which preserves the existing global-client behaviour for direct callers such as Cluster(config) and RayJob(...). Codeflare still calls set_api_client() so those callers are unaffected. No public argument became mandatory and no field was renamed. Behaviour changes, both facade-only: - the facade no longer falls back to the "default" namespace - the neither-target error message changed Testing ------- New src/codeflare_sdk/test_client_isolation.py covers client isolation (including that a second instance cannot steal the first's client and that the scoped client does not leak past an operation), legacy global fallback, the submit contract, and namespace precedence. Pre-existing failures unrelated to this change, also failing on main: test_kueue.py::test_cluster_creation_local_queue and test_config.py::test_default_cluster_creation, both caused by Python 3.13 having no default Ray image in this environment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98754: apply ruff-format from pre-commit Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98754: address PR #1176 review Keep namespace positional on the job handlers --------------------------------------------- Moving namespace behind * made positional submit(name, entrypoint, ns) raise TypeError, which is a facade API break that was not classified. namespace is back in its original third slot; only the newly added cluster_name/cluster_config are keyword-only, so no previously valid call changes meaning. The handlers still accept every RayJob parameter, so nothing was lost when **kwargs was replaced. Make config_check() honour the scoped client -------------------------------------------- config_check() keyed off the module global only. With a scoped client active but no global set, it fell through to kube-authkit auto-detection and assigned the module-level api_client as a side effect of a scoped call. It now returns early when a scoped client is active. Scope the remaining Cluster entry points ---------------------------------------- Cluster.config_check() and the Cluster._client_headers property resolve a client but were not decorated. Both now carry @_bound_to_api_client. An audit over both classes confirms no undecorated client resolver remains: 29 Cluster methods and 13 RayJob methods, zero undecorated. The module level functions in cluster.py that resolve a client are either scoped (get_cluster) or internal helpers only reachable from an already scoped caller. Assert RayJob isolation at the API-call level --------------------------------------------- job._api_client is client_a only proves the attribute was stored. Added the same post-cf_b API-call assertion the Cluster path already had, via _build_rayjob_cr() which resolves the client through the Kueue helpers. This behaviour was already correct; the test is a regression guard. One existing test in this file passed for the wrong reason: a stale module-level config_path leaked from an earlier test and short-circuited config_check(). The autouse fixture now restores config_path as well as api_client. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98754: review nits from b6b184b Scope two more Cluster members ------------------------------ _client_verify_tls reaches a client transitively via _is_openshift_cluster(), which the previous audit missed because it only looked for a literal get_api_client() in the method body. Reached directly rather than through a decorated caller, it could probe the wrong cluster after a second Codeflare exists. Now decorated, with a test that asserts the ApisApi call uses the owning client. cluster_uri() resolves no client itself — it calls the already decorated _check_tls_certs_exist() — but it is a public entry point, so decorating it keeps the rule uniform at no behavioural cost. Type runtime_env properly ------------------------- Optional[Union[Dict[str, Any], Any]] collapses to Any and documents nothing. Now Optional[Union[RuntimeEnv, Dict[str, Any]]], matching the RayJob constructor, with a test asserting the two stay equal. Docstring corrections --------------------- SDKConfig.namespace still promised a 'default' fallback the facade no longer has. config_check()'s priority list started at the module global and omitted the scoped client the code now checks first. Coverage for the module-helper path ----------------------------------- clusters.list() and clusters.list_queued() route through the module helpers and the ContextVar rather than Cluster._api_client, and had no two-client test. Added. Also added one asserting the CustomObjectsApi RayJob captures at init is built on its own client. Both passed on first run: these were coverage gaps, not bugs. The autouse fixture now asserts the ContextVar is clear after each test. _use_api_client's finally already guarantees this; the assertion stops a leaked scope from silently greening a later test, which is the failure mode config_path actually hit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
) * RHOAIENG-98754: fix stale public-API references in demos and docs Part of P1-6. PR #1091 removed `ManagedClusterConfig` and the top-level `set_api_client` export, but the guided demos and user docs still import them. Every affected example fails at the import cell, so the two guided notebook CI jobs (`verify-5_submit_rayjob_cr`, `verify-6_autoscaling`) are red on `main` and the README quickstart does not run. Notebooks: - 5_submit_rayjob_cr: `ManagedClusterConfig` -> `ClusterConfiguration`, `head_accelerators`/`worker_accelerators` -> `head_extended_resource_requests`/`worker_extended_resource_requests`. - 6_autoscaling: drop `set_api_client`/`get_k8s_client`, authenticate via `Codeflare(SDKConfig(auth=...))`. - 7_rayjob_checkpointing_example: same two fixes. Not covered by CI, but broken in the same way. `verify_ssl` is now set on `cf.client`. Docs: - README quickstart rewritten onto `Codeflare`/`SDKConfig`. - authentication.rst: all five auth methods rewritten onto the entrypoint. The "Deprecated Authentication Methods" section claimed `TokenAuthentication`/`KubeConfigFileAuthentication` "remain functional"; they raise `ImportError`. Corrected to "Removed". - rayjob.rst: `ManagedClusterConfig` -> `ClusterConfiguration`. - AGENTS.md and the rayjobs rule files pointed at a `ManagedClusterConfig` dataclass that no longer exists; `rayjobs/config.py` is now the `build_ray_cluster_spec` builder. Verified by applying the workflow's jq/sed transformations to copies of the notebooks and checking every `codeflare_sdk` import resolves. The jq cell-deletion step also requires `source` to stay a list of lines, so the notebooks keep their original JSON formatting. Not included, deliberately: - `docs/api/public-surface.json` still lists `Authentication`, `KubeConfiguration`, `TokenAuthentication` and `KubeConfigFileAuthentication`, which no longer exist. Removing them pre-empts the pending maintainer decision on whether to restore the legacy auth shims, so they are left for that discussion. - CI drift enforcement for the public API surface. - `docs/rayjob_config_migration_guide.md` names `ManagedClusterConfig` by design, as the "before" side of the migration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98754: bring AGENTS.md up to date with the current SDK AGENTS.md (symlinked as CLAUDE.md) had drifted from the code after the single-entrypoint and client-isolation work. Corrected: - `RayJob` cited at line 58; it is at 64. - "8 notebooks"; there are 9 — `6_autoscaling` and `6_single_entrypoint` share the `6_` prefix. - `.cursor/rules/` listed 6 of 10 files, and claimed `.claude/rules/` mirrors all of them. It mirrors only the three path-scoped rules; the numbered 01-07 rules exist only under `.cursor/rules/`. Added, because their absence leads to wrong code rather than just missing context: - The `Codeflare` entrypoint, with a worked example and a pointer to `src/codeflare_sdk/codeflare.py`. It was not mentioned at all. - Client isolation: `@_bound_to_api_client` on `Cluster`/`RayJob` methods, the `api_client=None` parameter on module-level helpers, and the ContextVar resolution order. Omitting the decorator is silent — the method works until a second `Codeflare` exists, then talks to the wrong cluster — so "Kubernetes API Patterns" as written was a trap. - `WorkerGroup`, and the warning that `build_ray_cluster.py` and `rayjobs/config.py` are two builders over one dataclass: a field added to only one is silently dropped by the other path. - Guided notebook jobs are gated on the `test-guided-notebooks` label and otherwise report skipped, so a broken notebook looks green. Also the `jq` cell-deletion step requires list-of-lines `source`. - `mypy` and `lint-imports` commands, and the ruff version mismatch that makes the CI `precommit` job fail with "files were modified by this hook". - `public-surface.json` is not CI-checked and currently lists four removed auth symbols. Every path and cited line number in the file was verified against the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98754: make `cf` actually own the client in notebooks 6 and 7 Review follow-up. Both notebooks constructed a `Codeflare` and then ignored it: `Cluster(...)`, `RayJob(...)` and `get_cluster(...)` resolved their client from the module-level global instead. That works only because `Codeflare.__init__` still calls `set_api_client()` — the last-writer-wins global #1176 exists to move off. Notebook 7's markdown claimed the isolation the code did not do. Notebook 7 (not covered by any CI workflow, so `cf` always exists): - `RayJob(..., api_client=cf.client)` - `get_cluster(..., api_client=cf.client)` at all three call sites Notebook 6 needs one extra step: the guided-notebook workflow deletes its auth cell (it matches the "Authenticate to your Kubernetes/OpenShift cluster using kube-authkit" marker), so `cf` does not exist under CI and `api_client=cf.client` would be a NameError. Instead `CF_CLIENT` is seeded to None in the imports cell and set from `cf.client` in the auth cell: - `Cluster(ClusterConfiguration(...), api_client=CF_CLIENT)` - `CoreV1Api(CF_CLIENT or get_api_client())` in both pod-counting helpers Users get an explicitly bound client; CI keeps today's behaviour, since `api_client=None` is the legacy fallback. The shared constants moved to the imports cell so `NAMESPACE` is available to `SDKConfig`. A full rewrite onto `cf.clusters.create(...)` / `cf.jobs.submit(...)` is deliberately left out to avoid churning guided-notebook CI in this PR. Verified by replaying the workflow's jq/sed steps: after the auth cell is deleted, `cf` appears in no executable statement (AST-checked, not grep — it survives in a comment), `CF_CLIENT` is None, and every sed still matches. Separately, binding was checked functionally: a cluster built with `api_client=cf.client` still issues its API calls on that client after a second `Codeflare` takes over the global. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The facade hid its arguments behind **kwargs, so a misspelled cluster field or a job with no execution target was only ever a runtime failure. cf.jobs.submit(name=..., entrypoint=...) shipping broken in #1091 is the case in point. - cf.clusters.create() takes a ClusterConfiguration, the same object cf.jobs.create(cluster_config=...) and Cluster() already take. One way to describe a cluster across the SDK, and the dataclass does the type checking without the facade mirroring 34 fields. - cf.clusters.get() drops **kwargs for explicit verify_tls / write_to_file, the only arguments get_cluster accepts. - cf.jobs.create() and submit() are overloaded on the execution target, making "exactly one of cluster_name / cluster_config" a type error rather than only a ValueError. Both delegate to JobHandler._build(), since an overloaded method cannot satisfy its own overloads without re-narrowing the target. - JobOptions is exported, and public-surface.json gains the codeflare group it was missing for Codeflare and SDKConfig. The handler resolves the namespace and injects it with copy.copy rather than dataclasses.replace: ClusterConfiguration.__post_init__ is not idempotent — it merges the default accelerator mapping into extended_resource_mapping and then rejects the merged result — so reconstructing a configured instance raises. Covered by test_create_accepts_a_fully_configured_config. The facade is unreleased (#1091 is in no tag), so this changes no published API. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* RHOAIENG-98954: type the Codeflare facade's keyword arguments The facade's primary entrypoints hid their arguments behind **kwargs, so a misspelled cluster field or a job with no execution target was only ever a runtime failure. cf.jobs.submit(name=..., entrypoint=...) shipping broken in - cf.clusters.create() takes **kwargs: Unpack[ClusterConfigKwargs], a TypedDict mirroring every ClusterConfiguration field except name and namespace, which the handler owns. - cf.clusters.get() drops **kwargs for explicit verify_tls / write_to_file, the only arguments get_cluster accepts. - cf.jobs.create() and submit() are overloaded on the execution target, making "exactly one of cluster_name / cluster_config" a type error rather than only a ValueError. Both now delegate to JobHandler._build(), since an overloaded method cannot satisfy its own overloads without re-narrowing the target. - ClusterConfigKwargs and JobOptions are exported, and public-surface.json gains the codeflare group it was missing for Codeflare and SDKConfig. Runtime behaviour is unchanged: the overloads are type-only and the job handler signatures are identical to before. The TypedDicts restate names that live elsewhere, so TestFacadeTypeSignatures guards them — a new ClusterConfiguration field, a changed type, a new job keyword or a dropped overload each fail a test rather than silently going unreachable through the facade. Signed-off-by: Saad Zaher <szaher@redhat.com> * RHOAIENG-98954: type the Codeflare facade's arguments The facade hid its arguments behind **kwargs, so a misspelled cluster field or a job with no execution target was only ever a runtime failure. cf.jobs.submit(name=..., entrypoint=...) shipping broken in #1091 is the case in point. - cf.clusters.create() takes a ClusterConfiguration, the same object cf.jobs.create(cluster_config=...) and Cluster() already take. One way to describe a cluster across the SDK, and the dataclass does the type checking without the facade mirroring 34 fields. - cf.clusters.get() drops **kwargs for explicit verify_tls / write_to_file, the only arguments get_cluster accepts. - cf.jobs.create() and submit() are overloaded on the execution target, making "exactly one of cluster_name / cluster_config" a type error rather than only a ValueError. Both delegate to JobHandler._build(), since an overloaded method cannot satisfy its own overloads without re-narrowing the target. - JobOptions is exported, and public-surface.json gains the codeflare group it was missing for Codeflare and SDKConfig. The handler resolves the namespace and injects it with copy.copy rather than dataclasses.replace: ClusterConfiguration.__post_init__ is not idempotent — it merges the default accelerator mapping into extended_resource_mapping and then rejects the merged result — so reconstructing a configured instance raises. Covered by test_create_accepts_a_fully_configured_config. The facade is unreleased (#1091 is in no tag), so this changes no published API. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Signed-off-by: Saad Zaher <szaher@redhat.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…rule docs/raycluster_spec_parity.md is the audit the ticket asks for: all 36 ClusterConfiguration fields classified SUPPORTED (29), CONTEXT_ONLY (3), NOT_APPLICABLE (3) or CONFLICTED (1), each with a reason. The page is not the enforcement — ray/test_builder_parity.py is — but it explains the decisions the test encodes, including why raycluster_spec.py has to stay free of Kubernetes API calls. The rayjobs rule files claimed "Must NOT import from ray.cluster or ray.client", which stopped being true at #1091: rayjobs/config.py:46 and rayjob.py:36 both import ClusterConfiguration from ray.cluster.config, and .importlinter has no contract for it, so CI never caught the contradiction. Both files now describe the real boundary — cluster.config and cluster.raycluster_spec are allowed, everything else in ray.cluster is not, and it is prose-only — and point at the shared module as the place to add a field. AGENTS.md's two-builder hazard note said a field added to one builder is dropped by the other, with no mention of where to put one instead. It now names raycluster_spec.py, the parity test and the audit page, and carries the no-I/O constraint. Every repo path and symbol cited across the four files was checked to resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rule docs/raycluster_spec_parity.md is the audit the ticket asks for: all 36 ClusterConfiguration fields classified SUPPORTED (29), CONTEXT_ONLY (3), NOT_APPLICABLE (3) or CONFLICTED (1), each with a reason. The page is not the enforcement — ray/test_builder_parity.py is — but it explains the decisions the test encodes, including why raycluster_spec.py has to stay free of Kubernetes API calls. The rayjobs rule files claimed "Must NOT import from ray.cluster or ray.client", which stopped being true at #1091: rayjobs/config.py:46 and rayjob.py:36 both import ClusterConfiguration from ray.cluster.config, and .importlinter has no contract for it, so CI never caught the contradiction. Both files now describe the real boundary — cluster.config and cluster.raycluster_spec are allowed, everything else in ray.cluster is not, and it is prose-only — and point at the shared module as the place to add a field. AGENTS.md's two-builder hazard note said a field added to one builder is dropped by the other, with no mention of where to put one instead. It now names raycluster_spec.py, the parity test and the audit page, and carries the no-I/O constraint. Every repo path and symbol cited across the four files was checked to resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch now changes no cluster or job behaviour. It shares duplicated spec-building code and records the differences; the two behavioural fixes have moved out. GCS fault tolerance is no longer emitted on the RayJob path. Investigating the release risk turned up 52a351a, "RHOAIENG-30720: Remove GCS FT for Lifecycled RayClusters", which stripped those four fields from the lifecycled path in August 2025 because the feature did not work — head pod restarts lost state — and whose ticket says the RayJob implementation was out of scope for the fix. So the absence was a decision, not drift, and emitting them here would re-enable something nobody has validated on that path. The real bug #1091 introduced is narrower: making ClusterConfiguration shared means the fields are now accepted and validated on the RayJob path and then ignored. Rejecting them or emitting them is RHOAIENG-98943's call; the asymmetry is pinned by test_gcs_fault_tolerance_is_standalone_only so neither side moves by accident. config.labels on the head and default worker pods moves to #1184, two lines that can ship without this refactor. labels is reclassified PARTIAL rather than a gap. A presence check passes on both paths because the RayJob builder merges config.labels into additional worker group pods; what differs is the default head and worker pods. The earlier xfail would have XPASSed once the sentinel config gained a real WorkerGroup, which is how this surfaced. docs/raycluster_spec_parity.md gains a PENDING DECISION section and the counts still total 36: SUPPORTED 24, PARTIAL 1, PENDING DECISION 4, CONTEXT_ONLY 3, NOT_APPLICABLE 3, CONFLICTED 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rule docs/raycluster_spec_parity.md is the audit the ticket asks for: all 36 ClusterConfiguration fields classified SUPPORTED (29), CONTEXT_ONLY (3), NOT_APPLICABLE (3) or CONFLICTED (1), each with a reason. The page is not the enforcement — ray/test_builder_parity.py is — but it explains the decisions the test encodes, including why raycluster_spec.py has to stay free of Kubernetes API calls. The rayjobs rule files claimed "Must NOT import from ray.cluster or ray.client", which stopped being true at #1091: rayjobs/config.py:46 and rayjob.py:36 both import ClusterConfiguration from ray.cluster.config, and .importlinter has no contract for it, so CI never caught the contradiction. Both files now describe the real boundary — cluster.config and cluster.raycluster_spec are allowed, everything else in ray.cluster is not, and it is prose-only — and point at the shared module as the place to add a field. AGENTS.md's two-builder hazard note said a field added to one builder is dropped by the other, with no mention of where to put one instead. It now names raycluster_spec.py, the parity test and the audit page, and carries the no-I/O constraint. Every repo path and symbol cited across the four files was checked to resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch now changes no cluster or job behaviour. It shares duplicated spec-building code and records the differences; the two behavioural fixes have moved out. GCS fault tolerance is no longer emitted on the RayJob path. Investigating the release risk turned up 52a351a, "RHOAIENG-30720: Remove GCS FT for Lifecycled RayClusters", which stripped those four fields from the lifecycled path in August 2025 because the feature did not work — head pod restarts lost state — and whose ticket says the RayJob implementation was out of scope for the fix. So the absence was a decision, not drift, and emitting them here would re-enable something nobody has validated on that path. The real bug #1091 introduced is narrower: making ClusterConfiguration shared means the fields are now accepted and validated on the RayJob path and then ignored. Rejecting them or emitting them is RHOAIENG-98943's call; the asymmetry is pinned by test_gcs_fault_tolerance_is_standalone_only so neither side moves by accident. config.labels on the head and default worker pods moves to #1184, two lines that can ship without this refactor. labels is reclassified PARTIAL rather than a gap. A presence check passes on both paths because the RayJob builder merges config.labels into additional worker group pods; what differs is the default head and worker pods. The earlier xfail would have XPASSed once the sentinel config gained a real WorkerGroup, which is how this surfaced. docs/raycluster_spec_parity.md gains a PENDING DECISION section and the counts still total 36: SUPPORTED 24, PARTIAL 1, PENDING DECISION 4, CONTEXT_ONLY 3, NOT_APPLICABLE 3, CONFLICTED 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ge) (#1182) * RHOAIENG-98942: add a builder parity test before unifying the two paths ClusterConfiguration feeds two independent spec builders — the standalone RayCluster in ray/cluster/build_ray_cluster.py and the embedded rayClusterSpec in ray/rayjobs/config.py. A field wired into one and forgotten in the other raises nothing; the cluster just comes up without it. Unifying them is a large refactor, so the parity check lands first and the refactor is held to it. The test sets every ClusterConfiguration field to a recognisable sentinel, renders both specs and asserts each sentinel reaches both. Anything that legitimately does not reach a builder is listed with a reason: - CONTEXT_ONLY (3): overwrite_default_resource_mapping and enable_usage_stats are consumed by __post_init__ (the latter flows on via envs); verify_tls governs dashboard calls, not the CR. - NOT_APPLICABLE (3): name, namespace and write_to_file are meaningless inside a RayJob, which derives '<job_name>-cluster' and embeds a spec with no metadata block. - CONFLICTED (1): local_queue is set on both sides with undefined precedence (RHOAIENG-98949). test_every_field_is_classified fails on a new field that is neither given a sentinel nor classified, so "no field may be silently ignored" is enforced rather than asserted. Four fields the RayJob path drops today are xfail(strict=True) rather than exempt, since they are bugs and the unification has to remove them: - redis_address, redis_password_secret, external_storage_namespace — GCS fault tolerance is entirely absent from the RayJob builder (RHOAIENG-98943). - labels — reaches additional worker groups only; the head and default worker pod templates at rayjobs/config.py:146,166 pass annotations and no labels. Not previously tracked by any ticket. Also recorded in the test: build_ray_cluster() is not pure. It reaches the Kubernetes API three times while rendering — local_queue_exists(), get_default_local_queue() and validate_autoscaling_with_kueue() — which the RayJob builder never does. The canonical builder therefore has to split a pure renderer from the resolve-cluster-facts step, or the RayJob path inherits network calls it does not make today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98942: extract the shared RayCluster spec pieces The standalone and RayJob builders each carried their own copy of the same primitives, which is how they drifted in the first place. Both now delegate to ray/cluster/raycluster_spec.py. Proven identical to both originals before being removed: the ODH trusted-CA volumes and mounts (byte-for-byte duplicates), cpu_limit_to_num_cpus (a literal copy), the storage merge, resource requirements, GPU counting, extended-resource mapping, and the head and worker containers — the last two compared by repr against both implementations. The module is deliberately side-effect-free. build_ray_cluster() reaches the Kubernetes API three times while rendering — local_queue_exists(), get_default_local_queue() and validate_autoscaling_with_kueue() — and the RayJob builder makes no calls at all, so anything needing the cluster stays in the caller. Lifting that I/O out is the next step. Two pieces are added here for the fixes that follow rather than used yet: gcs_fault_tolerance_options() and worker_replica_counts(), both unit tested. build_ray_cluster.py keeps its existing function names as one-line delegations, so no call site moved and the diff stays reviewable. Two spec builders totalling 1,186 lines are now 838 plus a 279-line shared module. Test patch targets for update_image moved with it, except in test_build_spec_additional_group_inherits_image: additional worker groups resolve their own image in rayjobs/config.py and still patch there. Not yet fixed, still xfail: the five fields the RayJob path drops. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98942: stop the RayJob path dropping GCS FT and labels The five fields the parity test had marked xfail now reach both builders, so KNOWN_GAPS is empty and the xfails are gone. GCS fault tolerance (RHOAIENG-98943) was the dangerous one. enable_gcs_ft, redis_address, redis_password_secret and external_storage_namespace were accepted and validated by ClusterConfiguration, then ignored entirely by the RayJob builder — the four names did not appear anywhere in rayjobs/config.py. A job configured for fault tolerance came up with no Redis to recover from, reported nothing, and only failed when the head node died. build_ray_cluster_spec() now emits gcsFaultToleranceOptions from the shared helper, the same one the standalone path uses. labels reached additional worker groups only. _build_additional_worker_group_spec merged config.labels into its pod template while the head and default worker templates passed annotations and nothing else, so ClusterConfiguration(labels=...) landed on extra worker groups and nowhere else. Both templates now pass them. The asymmetry that remains is deliberate: the standalone path also puts labels on the RayCluster metadata, which the embedded spec has no room for. Pod templates are where they can land here, and where this path already put them for additional worker groups. Regression tests cover the RayJob path directly rather than relying on the parity markers, including that metadata stays None when no labels are set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98942: record the field audit and correct the stale rayjobs rule docs/raycluster_spec_parity.md is the audit the ticket asks for: all 36 ClusterConfiguration fields classified SUPPORTED (29), CONTEXT_ONLY (3), NOT_APPLICABLE (3) or CONFLICTED (1), each with a reason. The page is not the enforcement — ray/test_builder_parity.py is — but it explains the decisions the test encodes, including why raycluster_spec.py has to stay free of Kubernetes API calls. The rayjobs rule files claimed "Must NOT import from ray.cluster or ray.client", which stopped being true at #1091: rayjobs/config.py:46 and rayjob.py:36 both import ClusterConfiguration from ray.cluster.config, and .importlinter has no contract for it, so CI never caught the contradiction. Both files now describe the real boundary — cluster.config and cluster.raycluster_spec are allowed, everything else in ray.cluster is not, and it is prose-only — and point at the shared module as the place to add a field. AGENTS.md's two-builder hazard note said a field added to one builder is dropped by the other, with no mention of where to put one instead. It now names raycluster_spec.py, the parity test and the audit page, and carries the no-I/O constraint. Every repo path and symbol cited across the four files was checked to resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98942: address review — use the shared helpers on both paths Review from @pawelpaszki on #1182. All points verified against the code before changing anything; every one of them held. Blocking: - The standalone builder still inlined the GCS fault tolerance block instead of calling the helper extracted for it, which is exactly the duplication 98942/98943 were meant to remove. It now calls gcs_fault_tolerance_options(). The uncovered line Codecov flagged in build_ray_cluster.py was indeed that leftover ValueError. - worker_replica_counts() was dead — only its own tests called it. Both builders now use it, replacing two copies of the replicas / minReplicas / maxReplicas branch. - format_resources_param() was used on the RayJob side only; the standalone path inlined json.dumps. It now calls the shared one. Non-blocking: - The parity test claimed more than it checked. Fields whose MARKERS entry was None were filtered out of the parametrized test entirely, so num_workers, min_workers, max_workers, enable_autoscaling, enable_gcs_ft and additional_worker_groups were never parity-checked, and the additional_worker_groups sentinel was an empty list. Those fields are now listed in STRUCTURAL and asserted against the sanitised spec dicts rather than by substring on repr(), and the sentinel is a real WorkerGroup. Mutation-checked: dropping GCS FT or additional worker groups from the RayJob path fails the suite. - Bottom-of-file imports moved to the top; there was no cycle, so the noqa: E402 pair was covering nothing. - Deleted the ODH volumes section heading left stranded above build_ray_cluster_spec. - docs: labels moved out of SUPPORTED into a PARTIAL section. Both paths carry them, but only the RayJob path puts them on the default head and worker pods, so a NetworkPolicy selector still behaves differently. Fixing that changes standalone output, so it belongs with the follow-up. - docs: added an explicit list of what is shared versus what each builder still assembles independently. The remaining blocking point — that the tree is still duplicated and 98942 should not close as done — is answered in the PR description rather than here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98942: use format_resources_param for standalone extra groups Review nit from @pawelpaszki: _build_worker_group_spec still inlined json.dumps for its rayStartParams resources while every other call site used the shared helper. The remaining json.dumps in this module is the YAML file writer, which is unrelated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98942: split out the behaviour changes, keep consolidation only This branch now changes no cluster or job behaviour. It shares duplicated spec-building code and records the differences; the two behavioural fixes have moved out. GCS fault tolerance is no longer emitted on the RayJob path. Investigating the release risk turned up 52a351a, "RHOAIENG-30720: Remove GCS FT for Lifecycled RayClusters", which stripped those four fields from the lifecycled path in August 2025 because the feature did not work — head pod restarts lost state — and whose ticket says the RayJob implementation was out of scope for the fix. So the absence was a decision, not drift, and emitting them here would re-enable something nobody has validated on that path. The real bug #1091 introduced is narrower: making ClusterConfiguration shared means the fields are now accepted and validated on the RayJob path and then ignored. Rejecting them or emitting them is RHOAIENG-98943's call; the asymmetry is pinned by test_gcs_fault_tolerance_is_standalone_only so neither side moves by accident. config.labels on the head and default worker pods moves to #1184, two lines that can ship without this refactor. labels is reclassified PARTIAL rather than a gap. A presence check passes on both paths because the RayJob builder merges config.labels into additional worker group pods; what differs is the default head and worker pods. The earlier xfail would have XPASSed once the sentinel config gained a real WorkerGroup, which is how this surfaced. docs/raycluster_spec_parity.md gains a PENDING DECISION section and the counts still total 36: SUPPORTED 24, PARTIAL 1, PENDING DECISION 4, CONTEXT_ONLY 3, NOT_APPLICABLE 3, CONFLICTED 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98942: give every WorkerGroup field a parity sentinel Review from @chipspeak: the WorkerGroup sentinel set only group_name and replicas, so envs, labels, tolerations, image, the GPU settings and extended resources could all drift between the two builders while this suite stayed green. Correct — additional_worker_groups is assembled by a second pair of duplicated functions, _build_worker_group_spec and _build_additional_worker_group_spec, which can diverge independently of the main builders this file was written to guard. All 15 fields now carry sentinels, and the four inherit-or-override rules get named tests on both paths, because a presence check cannot express them: image and tolerations are overrides that fall back to the cluster value, while envs and labels are merges where the group wins a key collision. The inherit case is asserted separately from the override case, so dropping either direction fails. test_every_worker_group_field_is_classified mirrors the existing ClusterConfiguration gate: a new WorkerGroup field must be given a sentinel or routed to a named test via WORKER_GROUP_STRUCTURAL. Also recorded in AGENTS.md, both rayjobs rule files and the audit doc, as asked. Mutation-checked, six ways: dropping the cluster-env merge, dropping the cluster-label merge, ignoring wg.tolerations, ignoring wg.image, dropping wg extended resources, and adding an unclassified field each fail the suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * RHOAIENG-98942: make the parity doc read as reference, not changelog Review from @chipspeak: either move it under docs/designs/History or drop the language about the previous implementation, since a doc that narrates what changed has to be maintained as a changelog forever. Keeping it in place and taking the second option. docs/designs/History holds one superseded design doc, so it is an archive — and this page is live: AGENTS.md sends contributors here before adding a field, and test_builder_parity.py names it in a failure message. Filing it under History would make both pointers misleading. So the framing is now current-state. Dropped "they were written separately and drifted", "now live in", "the subject of the follow-up to this work" and the @-attribution, retitled off "audit", and led with what the reader needs: read this before adding a field, here is where each one lands, here is which asymmetries are deliberate. The duplicated assemblers are stated as a present constraint with the ticket that tracks them rather than as history. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Pat O'Connor <114754356+chipspeak@users.noreply.github.com>
Summary
Codeflareclass as the single SDK entrypoint — authentication,namespace, retries, timeout, and logging configured once via
SDKConfigcf.clusters.*andcf.jobs.*handler pattern (thin orchestratorsthat delegate to existing
Cluster/RayJobclasses)TokenAuthentication,KubeConfigFileAuthentication,Authentication,KubeConfiguration) — kube-authkit is now the only auth methodUsage