Update gitlab submodule by fernandol-nvidia · Pull Request #2 · NVIDIA/OSMO

fernandol-nvidia · 2025-11-07T15:34:40Z

Description

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

fernandol-nvidia · 2025-11-14T21:20:31Z

Already merged from upstream.

#1 (CRITICAL): Store watcher on app state to prevent GC from killing the file watcher after configure_app() returns. #9: Activate ConfigMap mode on deferred successful reload. If initial load fails but a later watchdog-triggered reload succeeds, 409 write protection now activates correctly. #3: Move reject_if_configmap_mode() before loops in 6 bulk endpoints (put_pools, put_pod_templates, put_group_templates, put_resource_validations, put_roles, put_backend_tests). Guard doesn't depend on loop variable. #4: Fix Go race on lastModTime in FileRoleStore. Move write inside mu.Lock in Load(), add mu.RLock for read in Start(). #2: Fix import ordering in postgres.py — move configmap_state to first-party import group. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat: add ConfigMap-sourced dynamic configuration loader Add support for loading dynamic configs from a Kubernetes ConfigMap on service startup, enabling GitOps workflows where configs are version- controlled in Helm values and applied automatically via ArgoCD. Two managed_by modes per config type: - seed (default): only apply if config doesn't exist in DB - configmap: always overwrite DB from ConfigMap on startup Includes: - configmap_loader.py: core loader with dependency ordering, advisory lock for multi-replica safety, secret file resolution for dataset credentials, and per-type error isolation - dynamic_config_file field on WorkflowServiceConfig (CLI + env var) - Helm chart: ConfigMap template, Secret template for credentials, volume mounts, checksum annotation for pod restart on config change Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address bugs and code quality issues in configmap_loader - Fix crash when managed_configs is None (empty YAML section) - Fix partial state corruption in _resolve_dataset_secret_files by validating secret file content before mutating credential dict - Add configmap_loader.py and pyyaml dep to BUILD file - Add type annotations for apply_function, model_class params - Chain exception in _parse_managed_by (raise from error) - Use RETURNING clause in _insert_backend to skip history entry on conflict instead of always creating one - Add schema reference comment to _insert_backend - Warn on unknown keys in managed_configs - Update copyright years to 2025-2026 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add comprehensive tests for ConfigMap config loader Add unit tests (20 tests) and integration tests (18 tests) for the configmap_loader module. Unit tests cover file handling, YAML parsing, managed_by mode parsing, secret file resolution, safe_apply error isolation, advisory lock behavior, and unknown key warnings. Integration tests verify actual DB interactions including seed vs configmap mode for all config types, dependency ordering, per-item error isolation, end-to-end loading, and config history entries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review findings, add config watcher, fix test failures - Advisory lock: session-scoped -> transaction-scoped (pg_try_advisory_xact_lock) to prevent lock leaks on process kill - Seed mode check: use config_history table instead of exclude_unset to correctly detect if config was ever explicitly set - Config watcher: background polling thread (30s default) with SHA-256 hash comparison detects ConfigMap file changes without pod restart - Roles: pre-construct RolePolicy objects for pydantic v1 compatibility - Dataset: auto-populate credential endpoint from bucket dataset_path - Credential logging: removed secret file path from log messages - Tests: fixed fetch_from_db return types, semantic action format, advisory lock assertions, unused imports, mypy annotations - E2E: added test values overlay and dynamic config test file Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * simplify: consolidate singleton config functions, fix advisory lock - Consolidate _apply_service_config, _apply_workflow_config, _apply_dataset_config into single _apply_singleton_config with config_type parameter and optional pre_apply hook (dataset uses it for secret resolution) - Fix advisory lock: revert to session-scoped pg_try_advisory_lock with explicit unlock in finally block. Transaction-scoped xact lock was released immediately by execute_fetch_command's auto-commit, providing zero mutual exclusion. - Replace _backend_exists with _named_config_exists (eliminates duplicate) - Use SELECT 1 LIMIT 1 instead of COUNT(*) in _singleton_config_exists - Remove duplicate _DEFAULT_POLL_INTERVAL_SECONDS constant - Add WHY comment for endpoint defaulting from dataset_path - Update unit tests for session lock assertions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: add drift reconciliation for configmap-mode configs The ConfigMapWatcher now has two-tier polling: 1. File change detection (existing): SHA-256 hash comparison on the mounted ConfigMap file. When the file changes, re-apply everything. 2. Drift reconciliation (new): for managed_by=configmap singleton configs (service, workflow, dataset), compare the last-applied ConfigMap values against current DB state on each poll cycle. If someone modified a config via CLI, the watcher detects the drift and re-applies the ConfigMap values within one poll interval. Key design decisions: - Only reconciles singleton configs (not named configs like pools) to limit DB query load per cycle - Compares desired values against DB BEFORE calling patch_configs, so no config_history entries are created when there's no drift - Uses a separate advisory lock (configmap-reconcile) to prevent multiple replicas from correcting the same drift simultaneously - Resolves secret_file references in the cached config so dataset drift detection compares resolved credentials, not file paths - Seed-mode configs are never reconciled (by design) Also refactors module-level start_config_watcher + global state into a ConfigMapWatcher class with clean instance state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: add UX improvements — managed_by visibility, immediate re-apply, credentialSecretName Improvement 1: Management mode visibility - Config GET endpoints (service, workflow, dataset) include _managed_by field in responses ('configmap', 'seed', or absent) - Managed modes persisted to configmap_state table on startup - get_managed_mode() and get_cached_section() exposed for cross-module access Improvement 2: Immediate re-apply after CLI write - When patch_configs writes to a configmap-managed singleton config, the ConfigMap values are immediately re-applied (inside helpers.py) - Only triggers for non-configmap-sync writers (prevents infinite loops) - Eliminates the need to wait for 30s drift reconciliation poll Improvement 3: Simplified secret wiring (credentialSecretName) - Dataset buckets can now use credentialSecretName instead of secret_file - Helm template auto-generates volume + volumeMount for referenced secrets - Loader resolves credentialSecretName to /etc/osmo/secrets/<name>/cred.yaml - No manual extraVolumes/extraVolumeMounts needed Also adds configmap_state table to postgres.py and test schema. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: reject CLI writes to configmap-managed configs with 409 Conflict Replace accept-then-revert pattern with 409 rejection for all config types. When a user tries to modify a config managed by ConfigMap in configmap mode, the API returns 409 Conflict with a clear error message instead of silently reverting after the write. Singleton configs (service, workflow, dataset): - PUT/PATCH endpoints reject with 409 before any DB write - Removed the accept-then-revert hook from helpers.py patch_configs() Named configs (pools, pod_templates, backends, roles, etc.): - 19 single-item PUT/PATCH/DELETE endpoints reject with 409 - Bulk PUT endpoints (put_pools, put_pod_templates, etc.) are NOT guarded because the configmap_loader uses them internally Benefits over accept-then-revert: - Zero race conditions (no concurrent re-apply with watcher) - Zero duplicate history entries (write never happens) - Zero wasted side effects (no backend queue syncs, K8s updates) - Clear API feedback ("update Helm values instead") Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: extract configmap_guard module, fix circular import, cover all write paths Extract 409 guard logic into configmap_guard.py — a standalone module with no imports from helpers.py or configmap_loader.py. This eliminates the circular import (helpers -> configmap_loader -> helpers) that required import-outside-toplevel violations. Architecture: - configmap_guard.py: holds managed config state, guard functions, constants - configmap_loader.py: delegates guard functions to configmap_guard, pushes state via set_managed_configs() on load_and_apply() - helpers.py: imports configmap_guard at top level (no circular import) - config_service.py: imports configmap_guard for _add_managed_by and named item guards Coverage: - Singletons: guarded in helpers.put_configs() and helpers.patch_configs() (covers all singletons including rollback) - Named items: guarded in all 19 single-item endpoints + 6 bulk endpoints + rollback endpoint in config_service.py - configmap-sync bypass: reject_if_managed() checks username and skips for the loader's own writes Also: - Eliminated triple file read in load_and_apply() (reads once, computes hash + parses YAML from same bytes) - Removed dead get_configmap_state() from postgres.py - Updated drift test to expect 409 rejection instead of accept-then-revert Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: remove debug print, dead code, stale comment - Removed debug print(test_configs) from helpers.py - Removed unused re-export delegations from configmap_loader.py (get_managed_mode, get_cached_section, etc. — these are accessed directly from configmap_guard, not via configmap_loader) - Removed stale comment about circular imports Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: rollback guard referenced removed configmap_loader.reject_if_managed The rollback_config endpoint still called configmap_loader.reject_if_managed which was removed when extracting to configmap_guard. Fixed to use configmap_guard.reject_if_managed. Also removed unused configmap_loader import from config_service.py. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add design doc for ConfigMap-sourced dynamic configuration Covers architecture, management modes, 409 rejection, drift reconciliation, credentialSecretName, and full E2E test results from live dev instance validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: guard watcher startup to avoid service boot abort Wrap ConfigMapWatcher.start() in try/except so a malformed ConfigMap file or DB error during initial config load doesn't prevent the service from starting. The service logs the error and continues without ConfigMap config management. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: extend secret file references to all config credential fields Generalize the dataset-only _resolve_dataset_secret_files() into a recursive _resolve_secret_file_references() that works for all config types. Any dict with a 'secret_file' or 'secretName' key at any nesting level gets resolved by reading the mounted K8s Secret file. Supports two patterns: - Dict secrets (credentials): file contents merged into the dict - Simple string secrets (tokens, passwords): file with {value: "..."} replaces the dict entirely with the string value Helm templates updated: - _helpers.tpl: recursive secretName→secret_file transformer + secret name collector (4 levels deep) - dynamic-config.yaml: resolves secretName in workflow/service config - api-service.yaml: auto-generates volume + volumeMount for all secretName references across all config types - values.yaml: documents secretName pattern for workflow credentials Example Helm values: workflow: config: workflow_data: credential: secretName: osmo-workflow-data-cred workflow_alerts: slack_token: secretName: osmo-slack-token Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: auto-detect Docker registry .dockerconfigjson secret format The secret file resolver now auto-detects three formats: 1. Simple string: {value: "..."} → replaces dict with string 2. Docker registry: {auths: {registry: {username, auth}}} → extracts registry, username, auth fields automatically 3. YAML dict: merges all keys into current dict (existing behavior) Added secretKey field alongside secretName to specify which key in the K8s Secret to mount (defaults to "cred.yaml"). For Docker registry secrets, use secretKey: .dockerconfigjson. Example: backend_images: credential: secretName: imagepullsecret secretKey: .dockerconfigjson This lets users reuse existing imagepull secrets without creating a duplicate secret in a different format. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: redesign ConfigMap config to in-memory + watchdog + global mode Major redesign based on team feedback: 1. Single global toggle (no per-config managed_by seed/configmap) - dynamicConfig.enabled: true = ConfigMap mode (all writes blocked) - dynamicConfig.enabled: false = DB mode (CLI/API works normally) 2. In-memory config serving (standard K8s pattern) - ConfigMap file parsed on startup, cached in module-level dict - All config reads served from memory, not DB - DB only for runtime state (agent heartbeats, k8s_uid) + roles - Runtime fields (service_auth) injected from DB on first load 3. Watchdog file events replace 30s polling - Uses watchdog library (already a dep) with inotify - Watches parent dir for K8s ConfigMap symlink swaps - 2s debounce for atomic swap events 4. All-or-nothing validation before applying - Pydantic model validation for singleton configs - Structure validation for named config sections - On failure: previous config preserved, error logged 5. Simplified 409 write protection - Single reject_if_configmap_mode(username) function - 29 guard calls across config_service.py + helpers.py - Bug fix: added missing delete_dataset guard - Bug fix: simplified rollback guard New: src/utils/configmap_state.py (dependency-free state module) Modified: 11 files, ~900 lines added, ~2000 lines removed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: rewrite design doc for v2 architecture Updated to reflect redesigned ConfigMap config system: - Global mode (no per-config managed_by) - In-memory serving (standard K8s pattern) - Watchdog file events (no polling) - Config vs runtime data separation - All-or-nothing validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: authz_sidecar reads roles from ConfigMap file instead of DB Add --roles-file flag to authz_sidecar. When set, the sidecar reads roles, external role mappings, and pool names from the ConfigMap-mounted YAML file instead of PostgreSQL. This eliminates the DB dependency from the authz_sidecar in ConfigMap mode. New: src/utils/roles/file_loader.go - FileRoleStore loads roles + external_roles from YAML - In-memory reverse map: externalRole -> []osmoRole - ResolveExternalRoles replaces SyncUserRoles SQL query - Poll-based file watching (os.Stat every 30s) Modified: authz_server.go - NewFileBackedAuthzServer constructor (no pgClient needed) - Check(): file-backed ResolveExternalRoles vs DB SyncUserRoles - resolveRoles(): file store vs DB for role policy fetches - computeAllowedPools(): file store vs DB for pool names - MigrateRoles(): skipped in file-backed mode Modified: main.go - --roles-file flag for ConfigMap mode - Conditional init: file-backed vs DB-backed server - DB connection only created when needed Modified: configmap_loader.py - Removed _write_roles_to_db (authz_sidecar reads file directly) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: authz_sidecar uses --roles-file when dynamicConfig enabled Helm template conditionally passes --roles-file to the authz-sidecar container and mounts the dynamic-config ConfigMap volume. When dynamicConfig.enabled=true, postgres args are omitted (not needed). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: move product defaults to chart values, auto-derive service_base_url Chart values.yaml now ships with: - Default workflow limits (max_num_tasks, timeouts) - Default pod templates (ctrl, user) - Default resource validations (cpu, memory) - All 5 RBAC roles with policies (admin, user, default, ctrl, backend) service_base_url auto-derived from services.service.hostname in the dynamic-config template — no need to set it per deployment. Per-deployment values only need site-specific configs: - Dataset buckets + credentials - Backend definitions - Workflow image credentials - CLI version info Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: add default backend and pool to chart defaults Matches configure_app() behavior: default backend with kai scheduler, default pool referencing default backend with default platform. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: update design doc for v3 — authz file-backed, chart defaults - Authz sidecar reads roles from ConfigMap file (FileRoleStore) - IDP groups as source of truth for user role assignments - Product defaults (roles, templates, validations, backend, pool) in chart - Per-deployment values only for site-specific overrides - Updated architecture diagram, component table, E2E test results Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: eliminate remaining DB deps in ConfigMap mode - Role.list_from_db/fetch_from_db now check snapshot (was missing) - ConfigMapWatcher.postgres is optional (None when DB not available) - _inject_runtime_fields skips DB read when postgres is None - All config types now served from in-memory when ConfigMap mode active Remaining DB access in ConfigMap mode is ONLY: - Backend heartbeats (agent writes, pool status reads) - configure_app() startup (runs before ConfigMap mode activates) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: fix medium review findings — remove stale docs and dead code - Remove stale managed_by mode comments from values.yaml (v1/v2 leftover) - Remove unused dynamic_config_poll_interval field from WorkflowServiceConfig (Python watcher uses watchdog events, Go sidecar has its own constant) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: flatten ConfigMap YAML — remove managed_configs/config/items nesting ConfigMap format now matches API response format directly: - Removed `managed_configs:` top-level wrapper - Removed `config:` sub-key from singleton types (service, workflow, dataset) - Removed `items:` sub-key from named types (pools, roles, templates, etc.) Before: snapshot['service']['config']['cli_config'] After: snapshot['service']['cli_config'] Before: snapshot['pools']['items']['default'] After: snapshot['pools']['default'] This means `osmo config show` output can be pasted directly into Helm values with no format translation. 7 files changed: Helm template, Helm values, Python loader, postgres.py (15 interceptions), Go file_loader, unit tests, integration tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: clean up v1/v2 leftovers — remove dead code and stale references - Delete unused set_configmap_state() method from postgres.py - Remove dead CONFIGMAP_SYNC_USERNAME/TAGS imports from loader (loader no longer writes to DB) - Remove managed_by fields from example values (v3 uses global toggle) - Flatten example values to match v3 structure (no config:/items:) - Remove TestDynamicConfigSeedMode E2E test class (seed mode removed in v3) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: correct YAML indentation in chart values after items: removal The children of removed items:/config: wrappers kept their extra indentation, causing YAML parse errors in helm template. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: update api-service.yaml for flat config structure Remove .config. and .items. from dynamicConfig references in api-service.yaml template (volume mounts for secrets). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add dynamic-config volume to agent and logger deployments The authz-sidecar container mounts the dynamic-config ConfigMap for file-backed role reading. The volume was only defined in api-service.yaml but the sidecar is also included in agent and logger deployments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add checksum annotation to agent/logger, remove duplicate guards K8s/Helm review findings: - Add checksum/dynamic-config annotation to agent and logger pods so they restart on ConfigMap changes (was only on api-service) - Remove duplicate {{- if }} guards in api-service.yaml secret collection (copy-paste artifact) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: rename dynamicConfig → configs across codebase Rename the feature from "dynamicConfig" to "configs" to match industry conventions (ArgoCD uses "configs:"). Helm values: services.dynamicConfig → services.configs ConfigMap name: osmo-service-dynamic-config → osmo-service-configs Mount path: /etc/osmo/dynamic-config/ → /etc/osmo/configs/ CLI arg: --dynamic_config_file → --config_file Env var: OSMO_DYNAMIC_CONFIG_FILE → OSMO_CONFIG_FILE 14 files, mechanical rename, no logic changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: replace recursive secret templates with explicit secretRefs Pattern 2: secrets declared separately from config, not inline. Before (80 lines of manually unrolled 4-level recursion): - osmo.resolve-secret-names-in-config: walked config tree to transform secretName → secret_file in the ConfigMap YAML - osmo.collect-secret-names: walked config tree to find all secretName references for volume mount generation - dynamic-config-secrets.yaml: created K8s Secrets from inline values After (simple flat list): - secretRefs: list of {secretName} in values.yaml - Template iterates list for volume + volumeMount generation - secretName passed through to ConfigMap as-is - Python loader resolves secretName → file path at runtime - No inline secret creation (secrets managed out-of-band) Deleted: dynamic-config-secrets.yaml, resolve-secret-names-in-config, collect-secret-names, resolve-secret-name templates (~100 lines removed) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: rename dynamic-config files to configs - dynamic-config.yaml → configs.yaml (Helm template) - configmap-dynamic-config.md → configmap-configs.md (design doc) - osmo_dynamic_config_values.yaml → osmo_configs_values.yaml (example) - test_dynamic_config.py → test_configs.py (E2E test) - Updated internal references in renamed files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: cleanup dead code and stale references from review - Remove unused CONFIGMAP_SYNC_TAGS constant - Remove unused typing imports from configmap_guard - Fix integration test: remove stale managed_configs wrapper (#1) - Fix docstrings: remove "write roles to DB", "authz reads from DB" - Remove dead configmap_state table from postgres.py and schema.sql - Remove stale comment about deleted secret templates in _helpers.tpl - Rename $dc → $cfg in configs.yaml template Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * perf: bypass caches in file-backed authz mode In file-backed mode, FileRoleStore.GetRoles() and GetPoolNames() are already in-memory map lookups. The LRU role cache and pool name cache add overhead (locking, TTL) for zero benefit. - resolveRoles: direct file store lookup, skip role cache - computeAllowedPools: direct file store lookup, skip pool name cache - Caches only used in DB mode where they avoid DB round-trips Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: optimize loader + remove dead code configmap_loader.py: - Remove unnecessary copy.deepcopy (yaml.safe_load returns fresh dict) - Remove stale comments (circular import note, "kept from original") - Fix remaining "Dynamic config" log message - Remove extra blank lines file_loader.go: - Remove unused GetAllRoleNames() method (never called) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * perf: don't allocate caches in file-backed authz mode roleCache and poolNameCache are unused in file-backed mode (all lookups go directly to in-memory FileRoleStore). Don't create them. - NewFileBackedAuthzServer no longer accepts cache params - initFileBackedServer no longer creates caches - Caches only allocated in DB mode where they serve a purpose Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR review findings #1, #2, #3, #4, #9 #1 (CRITICAL): Store watcher on app state to prevent GC from killing the file watcher after configure_app() returns. #9: Activate ConfigMap mode on deferred successful reload. If initial load fails but a later watchdog-triggered reload succeeds, 409 write protection now activates correctly. #3: Move reject_if_configmap_mode() before loops in 6 bulk endpoints (put_pools, put_pod_templates, put_group_templates, put_resource_validations, put_roles, put_backend_tests). Guard doesn't depend on loop variable. #4: Fix Go race on lastModTime in FileRoleStore. Move write inside mu.Lock in Load(), add mu.RLock for read in Start(). #2: Fix import ordering in postgres.py — move configmap_state to first-party import group. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve CI failures after merge + add test coverage Fixes: - Remove PrettyJSONResponse (removed on main in pydantic v2 migration) - Fix pylint: rename _config_watcher, narrow exception catch - Fix mypy: use Any type for ResourceAssertion snapshot returns - Fix assertion: compare scheduler_type enum .value not object - Remove accidentally committed ui/node_modules file - Remove run/minimal/osmo_configs_values.yaml (rework later) Tests (14 new): - 8 Go tests for file-backed authz (Check, resolveRoles, computeAllowedPools, MigrateRoles, external role resolution) - 6 Python tests for snapshot read paths (GroupTemplate, Role, Backend list/names) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Code-review #2: when an importer says `from src.lib.data.storage import StorageBackend`, the previous fan-in resolver was adding +1 to *every* file that storage/__init__.py re-exports from — even though the importer only ever references backends/common.py. The package only matters as a value when the importer actually binds it (via `import X` or `from X.parent import X`), not when X is the source of named symbols. Refactor `_python_imports` to return `(direct, package_use)`: - `direct` — every dotted name that *might* resolve to a single file. Source `X` from `from X import Y` plus the composed `X.Y`, plus the whole-name target of `import X.Y.Z`. - `package_use` — names the importer binds *as a value*. Populated by `import X.Y.Z` (top-level package binding) and the composed `X.Y` from `from X import Y` (Y could be a sub-package the importer accesses as `Y.something`). Crucially **not** the source `X` of `from X import Y, Z` — in that form X is never referenced as a value. `build_python_fan_in` now consumes both: direct hits credit single files; package re-exports expand only for `package_use` entries that didn't already resolve to a file. Test added: `test_named_import_does_not_overcredit_package_siblings` asserts client.py stays at 0 when only StorageBackend was imported. Top-10 dry-run on main is unchanged (the over-credit was distributed evenly across packages so removing it shifted everyone proportionally). The fix matters more as __init__.py re-exports grow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* testbot: rank coverage targets by criticality x coverage gap Pure-coverage ranking let trivial low-coverage files dominate the budget — PR #939 picked a one-off file at 0% over heavily-imported public APIs at 30%. Replace the single-pass selector with a two-stage pipeline. Stage 1 (criticality_scorer.py): combines Codecov coverage with three cheap static signals — Python AST + Go reverse imports for fan-in, six-month git churn, and a path-tier classification keyed off AGENTS.md. Scores files by score = criticality * (1 - coverage) * log(uncovered_lines) with log normalization on fan-in/churn so a single mega-hub doesn't swamp the rest. Outputs a top-20 JSON shortlist with a per-signal breakdown so reviewers can see why each file ranked where it did. Stage 2 (select_targets_agent.py + SELECT_TARGETS_PROMPT.md): a small read-only Claude subagent (Read,Glob,Grep, 30-turn cap) reads each candidate and picks the 1-3 files where unit tests would have the highest ROI. Rejects heavy I/O glue, long-running orchestration, and SDK-call delegators. Can return zero picks rather than burn the budget on low-ROI work. Final picks plus the LLM rationale flow into the existing TESTBOT_PROMPT.md / create_pr.py contract — no downstream changes needed. Dry-run on main shortlists src/lib/{rsync,data/storage,utils}, src/utils/{job,roles}, and src/service/core/{data,workflow} — exactly the kind of files we want testbot writing tests for. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: set PYTHONPATH=. so criticality_scorer can resolve src.* imports The Stage-1/Stage-2 scripts use namespace-package imports (`from src.scripts.testbot.coverage_targets import ...`) which need the repo root on sys.path. CI invokes them as plain scripts, where Python adds the script's directory — not the repo root — to sys.path. Mirror the existing `PYTHONPATH=src/scripts python -m testbot.create_pr` pattern but at the repo-root level so the existing import paths work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: drop fragile inline shortlist printer The Python f-string with escaped double quotes broke under YAML+shell quoting (`SyntaxError: unexpected character after line continuation`). criticality_scorer.py already prints the ranked shortlist via logger.info in its main(), so the inline pretty-printer was redundant. Replace with a `cat` of the JSON so the full per-signal breakdown is still one click away in the workflow log. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: stream + diagnostics for Stage-2 target picker Mirror the foldable stream + diagnostics pattern the generate step uses so the target picker is debuggable in the workflow log: - Run npx with `--output-format stream-json --verbose` and tee the JSONL into a tempfile while echoing each line live. - Wrap the live stream in `::group::Claude Code stream — target picker` so the per-turn JSON doesn't dominate the default view. - After completion, parse the final `result` event in reverse and print exit_status + subtype + is_error + num_turns + duration_ms + duration_api_ms + total_cost_usd + truncated result text. - Wrap diagnostics in `::group::` + `::stop-commands::<token>` so untrusted result text can't inject `::warning::`, `::add-mask::`, or other workflow commands. - Use the parsed `result.result` string as the agent text fed into parse_agent_output, replacing the previous capture_output approach. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: resolve __init__.py re-exports + multi-name from-imports for fan-in OSMO uses three import patterns the naive AST scan missed, all of which caused fan_in=0 for genuinely heavily-imported files (lib/utils/client.py read 0 before this; it's actually 85): 1. `from src.utils.job import jobs, workflow, task` — record each imported name as a candidate submodule (not just the source package). 2. `from src.lib.data.storage import Client` — follow `__init__.py` re-exports (`from .client import Client`) so `Client` aliases to client.py. 3. `from src.lib.data import storage` — credit every file the storage/__init__.py imports from (Python runs __init__.py on package import, so the importer's fate is tied to all of them). Dedup resolved targets per importer so a file that imports both `Client` and `SingleObjectClient` adds 1 to client.py, not 2. Dry-run on main now shows lib/data/storage/client.py at rank 2 with fan_in=70 (was missing entirely before), lib/rsync/rsync.py at rank 3 with fan_in=5, and lib/data/dataset/manager.py at rank 11 with fan_in=5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: include uncovered_lines in criticality_scorer log output The shortlist log printed coverage % but not the absolute uncovered line count. Both matter — a 0%-covered 30-line file and a 0%-covered 800-line file have very different testing surfaces, and the score formula already weighs them differently. Surface the count next to the % so operators can see why two files at the same coverage rank differently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: bump fan_in weight 1.5->2.5 and uncovered cap 300->500 The previous defaults under-weighted dependency centrality: lib/utils/ common.py (fan_in=222 — by far the most-imported module in OSMO) ranked #13, despite its 45% coverage being the floor on a 1266-line file (~696 uncovered lines). Two adjustments: - fan_in weight 1.5 -> 2.5: dependency centrality is the most durable signal of criticality. A hub stays a hub for years, while coverage gaps and recent churn shift week to week. Both fan_in and churn are still log-normalized in score_entry so the heaviest hub's contribution is bounded at the weight value itself. - uncovered_lines cap 300 -> 500: a 1266-line file with 700+ uncovered lines was being clipped to 300, throwing away signal that it has substantially more surface area than a 300-line file at the same coverage %. Dry-run effect: lib/utils/common.py rises #13 -> #8. The top 5 stay the same backend-foundation files (utils/job/*, lib/data/storage/ client.py, lib/utils/client.py) because their coverage gap is wider — the multiplicative score formula keeps "important AND under-covered" ahead of "very important but already half-covered." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: simplify — consolidate Codecov parsing + markdown formatting Three reviewer agents flagged duplication across coverage_targets, criticality_scorer, and select_targets_agent. Fix the high-value items: * Extract `parse_coverage_entries()` in coverage_targets.py and reuse it from criticality_scorer.py — kills the 35-line copy of the Codecov line-coverage parsing loop. select_targets and rank_targets now layer their own ordering/filtering on the shared parser. * Extend `coverage_targets.format_targets()` with an optional empty_message and a `Why this file:` line (read off `target.reason`) so select_targets_agent.format_targets_markdown collapses to a one-line wrapper. * Add CODECOV_LINE_HIT/CODECOV_LINE_MISS constants — kills the last magic-number 0/1 branch in the parser. * Pre-compute `path -> rel_str` in build_python_fan_in so the inner loop doesn't re-call relative_to() per file (~220 redundant conversions on the OSMO repo). * Single-pass classification in build_go_fan_in: package_files and package_imports are populated in one loop instead of two `_go_package_for_file` calls per file. * Replace `assert proc.stdout is not None` with a runtime RuntimeError — assertions can be stripped under -O and shouldn't gate production behavior even when the path is unreachable. * Drop two narrating comments per repo style (MIN_LOC explainer and 1-level-relative rationale). Verified: all 17 testbot bazel targets still pass; dry-run on main yields identical top-5 scores and ranking. Net diff: 74 insertions, 106 deletions across the three files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: clarify scoring formula in README Replace the one-line score formula with a structured ASCII diagram + per-term table so each component's contribution and bound is obvious without reading the source. Adds a short rationale for the two non-obvious modeling choices (multiplicative score, log normalization on fan_in/churn). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: split scoring formula into three equations The combined ASCII diagram with bracketed labels was hard to parse — criticality and coverage_gap appeared on the same line joined by a mid-line `×`. Split into three separate equations so each piece reads left-to-right on its own. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: prune ignored subtrees + resolve re-exports to package __init__ Two issues from review, both verified against current code: 1. **Pruned filesystem walk.** _walk_files used rglob('*') which descended into every subtree before filtering — including src/ui/node_modules (~645MB), .git, and any .claude/worktrees/ copies of the repo a developer might have. Replace with a single os.walk-based generator that prunes ignored subtrees in place (PRUNE_DIR_NAMES at any depth, PRUNE_ROOTED_DIRS at the top). Both _walk_files and _walk_init_files now share this iterator. Real fix: my locally-developed branch had .claude/worktrees/ from a prior agent run, which the old rglob descended into. That inflated fan-in counts ~5x because every src/ file appeared twice in the importer set. CI (no .claude/) was always seeing the correct, lower numbers — this commit makes local runs match. 2. **Re-export to package __init__.py fallback.** The relative-import resolver in _re_export_targets always set target_file to `<module>.py`. For `from .backends import construct_storage_backend` in storage/__init__.py, this pointed at a phantom src/lib/data/storage/backends.py — but that file doesn't exist; only backends/__init__.py does. Try the .py target first, fall back to <package>/__init__.py if it exists. Phantom paths no longer leak into module_to_path or package_re_exports. Tests: 17/17 testbot bazel targets still pass. Two new tests cover the package-init fallback case and the phantom-path absence invariant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: harden Stage-2 pick handling — non-dict guard + max_targets cap Two findings from review, both verified: 1. **Non-dict picks.** parse_agent_output() validates only that `targets` is a list, not that each element is a dict. If the model ever emits a list of strings (e.g. `["src/lib/foo.py", ...]`) the loop `for pick in picks: pick.get("file_path")` would raise AttributeError mid-run. Add an isinstance(pick, dict) guard with the same warn-and-skip pattern used for missing file_path. 2. **max_targets ceiling.** The prompt says "at most {max_targets}" but if the agent returns more we currently merge all of them. After parse_agent_output, slice picks to args.max_targets and log a warning when truncation happens. The downstream pipeline now never sees more targets than configured. Tests: 17/17 testbot bazel targets pass. Added a non-dict-pick test with strings, ints, and None. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: drain Claude CLI stdout from a background thread so timeout fires Verified: the synchronous `for line in proc.stdout: ...` loop blocked until the agent closed its pipe, which meant `proc.wait(timeout=...)` ran *after* the process had already exited and could never raise TimeoutExpired during a hang. If the agent stalled without writing, the wrapper would hang forever despite the timeout argument. Spawn a daemon thread to drain stdout (echoing to sys.stdout and the JSONL log file), then call proc.wait(timeout=timeout_sec) from the main thread. On TimeoutExpired we kill the process, reap it with proc.wait(), and join the drain thread with a 2s ceiling so a stuck reader can't block teardown. On clean exit we still join the drain thread so the last lines flush before the function returns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: harden _stream_npx — encoding, drain race, exit path, timeout test Code-review feedback: - **Drain/teardown race**: on the success path drop the 2.0s ceiling on drain.join() — the pipe closes when the process exits, the for-loop sees EOF, and the thread completes promptly. Bounding the join risked closing logf with the daemon thread mid-write, which could lose the final `result` event under exactly the conditions the timeout was meant to catch. On the timeout path keep the bounded join (process is dead, pipe is closed, thread should exit fast) and wrap the drain loop in a ValueError/OSError except so a teardown race can't crash the daemon. - **Encoding**: pass `encoding="utf-8", errors="replace"` to Popen. With `text=True` alone we were inheriting locale.getpreferredencoding — fine on the GitHub-hosted Ubuntu runner today, but a single non-UTF-8 byte from the agent would have killed the daemon thread silently. - **Diagnostics on unreachable Popen-no-stdout branch**: convert the `raise RuntimeError(...)` into `return 1` + log so the diagnostics block in invoke_claude still emits before exit. - **Timeout integration test**: TestStreamNpxTimeout runs `sleep 30` with timeout_sec=1 and asserts exit code 124. Pins the background-thread-drain fix — the earlier synchronous loop would have hung indefinitely. 17/17 testbot bazel targets pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: stop over-crediting package siblings on named-symbol imports Code-review #2: when an importer says `from src.lib.data.storage import StorageBackend`, the previous fan-in resolver was adding +1 to *every* file that storage/__init__.py re-exports from — even though the importer only ever references backends/common.py. The package only matters as a value when the importer actually binds it (via `import X` or `from X.parent import X`), not when X is the source of named symbols. Refactor `_python_imports` to return `(direct, package_use)`: - `direct` — every dotted name that *might* resolve to a single file. Source `X` from `from X import Y` plus the composed `X.Y`, plus the whole-name target of `import X.Y.Z`. - `package_use` — names the importer binds *as a value*. Populated by `import X.Y.Z` (top-level package binding) and the composed `X.Y` from `from X import Y` (Y could be a sub-package the importer accesses as `Y.something`). Crucially **not** the source `X` of `from X import Y, Z` — in that form X is never referenced as a value. `build_python_fan_in` now consumes both: direct hits credit single files; package re-exports expand only for `package_use` entries that didn't already resolve to a file. Test added: `test_named_import_does_not_overcredit_package_siblings` asserts client.py stays at 0 when only StorageBackend was imported. Top-10 dry-run on main is unchanged (the over-credit was distributed evenly across packages so removing it shifted everyone proportionally). The fix matters more as __init__.py re-exports grow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…max name Two latent bugs that surfaced during a real swedencentral deploy: 1. tfvars heredoc hardcoded `availability_zones = ["1", "2"]`. Works in eastus2 (Standard_NC40ads_H100_v5 zones are 1,2,3) but breaks in swedencentral (zones are 1,3) — TF apply hard-failed with AvailabilityZoneNotSupported on the GPU pool. Both AKS pools share `var.availability_zones` in TF so the GPU SKU's zone set must intersect with the node SKU's. 2. tfvars heredoc wrote `gpu_min` / `gpu_max`, but variables.tf declares them as `gpu_node_pool_min_size` / `gpu_node_pool_max_size`. TF ignored the unknown vars (warning only), the GPU pool fell back to its hardcoded TF defaults (1/3), and the preflight's `gpu_max × vCPUs/node` quota math was applied to a number TF never read. Fixes: - New `_azure_sku_zones_json` + `_azure_resolve_pool_zones` helpers query `az vm list-skus` and compute zone intersection. - `azure_generate_tfvars` now writes the intersection (with `TF_AVAILABILITY_ZONES` env override for hand-picking). - Heredoc now uses the correct `gpu_node_pool_{min,max}_size` names. - `azure_preflight_sku_quota` hard-fails on empty intersection with the two SKUs' actual zones in the error so the operator can pick a different region. Companion SKILL.md edit (skills/osmo-deploy/SKILL.md): - Promoted "Required user inputs" to section #2 ("🛑 STOP — gather inputs before invoking the script") so an agent reading top-down hits it before any invocation example. - Restructured Provider → GPU prompts → per-provider prompts. - Fixed broken numbering where Azure-only restarted at 4 instead of 5. - Reconciled flag/env wording (`--no-gpu` exists, `--gpu-count` does not — explicitly noted) and added anchor to Common invocations. Verified locally against eastus2 and swedencentral: swedencentral / D2s_v3 + H100 → ["1", "3"] eastus2 / D2s_v3 + H100 → ["1", "2", "3"] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Add OETF (OSMO E2E Test Framework) to public repo Migrate the OETF framework + KIND adapter + OSS-safe scenarios from the internal monorepo to NVIDIA/OSMO. The internal repo retains internal-only artifacts (DevAdapter, dev_argocd, KEEP-tier scenarios) via a Bazel external-module overlay; this PR is the canonical home for everything else. What's included: - test/oetf/ : framework + KIND adapter + noop adapter - test/oetf/staging/ : api/cli/websocket smoke + MOVE/SPLIT scenarios (auth-checks stays internal — chart ships no JWT issuer yet) - test/oetf/tests/ : framework unit tests (16 files, excluding test_dev_adapter which stays internal) - validation/workflow/ : 35 yaml fixtures referenced by MOVE/SPLIT scenarios (9 KEEP yamls stay internal) - .github/workflows/oetf-kind.yaml : new GitHub Actions workflow that runs `bazel run //test/oetf:deploy_and_run -- --env kind --tags kind` on ubuntu-latest with kind v0.24.0 / kubectl v1.31.0 / helm v3.16.2 (pinned + sha256-verified). Notable refactors in the framework (already pushed to the internal repo as the Step 1 pre-flight MR): - deploy_adapters/factory.py: plugin registry with try/except optional imports for dev_argocd + DevAdapter; overlay packages register via factory.register_adapter("dev", ...). - teardown_main.py: uses factory.build_teardown_adapter (no direct DevAdapter import). - main.py: --target-pattern CLI flag lets a single invocation combine public framework paths with internal overlay paths. - environments.py: auto-discovers data/oetf.internal.yaml sibling. - bzl/oetf.bzl: workflow_data kwarg in oetf_scenario_test so overlay scenarios can override the filegroup label. - runner_fixture.py: default_image / default_platform / default_bucket properties reading OETF_DEFAULT_* env vars. Verification: - Internal repo: bazel test //test_infra/oetf/tests/... = 34/34 PASSED. - Public repo (this PR): GitHub Actions oetf-kind workflow runs the M1 milestone (`bazel run //test/oetf:deploy_and_run -- --env kind --tags kind`). Pre-merge guard: `git grep -iE 'jiaenr|pdx\.s8k\.io|nvstaging|staging\.osmo\.nvidia\.com|isaac-nightly|urm\.nvidia\.com|gitlab-master|nvcr\.io/nvstaging|dockerhub\.nvidia\.com|svc-isaac|AUTH_team-osmo|isaac-hil|swiftstack-maglev' -- test/ validation/workflow/` must return zero hits. Migration plan: internal repo's projects/osmo-oetf-migration-plan.md (Strategy 4 Single Clean Home). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * oetf-kind: sudo the bazelisk install curl -fsSLo /usr/local/bin/bazel failed with "Failure writing output to destination" (exit 23) on ubuntu-latest — the runner ships a pre-existing /usr/local/bin/bazel symlink owned by root, and the unprivileged curl can't overwrite it. Prefix sudo so the kind/kubectl/helm pattern is consistent. PR #1062 first run: https://github.com/NVIDIA/OSMO/actions/runs/26871190048 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * public: rewrite Python imports test_infra.oetf → test.oetf The initial migration commit rewrote Bazel labels (//test_infra/oetf → //test/oetf) but missed Python import statements, causing: ModuleNotFoundError: No module named 'test_infra' at runtime when deploy_and_run_main.py tried `from test_infra.oetf import breadcrumb`. Sed pass across test/ and validation/workflow/ to rewrite all `from test_infra.oetf` → `from test.oetf` (and the dotted-import + string-literal forms). PR #1062 first deploy_and_run failure: https://github.com/NVIDIA/OSMO/actions/runs/26871270505/job/79246935674 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * oetf-kind: add quick-start.osmo to /etc/hosts + catch ImportError in factory Two fixes for the M1 milestone CI run: 1. KIND adapter's check_kind_prereqs requires quick-start.osmo to resolve so tests can reach the ingress. ubuntu-latest /etc/hosts doesn't have it; add it before the preflight step. 2. factory.py's optional-import was `except ModuleNotFoundError` — too narrow. When `from test.oetf import dev_argocd` runs and the public package is loaded (PEP 420 namespace) but dev_argocd.py is absent, the missing-attribute case raises plain ImportError, not ModuleNotFoundError. Broaden to `except ImportError` so both cases are silent-and-expected. PR #1062 third run failure logs: https://github.com/NVIDIA/OSMO/actions/runs/26871363921/job/79247357681 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * oetf: rewrite hardcoded test_infra/oetf paths in moved code kind_adapter._KIND_CONFIG_BUNDLED_RELPATH hardcoded the kind config path as test_infra/oetf/data/kind-osmo-cluster-config.yaml — that broke at runtime in the public copy where the file is at test/oetf/data/. Same for main._short_label's str-replace. Comment-only references in environments.py, deploy_main.py, deploy_and_run_main.py, README.md still mention test_infra/oetf — left alone for now since they're docs (will sweep in a follow-up). PR #1062 fourth run failure: ERROR: failed to create cluster: error reading file: ... test_infra/oetf/data/kind-osmo-cluster-config.yaml: no such file or directory https://github.com/NVIDIA/OSMO/actions/runs/26871556751/job/79247882853 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * oetf: sweep remaining test_infra/oetf references in moved code Catches the runtime path in main.py (Path(workspace) / "test_infra" / ...), test_aggregate's fixture path, and the docstring comments in runner_fixture.py + smoke_fixture.py. With this commit, the public copy has zero `test_infra` strings under test/. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * validation/workflow: allow_empty=True for absent scripts/ + input/ in public KIND cluster create + helm install both succeeded on PR #1062 sixth run (6 min elapsed in deploy phase), but bazel test analysis failed at //validation/workflow:all_workflow_yamls because the filegroup glob's 'scripts/**' and 'input/**' patterns matched nothing (allow_empty defaults to False). scripts/ contains shell scripts + python_library_data_test.py — KEEP-only, not shipped publicly. input/ contains uploaded dataset trees for folder_input — not in the kind tag set either. Set allow_empty=True so the filegroup loads cleanly with just the *.yaml subset. PR #1062 sixth-run failure log: https://github.com/NVIDIA/OSMO/actions/runs/26871737190/job/79248531629 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * oetf-kind: build osmo CLI + symlink to /usr/local/bin/osmo 9/11 PR #1062 kind-tagged tests passed on commit b7bea35 — KIND deploy + helm install + scenario execution all work end-to-end. The 2 remaining failures (router-connectivity, smoke:cli-checks) are both: FileNotFoundError: [Errno 2] No such file or directory: '/usr/local/bin/osmo' i.e. the CLI-mode scenarios need the `osmo` binary present. Build //src/cli:cli and symlink the py_binary to /usr/local/bin/osmo (the default --local-osmo path) before invoking deploy_and_run. PR #1062 seventh-run log: https://github.com/NVIDIA/OSMO/actions/runs/<TBD>/job/<TBD> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * oetf-kind: drop kind tag from cli-checks (requires JWT auth flow) cli-checks invokes `osmo login` which requires the JWT auth flow. The public quick-start chart with auth.disabled=true (the kind env default) doesn't ship a JWT issuer, so the CLI login fails before any tested subcommand can run. Re-add the `kind` tag when one of: - the chart ships a JWT issuer for the kind/dev path - the OETF kind env is wired to pass --auth-method=dev through to the CLI Note: task-runtime-environment 504'd on this same run but PASSED on b7bea35 (104s) — likely transient ingress latency from added CLI build load. Leaving its `kind` tag. PR #1062 commit 2442572 results: - 9 PASS, 2 FAIL (cli-checks=login-fail, task-runtime-env=504) - router-connectivity ✓ (was the other CLI-needing test from b7bea35 run) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test/oetf: expose deploy_main_lib + deploy_and_run_main_lib Add py_library wrappers around deploy_main.py and deploy_and_run_main.py so overlay packages (downstream consumers of @osmo_workspace//test/oetf:*) can `from test.oetf.deploy_main import main` and `from test.oetf.deploy_and_run_main import main`. Mirrors the existing `:run_lib` and `:teardown_main` lib targets. teardown_main was already a py_library (`:teardown_main`); only `deploy` and `deploy_and_run` lacked lib variants. Their binaries now depend on the new libs. Required by the internal cutover step that creates `//test/oetf:deploy`, `:deploy_and_run`, `:teardown` overlay py_binaries which import + delegate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test/oetf/tests: restore swift:// scheme in test_sinks normalize-endpoint test Step 2's sed redaction (`pdx.s8k.io` → `s3.example.com`) accidentally rewrote the `swift://pdx.s8k.io/...` input in test_swift_url_with_path_ becomes_https_host into `s3://...`, defeating the test's purpose (exercising the normalizer's `swift://` → `https://` scheme rewrite). Restore a swift:// input + non-internal tenant path so the test asserts real normalization behavior again. Found via internal repo's `bazel test @osmo_workspace//test/oetf/tests/...` during Step 3 cutover verification. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * environments: honor OETF_INTERNAL_YAML env var for overlay yaml discovery Overlay binaries shipped in a separate Bazel module (i.e. consuming this OETF as @osmo_workspace) live in a runfiles tree disjoint from the public framework's. The canonical sibling-discovery in default_environment_paths can only see ``data/oetf.internal.yaml`` adjacent to the public default's ``data/oetf.default.yaml`` — never the overlay's separate copy. Add an OETF_INTERNAL_YAML env var override so overlay shims can point at their own yaml at runfiles-tree-relative paths. Sibling-discovery remains the fallback for in-repo overlays. Found via internal-repo Step 3 cutover: --env staging via //test/oetf:run was reading the public's placeholder staging env (token_env: OETF_TOKEN) instead of the overlay's real entry (token_env: OSMO_STAGING_TOKEN). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test/workflow: rename from validation/workflow (consolidate under test/) Move the OSS-safe workflow YAML fixtures + the all_workflow_yamls filegroup from //validation/workflow/ to //test/workflow/ so that the entire OETF surface lives under test/. The runner_fixture spec-path resolution already handles `test/` as a workspace-top-level prefix (added in Step 1 alongside the rename from `test_infra/`). Rewrites: - git mv validation/workflow → test/workflow (preserves history) - All `self.workflow("validation/workflow/...")` strings → "test/workflow/..." - All `//validation/workflow:...` Bazel labels → `//test/workflow:...` - bzl/oetf.bzl `workflow_data` default - README.md doc references - .github/workflows/oetf-kind.yaml `paths:` filter - runner_fixture.py spec-path workspace-prefix list Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test/oetf + test/workflow: address CodeRabbit findings Migration-introduced fixes: - Copyright headers: NVIDIA CORPORATION. → NVIDIA CORPORATION & AFFILIATES. across all moved .py/.bzl/BUILD/.md/.yaml files. AGENTS.md requires the "& AFFILIATES" form; the internal repo had drifted from this and the migration carried the drift forward. Addresses CodeRabbit comments #2 (aggregate.py:3) and #11 (mount_validation.py:3) and ~40 similar callouts across the moved tree. - router_connectivity/test_runner.py:64: assert WorkflowServerStatus.FAILED_CANCELED specifically (not the `failed` union which also accepts plain FAILED). Cancellation test should prove cancellation happened, not just generic failure. Addresses CodeRabbit comment #12. Pre-existing concerns flagged by CodeRabbit (kind_adapter authz-sidecar override, helm-list JSON parse, aggregate.py HTML XSS, auth.py partial data-cred skip, local_images web-ui selector, restart.yaml BusyBox [[ ]], deploy_and_run_main.py teardown-for-non-kind) are real bugs that pre-exist in the internal repo and not introduced by this migration. Filing as follow-up issues; not blocking M1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: re-trigger oetf-kind to confirm router-connectivity 504 was transient 5e188b5 hit a 504 on scenarios:router-connectivity (different test from 2442572's task-runtime-environment 504 and cli-checks login). Each run shows 1 different test 504'ing — pattern is environmental KIND ingress flakiness under load, not test bugs. Empty commit to re-run CI and confirm. * oetf-kind: serialize tests with --jobs=1 to stop ingress 504 flakes Latest 3 oetf-kind runs hit a 504 from the KIND-deployed osmo workflow service, with a different test failing each time: - 2442572: task-runtime-environment 504 (submit phase) - 5e188b5: router-connectivity 504 (submit) - 8fa00f4: router-connectivity 504 again (submit, line 37) Pattern is the parallel test runner (default bazel local_test_jobs ~= ncpu) saturating the single-replica ingress-nginx the public quick-start chart deploys with default resources. The internal Jenkins uses --jobs=5 against a production-shape staging cluster which can absorb that load; the public KIND has the umbrella chart's defaults, which cannot. Pin --jobs=1 in the CI workflow only. Internal Jenkins keeps its setting. Trade-off: longer wall clock (~2-3x the previous ~8 min test phase), but the M1 milestone becomes deterministically green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test/oetf: simplify migration code (post-review cleanups) Applied findings from /simplify review pass: - environments.py default_environment_paths: collapse if/elif into a single `overlay = env or default` resolve + isfile check. ~3 LOC, clearer intent. - cli_args.parse_target_patterns: replace explicit `seen` set + accumulator loop with `dict.fromkeys()` order-preserving dedupe. ~12 LOC saved. - .github/workflows/oetf-kind.yaml: replace bespoke bazelisk curl + sha256 + actions/cache@v4 with bazel-contrib/setup-bazel@4fd964a (SHA-pinned, matches pr-checks.yaml convention). Bazelisk cache + disk cache + repository cache for free. ~15 LOC saved. Also SHA-pin actions/checkout. - BUILD: drop "(or dev env via overlay)" from the :deploy comment — public framework shouldn't enumerate downstream overlay shapes. Replaced with a generic note about factory.register_adapter. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * oetf-kind.yaml: rename workflow to "Local KIND Deployment" The workflow does more than smoke checks — it performs a full deploy_and_run cycle (kind cluster create + osmo/quick-start helm install + end-to-end scenarios + teardown). The "smoke" framing under-sells what's actually verified. "Local KIND Deployment" matches the scope. Filename stays oetf-kind.yaml to keep the existing path-filter and external references stable. * oetf migration: split out Local KIND Deployment CI to a separate PR The deploy_and_run-on-KIND GitHub Actions workflow lives in a follow-up PR so reviewers can evaluate the framework migration and the new CI gate on their own merits. The workflow has no value until the framework targets exist in main, so the follow-up PR opens as draft and goes ready once this one merges. Follow-up: <link added once the second PR is opened> * factory: silence mypy on the optional-import block mypy correctly observes that `test.oetf.dev_argocd` and `test.oetf.deploy_adapters.dev_adapter` don't exist in this tree — the optional-import block exists precisely so they can be absent. The runtime try/except handles both ModuleNotFoundError and ImportError; mypy doesn't need to grok that. Type-ignore the two import lines. The downstream `dev_argocd.X` and `DevAdapter(...)` references in the dev-only builders are gated by `if DevAdapter is None: raise` narrowing, which mypy already handles. CI failure on PR #1062 (post-split): https://github.com/NVIDIA/OSMO/actions/runs/26917568585/job/79410722973 * factory: pylint-disable invalid-name on DevAdapter = None fallbacks `DevAdapter = None` at module level reads as a constant assignment to pylint, which then enforces UPPER_CASE constant naming (C0103). The intent is "DevAdapter is a class symbol that's None when the optional import failed" — class-style naming is correct. Disable invalid-name on the two fallback assignments. CI failure: https://github.com/NVIDIA/OSMO/actions/runs/26920021918/job/79417874019 * test: reorganize test/oetf/staging/{smoke,scenarios}/ → test/{smoke,scenarios}/ Separates test definitions from the framework src tree. After this move: test/oetf/ — framework source code only test/smoke/ — smoke test definitions test/scenarios/ — scenario test definitions test/workflow/ — workflow YAML fixtures The "staging/" prefix was historical (the tests originally only ran against the staging cluster); now they run against any env, so the directory was misleading. Mechanical: `git mv` preserves history, plus rewrites of: - `//test/oetf/staging/{smoke,scenarios}` Bazel labels - "test/oetf/staging/..." spec-path strings in scenarios - main.py TARGET_PATTERN, `_short_label`, `_resolve_name_target` (now searches both test/smoke and test/scenarios roots) - README + CLAUDE.md doc paths * test_aggregate: update fixture path to test/smoke/ post-reorg The test fixture built `tmp/test/oetf/staging/smoke/auth-checks/...` via os.path.join with separated components — the sed pass for the test/oetf/ staging/ → test/ rename missed it. collect_allure_results derives its search path from the bazel label //test/smoke:auth-checks, looked for tmp/test/smoke/auth-checks/..., didn't find anything, returned 0 — and the test asserted 2. Update the fixture to build tmp/test/smoke/auth-checks/... * oetf(deploy): /simplify pass on deploy surface Three cleanups, net -121 LOC, no behavior change. All 32 OETF unit tests + pylint still pass. A1 — Extract prepare_deploy() helper (test/oetf/deploy_pipeline.py) deploy_main.py and deploy_and_run_main.py were running ~70 lines of identical pre-DeploySession boilerplate: signal shim + arg parse, env resolve, check_deployable/check_auth, kind preflight, DeployParams build, build_adapter, pre_deploy_check. Same error paths, same ERROR/ NEXT logging contract. Moved that whole block into a shared helper that returns (env, params, adapter) on success or None on any failure (with errors already logged). Both main()s shrink to ~60 lines. A2 — Extract KindAdapter._helm_json() helper Two callsites parsed JSON-emitting helm output the same way (_existing_release_global_values + _ensure_helm_repo): subprocess capture with allow_failure → tolerate empty output → json.loads under try/except. Lifted into one helper so the tolerance behavior is described once. A3 — Delete _redact_cmd + _REDACTED_FLAGS The redaction tuple targets --container-registry-password=, --auth-token=, --token=. None of these flags are ever constructed by any helm/kubectl/ kind/docker invocation in this file. The dev adapter (which does pass auth tokens) lives in a separate module. Inlining " ".join(args) for the two log lines that called _redact_cmd. Verified: bazel test //test/oetf/tests/... 32/32 pass bazel build //test/oetf:deploy //test/oetf:deploy_and_run //test/oetf:teardown * kind_adapter: resolve KIND config from __file__, not $BUILD_WORKSPACE_DIRECTORY The prior path resolution joined $BUILD_WORKSPACE_DIRECTORY with "test/oetf/data/kind-osmo-cluster-config.yaml", which works when this repo is the workspace root. It breaks when called through the internal-repo overlay (where the public OETF is mounted as a submodule at external/): the env var points at the internal-repo root, but the kind config lives under external/test/oetf/data/, so kind reports "no such file or directory" and the deploy fails before cluster creation. The config is declared as a `data=` dep of `deploy_adapters_kind`, so __file__-relative resolution reaches it in runfiles regardless of caller — external standalone, internal overlay, or sandboxed unit tests. Drop the $BUILD_WORKSPACE_DIRECTORY branch + its `_KIND_CONFIG_BUNDLED_RELPATH` constant. Verified: bazel test //test/oetf/tests:test_deploy PASSED 50.6s * oetf(bzl): qualify macro deps with @osmo_workspace// for overlay use The `oetf_smoke_test` and `oetf_scenario_test` macros referenced deps as bare `//test/oetf:*` and `//src/lib/utils:*`. That works inside this repo (where those paths resolve to the public framework) but breaks when the macros are loaded from the internal overlay in a downstream repo: there, the bare `//src/lib/utils:client` label resolves to the *internal* repo's `src/lib/utils`, which doesn't exist (the public sources live under `external/`). Concrete failure (internal repo, before this commit): bazel run //test/oetf:run -- --env jiaenr-dev --tags smoke ERROR: no such package 'src/lib/utils': BUILD file not found... referenced by '//test/smoke:auth-checks' Fix: qualify every cross-package label inside the macros with `@osmo_workspace//`. Bazel treats the qualifier as "this repo" when the load is from inside @osmo_workspace, so external-repo callers see no behavior change. Also qualify the `workflow_data` default (`@osmo_workspace//test/workflow:all_workflow_yamls`) — same reason. Overlay packages still override via the kwarg per the docstring. Verified: bazel test //test/oetf/tests:test_deploy PASSED (cached, 50.6s) bazel build //test/smoke:{api,cli}-checks clean * oetf: refresh README — Layout, paths, kind-tag count, start_service_kind link Layout section had drifted from reality across several rounds of refactoring: - Tests live at `test/{smoke,scenarios,workflow}/`, not under a `test/oetf/staging/` subtree. Rewrote the tree to reflect the current layout, separating framework code from the test suites and the workflow YAML fixtures. - Framework files added since the original README — `aggregate.py`, `sinks.py`, `reporter.py`, `fixture_base.py`, and the recently- added `deploy_pipeline.py` (shared pre-DeploySession helper) — weren't listed. - `tests/` listing only named three files; the directory actually has 16 unit-test files. Listed all of them. - `data/` was missing `categories.json` (Allure failure-bucket rules). - Note about `bzl/oetf.bzl` macros qualifying cross-package deps with `@osmo_workspace//` so downstream overlay callers work. Also fixed staling-around-the-Layout-section breadcrumbs: - ~20 `staging/{smoke,scenarios}/` path references in the writing- tests sections updated to `test/{smoke,scenarios}/` to match the current layout (matching the BUILD targets they document). - `--name` flag docs: the wrapper now scans `test/smoke/` and `test/scenarios/`, not `staging/`. - "currently 11 tests" with kind tag → 10 (cli-checks dropped its `kind` tag when the chart's auth mode changed; the README's breakdown was already "3 smoke probes" but only 2 smoke targets remain kind-tagged). Verified via `bazel query 'attr(tags, "\bkind\b", tests(//test/...))'` → 10 unique test targets. - `external/run/start_service_kind.py` reference + relative link was broken from the README's own location — the path was `../../external/run/...` which resolves outside the tree. Fixed to `../../run/start_service_kind.py` (correct from `external/test/oetf/README.md` in the standalone repo). * oetf: drop "OSS"-flavored wording from README + adjacent comments Audit pass — the user-facing README and the framework comments referred to "OSS-safe smoke tests", "OSS-distributed copy", "OSS KIND deploys", "older OSS script", "(OSS / non-NVIDIA)", and "safe OSS defaults". These labels call attention to a public/internal split that should stay implicit in this repo — readers landing on the public repo shouldn't have to think about what's hiding elsewhere. Five files touched, comments only — no behavior changes. Verified bazel test //test/oetf/tests:test_runner_fixture clean. - README.md: Layout comments + two section headers reworded. - runner_fixture.py: "OSS KIND deploys" → "KIND deploys". - factory.py: comment on the optional dev_adapter import block now describes the mechanic ("present only when a downstream overlay adds them to the runfiles tree") instead of labeling distributions. - tests/test_runner_fixture.py: docstring trimmed similarly. * oetf(README): drop "(NVIDIA-internal)" / "(non-NVIDIA)" callouts Same audit lens as the prior commit (drop "OSS"-flavored wording) — the two Reporting Quick-start subheadings called out NVIDIA-internal vs non-NVIDIA, and the Layout intro mentioned NVIDIA-internal overlay packages. Reworded around the technical distinction users actually care about (whether credentials are already in env vars vs. passed explicitly) so the public README reads as one tree, not two halves. --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

fernandol-nvidia marked this pull request as ready for review November 7, 2025 15:35

fernandol-nvidia requested a review from a team as a code owner November 7, 2025 15:35

fernandol-nvidia marked this pull request as draft November 7, 2025 15:37

fernandol-nvidia force-pushed the fernandol/submodule branch from 2df0b9f to 50206c8 Compare November 11, 2025 05:20

fernandol-nvidia added 3 commits November 12, 2025 20:04

Update gitlab submodule

4e6f8ba

Test

7072ca7

Rework GitHub CI to use hosted runners + bazel cache

3d2a169

fernandol-nvidia force-pushed the fernandol/submodule branch from 50206c8 to 3d2a169 Compare November 13, 2025 04:09

fernandol-nvidia added 6 commits November 12, 2025 20:16

Use eligible bazel-contrib/setup-bazel

167a4a8

Exclude //ui/... properly

15f0b53

Proper dind dns resolution

9ddb763

Simplify ci.yaml

1dd01fb

Internal vs External CI

4d9a2ed

Test change

8152a17

fernandol-nvidia had a problem deploying to internal-ci November 13, 2025 15:26 — with GitHub Actions Failure

git-lfs

2a719d8

fernandol-nvidia temporarily deployed to internal-ci November 13, 2025 15:28 — with GitHub Actions Inactive

fernandol-nvidia added 2 commits November 13, 2025 07:38

Remove cleanup

1c40d0b

Consolidate

c8852de

fernandol-nvidia temporarily deployed to internal-ci November 13, 2025 15:47 — with GitHub Actions Inactive

fernandol-nvidia closed this Nov 14, 2025

fernandol-nvidia deleted the fernandol/submodule branch November 19, 2025 02:11

ethany-nv mentioned this pull request Mar 24, 2026

Optimize UpdateGroup DB performance: batch updates, lightweight aggregation, skip unnecessary task loading #742

Merged

3 tasks done

vvnpn-nv mentioned this pull request May 20, 2026

BYO/Azure deploy: bash3 array, dead NGC block + MEK guard, GPU label, smoke-test failure exit code, single-pool auto-pick #1030

Merged

10 tasks done

svc-osmo-ci mentioned this pull request Jun 3, 2026

[testbot] Add tests for backend_jobs.py #1063

Closed

3 tasks done

jiaenren mentioned this pull request Jun 3, 2026

Add OETF (OSMO E2E Test Framework) #1062

Merged

3 tasks done

Navigation Menu

Repository navigation

Update gitlab submodule - #2

Update gitlab submodule#2
fernandol-nvidia wants to merge 12 commits into
mainfrom
fernandol/submodule

fernandol-nvidia commented Nov 7, 2025 •

edited

Loading

Uh oh!

fernandol-nvidia commented Nov 14, 2025

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

fernandol-nvidia commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

fernandol-nvidia commented Nov 14, 2025

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fernandol-nvidia commented Nov 7, 2025 •

edited

Loading