test(oetf): KIND-safe workflow-labels scenario (D1) - #1254
Closed
jiaenren wants to merge 3 commits into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughAdds pod-label prefix configuration and validation across workflow submission and Kubernetes resource creation. Adds a KIND OETF scenario for policy modes, malformed labels, filtering, persistence, and prefix behavior. ChangesWorkflow labels and pod-label prefixes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowLabels
participant WorkflowAPI
participant KubernetesResources
WorkflowLabels->>WorkflowAPI: Submit labeled workflow
WorkflowAPI->>WorkflowAPI: Validate merged label keys and policy
WorkflowAPI->>KubernetesResources: Create or retry task resources
KubernetesResources->>KubernetesResources: Apply pod-label prefix
WorkflowLabels->>WorkflowAPI: Retrieve and filter workflows
WorkflowAPI-->>WorkflowLabels: Return policy results and labels
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
from
July 30, 2026 18:25
cde9d2d to
bd4f40b
Compare
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
from
July 30, 2026 22:31
bd4f40b to
890fb80
Compare
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
from
July 30, 2026 23:47
890fb80 to
1048b3f
Compare
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
3 times, most recently
from
August 3, 2026 18:57
dbd9d4c to
15abaa8
Compare
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
from
August 3, 2026 21:28
15abaa8 to
741c039
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/scenarios/workflow_labels.py (1)
48-53: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuarantee
super().tearDown()runs after the config restore.If the restore PATCH raises,
super().tearDown()is skipped and fixture-level cleanup does not run. Wrap the restore intry/finally.♻️ Proposed refactor
def tearDown(self) -> None: - for workflow_id in self._tracked: - self._cancel(workflow_id) - if self._config_mode == "database": - self._patch_labels_config(self._baseline, "restore baseline") - super().tearDown() + try: + for workflow_id in self._tracked: + self._cancel(workflow_id) + if self._config_mode == "database": + self._patch_labels_config(self._baseline, "restore baseline") + finally: + super().tearDown()🤖 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 `@test/scenarios/workflow_labels.py` around lines 48 - 53, Update tearDown so the database-mode _patch_labels_config restore runs inside a try/finally, ensuring super().tearDown() always executes even when restoring _baseline raises. Keep the workflow cancellation and restore behavior 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 @.github/workflows/oetf-kind.yaml:
- Around line 170-173: Update the workflow-labels comment to remove the claim
that the scenario covers pod stamping or pod-object label assertions, and state
that it runs a labeled workflow to completion while validating the label
specification, admission, filters, and label round-trip behavior.
In `@test/scenarios/workflow_labels.py`:
- Around line 93-104: Update the payload construction in the policy test around
_patch_labels_config so it starts from a copy of self._baseline and merges or
replaces only the policy field. Preserve all other baseline labels_config fields
while retaining the existing policy assertions and mode-specific values.
- Around line 255-265: Update test_label_filters_select_and_exclude so every
_submit_tracked call includes an allowed PPP label value alongside the
experiment labels where applicable. Use the existing policy-compatible label
format and ensure the baseline id_c submission also supplies PPP before
accessing ["name"].
---
Nitpick comments:
In `@test/scenarios/workflow_labels.py`:
- Around line 48-53: Update tearDown so the database-mode _patch_labels_config
restore runs inside a try/finally, ensuring super().tearDown() always executes
even when restoring _baseline raises. Keep the workflow cancellation and restore
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 27f50af5-1f15-465d-bc9b-d8a2a64e5d12
📒 Files selected for processing (4)
.github/workflows/oetf-kind.yamltest/scenarios/BUILDtest/scenarios/workflow_labels.pytest/scenarios/workflow_labels.yaml
Comment on lines
+93
to
+104
| self._patch_labels_config( | ||
| {"policy": [ | ||
| {"key": "PPP", "allow_list": values, "enforcement": mode}]}, | ||
| f"set {mode}") | ||
| policies = self._read_labels_config().get("policy", []) | ||
| self.assertEqual(len(policies), 1, policies) | ||
| # Compare only the fields we set; the model carries extra keys | ||
| # (e.g. assert_message) that default in on read-back. | ||
| applied = policies[0] | ||
| self.assertEqual(applied["key"], "PPP") | ||
| self.assertEqual(applied["enforcement"], mode) | ||
| self.assertEqual(applied["allow_list"], values) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Merge the policy onto the baseline instead of replacing labels_config.
The PATCH sends a complete labels_config object that contains only policy. Every other baseline field of labels_config is therefore dropped while the policy tests run. The read-back assertions check only policy, so the loss stays hidden. Build the payload from a copy of self._baseline.
🐛 Proposed fix
values = ALLOWED_PPP_VALUES if allow_list is None else allow_list
- self._patch_labels_config(
- {"policy": [
- {"key": "PPP", "allow_list": values, "enforcement": mode}]},
- f"set {mode}")
+ labels_config = dict(self._baseline)
+ labels_config["policy"] = [
+ {"key": "PPP", "allow_list": values, "enforcement": mode}]
+ self._patch_labels_config(labels_config, f"set {mode}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self._patch_labels_config( | |
| {"policy": [ | |
| {"key": "PPP", "allow_list": values, "enforcement": mode}]}, | |
| f"set {mode}") | |
| policies = self._read_labels_config().get("policy", []) | |
| self.assertEqual(len(policies), 1, policies) | |
| # Compare only the fields we set; the model carries extra keys | |
| # (e.g. assert_message) that default in on read-back. | |
| applied = policies[0] | |
| self.assertEqual(applied["key"], "PPP") | |
| self.assertEqual(applied["enforcement"], mode) | |
| self.assertEqual(applied["allow_list"], values) | |
| values = ALLOWED_PPP_VALUES if allow_list is None else allow_list | |
| labels_config = dict(self._baseline) | |
| labels_config["policy"] = [ | |
| {"key": "PPP", "allow_list": values, "enforcement": mode}] | |
| self._patch_labels_config(labels_config, f"set {mode}") | |
| policies = self._read_labels_config().get("policy", []) | |
| self.assertEqual(len(policies), 1, policies) | |
| # Compare only the fields we set; the model carries extra keys | |
| # (e.g. assert_message) that default in on read-back. | |
| applied = policies[0] | |
| self.assertEqual(applied["key"], "PPP") | |
| self.assertEqual(applied["enforcement"], mode) | |
| self.assertEqual(applied["allow_list"], values) |
🤖 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 `@test/scenarios/workflow_labels.py` around lines 93 - 104, Update the payload
construction in the policy test around _patch_labels_config so it starts from a
copy of self._baseline and merges or replaces only the policy field. Preserve
all other baseline labels_config fields while retaining the existing policy
assertions and mode-specific values.
Comment on lines
+255
to
+265
| def test_label_filters_select_and_exclude(self) -> None: | ||
| # Use a non-curated key so the PPP policy never rejects these submits. | ||
| tag_a = f"alpha-{self.run_token}" | ||
| tag_b = f"beta-{self.run_token}" | ||
| # submit returns the full workflow id (base name + a "-<job>" suffix), | ||
| # which is what the list echoes, so filter assertions use the returned id. | ||
| id_a = self._submit_tracked( | ||
| self._workflow_name("filter-a"), labels=[f"experiment={tag_a}"])["name"] | ||
| id_b = self._submit_tracked( | ||
| self._workflow_name("filter-b"), labels=[f"experiment={tag_b}"])["name"] | ||
| id_c = self._submit_tracked(self._workflow_name("filter-c"))["name"] |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A baseline enforce policy on PPP breaks the filter test.
These submits omit PPP. validate_workflow_label_policy in src/service/core/workflow/objects.py (lines 1058-1090) raises on a missing key when enforcement is enforce. The comment at line 256 covers only disallowed values, not a missing label. If the target deployment ships an enforcing PPP policy, the submits fail and the subscript ["name"] raises. Submit an allowed PPP value here.
🐛 Proposed fix
id_a = self._submit_tracked(
- self._workflow_name("filter-a"), labels=[f"experiment={tag_a}"])["name"]
+ self._workflow_name("filter-a"), ppp_yaml=ALLOWED_PPP_VALUES[0],
+ labels=[f"experiment={tag_a}"])["name"]
id_b = self._submit_tracked(
- self._workflow_name("filter-b"), labels=[f"experiment={tag_b}"])["name"]
- id_c = self._submit_tracked(self._workflow_name("filter-c"))["name"]
+ self._workflow_name("filter-b"), ppp_yaml=ALLOWED_PPP_VALUES[0],
+ labels=[f"experiment={tag_b}"])["name"]
+ id_c = self._submit_tracked(
+ self._workflow_name("filter-c"),
+ ppp_yaml=ALLOWED_PPP_VALUES[0])["name"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_label_filters_select_and_exclude(self) -> None: | |
| # Use a non-curated key so the PPP policy never rejects these submits. | |
| tag_a = f"alpha-{self.run_token}" | |
| tag_b = f"beta-{self.run_token}" | |
| # submit returns the full workflow id (base name + a "-<job>" suffix), | |
| # which is what the list echoes, so filter assertions use the returned id. | |
| id_a = self._submit_tracked( | |
| self._workflow_name("filter-a"), labels=[f"experiment={tag_a}"])["name"] | |
| id_b = self._submit_tracked( | |
| self._workflow_name("filter-b"), labels=[f"experiment={tag_b}"])["name"] | |
| id_c = self._submit_tracked(self._workflow_name("filter-c"))["name"] | |
| def test_label_filters_select_and_exclude(self) -> None: | |
| # Use a non-curated key so the PPP policy never rejects these submits. | |
| tag_a = f"alpha-{self.run_token}" | |
| tag_b = f"beta-{self.run_token}" | |
| # submit returns the full workflow id (base name + a "-<job>" suffix), | |
| # which is what the list echoes, so filter assertions use the returned id. | |
| id_a = self._submit_tracked( | |
| self._workflow_name("filter-a"), ppp_yaml=ALLOWED_PPP_VALUES[0], | |
| labels=[f"experiment={tag_a}"])["name"] | |
| id_b = self._submit_tracked( | |
| self._workflow_name("filter-b"), ppp_yaml=ALLOWED_PPP_VALUES[0], | |
| labels=[f"experiment={tag_b}"])["name"] | |
| id_c = self._submit_tracked( | |
| self._workflow_name("filter-c"), | |
| ppp_yaml=ALLOWED_PPP_VALUES[0])["name"] |
🤖 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 `@test/scenarios/workflow_labels.py` around lines 255 - 265, Update
test_label_filters_select_and_exclude so every _submit_tracked call includes an
allowed PPP label value alongside the experiment labels where applicable. Use
the existing policy-compatible label format and ensure the baseline id_c
submission also supplies PPP before accessing ["name"].
…abels Operators can now set labels_config.pod_label_prefix (empty by default) to namespace user workflow labels on pods without users typing the prefix on every spec or query. The prefix is prepended to each label key only when the labels are stamped onto pod metadata; the workflow row, list filters, CLI, UI, and service metrics keep using the bare keys, so the submit/query UX is unchanged. The prefix is an opaque string, not assumed to be a DNS prefix: the key and prefix are merged first, then the merged key is validated as a Kubernetes label key at submission (including validation-only submits), so a user key that would form an invalid merged key (e.g. a key that already carries its own prefix) is rejected with a message naming the key, the prefix, and the resulting key. OSS default is empty to keep the deployment neutral; an operator sets the prefix (e.g. "osmo.nvidia.com/") in its own labels_config. System osmo.* labels and env-var derivation are unaffected. - LabelsConfig.pod_label_prefix field + light sanity validator (no whitespace, <=253 chars) - validation.apply_pod_label_prefix / validate_prefixed_workflow_label_keys - submission gate validates merged keys; stamping applies the prefix at both pod-build sites - regenerated openapi.json, generated.ts, generated-mocks.ts - unit tests for the transform, the schema field, and the submission gate Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a KIND-safe workflow-labels scenario and wire it into the oetf-kind CI gate: - test/scenarios/workflow_labels.py — scenario driver - test/scenarios/workflow_labels.yaml — scenario workflow spec - test/scenarios/BUILD — bazel py_test target - .github/workflows/oetf-kind.yaml — run the scenario in the KIND gate Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a KIND-safe case for the pod-label prefix (PR #1259): with pod_label_prefix set, a bare key validates, a key that already carries its own prefix is rejected for forming an invalid merged key, and the workflow API/list still return the bare keys (the prefix is a pod-stamping detail). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
from
August 4, 2026 23:14
741c039 to
22ab003
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
3 tasks
Collaborator
Author
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1254 +/- ##
==========================================
- Coverage 67.00% 64.68% -2.32%
==========================================
Files 203 203
Lines 26109 26136 +27
Branches 3952 3957 +5
==========================================
- Hits 17494 16906 -588
- Misses 7854 8471 +617
+ Partials 761 759 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/utils/connectors/postgres.py`:
- Around line 3003-3013: Update validate_pod_label_prefix to reject every
whitespace character by checking each character with str.isspace(), while
preserving the existing length validation and error behavior. Add regression
coverage for non-ASCII whitespace such as form-feed and non-breaking space in
pod_label_prefix.
In `@src/utils/job/jobs.py`:
- Around line 515-516: Persist the submission-time pod-label prefix on each
Workflow after validate_workflow_label_policy succeeds, then update both
resource-generation sites in src/utils/job/jobs.py (lines 515-516 and 1216-1217)
to use that stored prefix rather than the current configuration. Ensure initial
creation and retry paths apply the same validated prefix to workflow labels.
🪄 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: 31a84ebe-8df4-408e-870c-3942bdda5ac0
📒 Files selected for processing (14)
.github/workflows/oetf-kind.yamlsrc/lib/utils/tests/test_validation.pysrc/lib/utils/validation.pysrc/service/core/workflow/objects.pysrc/service/core/workflow/tests/test_workflow_labels.pysrc/ui/openapi.jsonsrc/ui/src/lib/api/generated.tssrc/ui/src/mocks/generated-mocks.tssrc/utils/connectors/postgres.pysrc/utils/connectors/tests/test_workflow_config.pysrc/utils/job/jobs.pytest/scenarios/BUILDtest/scenarios/workflow_labels.pytest/scenarios/workflow_labels.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/oetf-kind.yaml
- test/scenarios/BUILD
Comment on lines
+3003
to
+3013
| @pydantic.field_validator('pod_label_prefix') | ||
| @classmethod | ||
| def validate_pod_label_prefix(cls, pod_label_prefix: str) -> str: | ||
| # Structure-agnostic sanity only; a whitespace-bearing or oversized | ||
| # prefix would make every label key invalid. Full validity is checked | ||
| # per-key at submission once the prefix is merged with the user's key. | ||
| if any(character in pod_label_prefix for character in ' \t\r\n'): | ||
| raise ValueError('Label pod_label_prefix must not contain whitespace.') | ||
| if len(pod_label_prefix) > 253: | ||
| raise ValueError('Label pod_label_prefix must be at most 253 characters.') | ||
| return pod_label_prefix |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject all whitespace characters in pod_label_prefix.
Line 3009 accepts form-feed and non-breaking-space characters. These values contradict the validation error and make merged pod label keys fail later during submission. Use str.isspace() and add regression cases for non-ASCII whitespace.
Proposed fix
- if any(character in pod_label_prefix for character in ' \t\r\n'):
+ if any(character.isspace() for character in pod_label_prefix):
raise ValueError('Label pod_label_prefix must not contain whitespace.')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pydantic.field_validator('pod_label_prefix') | |
| @classmethod | |
| def validate_pod_label_prefix(cls, pod_label_prefix: str) -> str: | |
| # Structure-agnostic sanity only; a whitespace-bearing or oversized | |
| # prefix would make every label key invalid. Full validity is checked | |
| # per-key at submission once the prefix is merged with the user's key. | |
| if any(character in pod_label_prefix for character in ' \t\r\n'): | |
| raise ValueError('Label pod_label_prefix must not contain whitespace.') | |
| if len(pod_label_prefix) > 253: | |
| raise ValueError('Label pod_label_prefix must be at most 253 characters.') | |
| return pod_label_prefix | |
| `@pydantic.field_validator`('pod_label_prefix') | |
| `@classmethod` | |
| def validate_pod_label_prefix(cls, pod_label_prefix: str) -> str: | |
| # Structure-agnostic sanity only; a whitespace-bearing or oversized | |
| # prefix would make every label key invalid. Full validity is checked | |
| # per-key at submission once the prefix is merged with the user's key. | |
| if any(character.isspace() for character in pod_label_prefix): | |
| raise ValueError('Label pod_label_prefix must not contain whitespace.') | |
| if len(pod_label_prefix) > 253: | |
| raise ValueError('Label pod_label_prefix must be at most 253 characters.') | |
| return pod_label_prefix |
🤖 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 `@src/utils/connectors/postgres.py` around lines 3003 - 3013, Update
validate_pod_label_prefix to reject every whitespace character by checking each
character with str.isspace(), while preserving the existing length validation
and error behavior. Add regression coverage for non-ASCII whitespace such as
form-feed and non-breaking space in pod_label_prefix.
Comment on lines
+515
to
+516
| workflow_labels=validation.apply_pod_label_prefix( | ||
| workflow_obj.labels, workflow_config.labels_config.pod_label_prefix), |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/service/core/workflow/objects.py --items all --match WorkflowSubmitInfo
rg -n -C3 'get_workflow_configs\(\)|pod_label_prefix|apply_pod_label_prefix|validate_prefixed_workflow_label_keys' \
src/service/core/workflow src/utils/job src/utils/connectorsRepository: NVIDIA/OSMO
Length of output: 29454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/service/core/workflow/objects.py --items all --match WorkflowSubmitInfo
ast-grep outline src/utils/job/jobs.py --items all --match 'class Job'
rg -n -C8 'def apply_pod_label_prefix|def validate_prefixed_workflow_label_keys|class Workflow|labels:|workflow_labels|build_workflow_object|construct_workflow_dict|insert_failed_submission_to_db|validate_workflow_label_policy' \
src/service/core/workflow/objects.py src/service/core/workflow src/utils/job/jobs.py src/utils/job/workflow.py src/utils/job/validation.pyRepository: NVIDIA/OSMO
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
Path("src/service/core/workflow/objects.py"),
Path("src/utils/job/jobs.py"),
Path("src/utils/job/workflow.py"),
]
terms = (
"class WorkflowSubmitInfo",
"validate_workflow_label_policy",
"apply_pod_label_prefix",
"validate_prefixed_workflow_label_keys",
"workflow_labels=",
"class Workflow",
"labels:",
"build_workflow_object",
"construct_workflow_dict",
"insert_failed_submission_to_db",
)
for path in files:
lines = path.read_text().splitlines()
hits = [index for index, line in enumerate(lines) if any(term in line for term in terms)]
if not hits:
continue
print(f"\n--- {path} ---")
shown = set()
for hit in hits:
start = max(0, hit - 12)
end = min(len(lines), hit + 20)
key = (start, end)
if key in shown:
continue
shown.add(key)
for index in range(start, end):
print(f"{index + 1:5}: {lines[index]}")
PYRepository: NVIDIA/OSMO
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f -a 'validation.py' src
rg -n -C12 'def apply_pod_label_prefix|def validate_prefixed_workflow_label_keys|def from_workflow_spec|def fetch_from_db|def insert_to_db|def convert_to_pod_spec|class SubmitWorkflow|class Retry' \
src/lib src/utils/job src/service/core/workflowRepository: NVIDIA/OSMO
Length of output: 37178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for range in \
'src/utils/job/workflow.py:865,1045' \
'src/utils/job/jobs.py:430,535' \
'src/utils/job/jobs.py:1120,1250' \
'src/service/core/workflow/objects.py:1058,1098'; do
file=${range%%:*}
lines=${range#*:}
echo "--- $file:$lines ---"
sed -n "${lines}p" "$file"
doneRepository: NVIDIA/OSMO
Length of output: 19893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
needles = (
"apply_pod_label_prefix(",
"validate_prefixed_workflow_label_keys(",
"pod_label_prefix",
)
for path in Path("src").rglob("*.py"):
lines = path.read_text(errors="replace").splitlines()
for number, line in enumerate(lines, 1):
if any(needle in line for needle in needles):
print(f"{path}:{number}:{line.strip()}")
PYRepository: NVIDIA/OSMO
Length of output: 4005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,150p' src/lib/utils/validation.py
sed -n '430,490p' src/lib/utils/tests/test_validation.py
sed -n '1020,1080p' src/utils/job/workflow.py
sed -n '1090,1175p' src/utils/job/workflow.py
sed -n '175,330p' src/utils/job/jobs.pyRepository: NVIDIA/OSMO
Length of output: 23169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
checks = {
Path("src/utils/job/workflow.py"): (
"labels=workflow_spec.labels",
"labels=workflow_spec",
"workflow_spec.labels",
"SELECT * FROM workflows",
"Workflow(**",
),
Path("src/utils/job/jobs.py"): (
"SubmitTask(",
"CreateGroup(",
"prepare_execute(",
"get_kb_specs(",
"_retry_task(",
),
}
for path, needles in checks.items():
lines = path.read_text().splitlines()
print(f"--- {path} ---")
for number, line in enumerate(lines, 1):
if any(needle in line for needle in needles):
start = max(1, number - 8)
end = min(len(lines), number + 12)
print(f"\n[{start}-{end}]")
for index in range(start, end + 1):
print(f"{index:5}: {lines[index - 1]}")
PYRepository: NVIDIA/OSMO
Length of output: 15487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
def merged_key(prefix: str, key: str) -> str:
return f"{prefix}{key}"
def is_kubernetes_label_key(key: str) -> bool:
# The relevant failure condition here is more than one `/` separator.
return key.count("/") <= 1
cases = [
("", "team.example.com/role"),
("osmo.nvidia.com/", "team.example.com/role"),
("osmo.nvidia.com/", "role"),
]
for prefix, key in cases:
merged = merged_key(prefix, key)
print(prefix or "<empty>", key, "=>", merged, "valid:", is_kubernetes_label_key(merged))
PYRepository: NVIDIA/OSMO
Length of output: 367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
def merged_key(prefix: str, key: str) -> str:
return f"{prefix}{key}"
def is_kubernetes_label_key(key: str) -> bool:
return key.count("/") <= 1
cases = [
("", "team.example.com/role"),
("osmo.nvidia.com/", "team.example.com/role"),
("osmo.nvidia.com/", "role"),
]
for prefix, key in cases:
merged = merged_key(prefix, key)
print(prefix or "<empty>", key, "=>", merged, "valid:", is_kubernetes_label_key(merged))
PYRepository: NVIDIA/OSMO
Length of output: 367
Persist the validated pod-label prefix with each workflow before resource generation.
validate_workflow_label_policy validates merged keys with the submission-time prefix, but Workflow stores only raw labels. Both resource-generation paths fetch the current prefix. A later change can produce osmo.nvidia.com/team.example.com/role, which Kubernetes rejects during initial creation or retry. Store the prefix with the workflow and use it at both sites, or revalidate each merged key before resource creation.
📍 Affects 1 file
src/utils/job/jobs.py#L515-L516(this comment)src/utils/job/jobs.py#L1216-L1217
🤖 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 `@src/utils/job/jobs.py` around lines 515 - 516, Persist the submission-time
pod-label prefix on each Workflow after validate_workflow_label_policy succeeds,
then update both resource-generation sites in src/utils/job/jobs.py (lines
515-516 and 1216-1217) to use that stored prefix rather than the current
configuration. Ensure initial creation and retry paths apply the same validated
prefix to workflow labels.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue - None
Adds an OETF end-to-end scenario for the workflow-labels feature, run in the KIND gate. It exercises the feature purely through the OSMO API (a sandboxed OETF test has no in-cluster kubeconfig):
warnsurfaceswarnings,enforcerejects with 400 and leaves no row,offaccepts anything.label, glob, andno_label.pod_label_prefix): the prefix is validated against the merged key at submission (a bare key merges to a valid Kubernetes key; a key that already carries a prefix is rejected), and never leaks into the workflow API/list, which keep the bare keys.Tagged
kind, so it runs in theoetf:deploy_and_run --env kindgate (which--build-locals the stack and deploys it to KIND in DB mode). Serial + exclusive because the policy tests mutatelabels_config, restoring the baseline intearDown; on a ConfigMap-mode target those mutation testsskipTest, so the scenario is also portable to non-KIND deployments.Scope note (pod-object verification)
A literal in-cluster pod-label assertion is not reachable from the gate: OETF scenarios run via
bazel testwith onlyOETF_URL+ auth +OETF_POOLforwarded, so the test can't reach the KIND cluster with kubectl. The KIND gate also doesn't run workflows to completion, so the end-to-end test asserts the label round-trip (submit → persisted labels echoed + filterable), not the run outcome. The actual pod stamping (apply_workflow_labels/ the prefix application) is covered by unit tests; a true pod-object check would need kubeconfig forwarded into the OETF test env — a small runner change we can add later if wanted.Verification
bazel build //test/scenarios:workflow-labels(and its pylint target) pass locally.oetf:deploy_and_run --env kind) runs on this PR.Checklist
🤖 Generated with Claude Code