refactor: make orchestrator and discovery domains fully self-contained - #4880
Conversation
55c4393 to
3991824
Compare
abhishek-sa1
left a comment
abhishek-sa1
left a comment
There was a problem hiding this comment.
Now let me look at the actual file changes:
Let me try to get the file list from the commit directly:
The diff page is too large (GitHub "Uh oh" / lazy-loading). Let me try the API to get the file list:
I now have a clear picture of the PR. Here's my analysis:
PR #4880 Review — Critical Issues
TL;DR
The PR blindly duplicates the entire input_validation framework (all schemas, all validation flows, all utility code for ALL domains) into both src/orchestrator/ and src/discovery/. This is the wrong approach — it creates a 3x maintenance burden with no value.
Issue 1: Full input_validation/ Wholesale Copy (BLOCKER)
Both orchestrator and discovery get a complete copy of the entire validation framework:
orchestrator/library/module_utils/input_validation/
├── common_utils/
│ ├── config.py ← 241 lines — contains ALL domain configs
│ ├── data_fetch.py ← 175 lines
│ ├── data_validation.py ← 229 lines
│ ├── data_verification.py
│ ├── en_us_validation_msg.py
│ ├── logical_validation.py
│ ├── slurm_conf_utils.py ← WHY does discovery need slurm utils?
│ ├── timezone.txt
│ └── validation_utils.py
└── schema/
├── additional_software.json
├── build_stream_config.json
├── credential_rules.json
├── discovery_config.json ← WHY does orchestrator have discovery schema?
├── functional_groups_config.json
├── ...ALL other schemas
The same files are copied identically to discovery/library/module_utils/input_validation/.
What's wrong:
- config.py (241 lines) contains configs for telemetry, security, storage, slurm, k8s, local_repo, provision — none of which are relevant to discovery
slurm_conf_utils.pyis copied to discovery — discovery has zero slurm interaction- All 15+ schemas are copied to both domains — discovery only needs
discovery_config.json, orchestrator only needsorchestrator_config.json+network_spec.json - All validation flows for ALL domains (telemetry, security, storage, etc.) are duplicated
What should be done instead:
Follow the image_build_manager pattern — only copy what the domain actually uses:
| Domain | Actually Needs |
|---|---|
| discovery | discovery_config.json schema, validate_input.py, data_validation.py, config.py (trimmed to discovery tag only) |
| orchestrator | orchestrator_config.json + network_spec.json schemas, validate_input.py, credential rule schema |
Or better: keep validate_input.py in common/ since it's a tag-dispatched engine shared by everyone, and only copy the specific schemas each domain needs.
Issue 2: config.py Contains Cross-Domain Secrets Map
The copied config.py includes get_vault_password() which maps credential files:
vault_passwords = {
"omnia_config_credentials.yml": ".omnia_config_credentials_key",
"image_build_credentials.yml": ".image_build_credentials_key",
}And input_file_inventory which maps ALL tags to ALL files:
input_file_inventory = {
"build_image": [...],
"scheduler": [...],
"provision": [...],
"orchestrator": [...],
"security": [...],
"telemetry": [...],
"local_repo": [...],
"slurm": [...],
"service_k8s": [...],
...
}Discovery doesn't need image_build_credentials.yml vault mapping or scheduler tag definitions. This is a security smell — leaking credential file names across domains.
Issue 3: Orchestrator Design Docs Are Empty
src/orchestrator/ORCHESTRATOR_DESIGN.md — 0 additions (empty)
src/orchestrator/INPUT_CONTRACT.md — 0 additions (empty)
src/orchestrator/OUTPUT_CONTRACT.md — 0 additions (empty)
These are added but completely empty. Either add real content or don't add the files yet.
Issue 4: validate_input.py Module Copied to Both Domains
The central validate_input.py is a tag-dispatched validation engine — it reads input_file_inventory from config.py, iterates over the matching files for the current tag, and runs L1 schema + L2 logic validation.
Copying this to each domain means:
- Any bugfix to validation logic requires updating 3+ copies
- Schema additions for new domains require touching every copy
- The
validate_input.py→ config.py → schema lookup chain is tightly coupled to the central framework
Better approach:
- image_build_manager pattern: Create a domain-specific validation module (like we did with validate_image_build_config.py) that only validates the domain's own files using domain-specific schemas. No need to carry the entire central framework.
Issue 5: validate_credentials.py + fetch_credential_rule.py Copied to Discovery
Discovery copies validate_credentials.py and fetch_credential_rule.py as modules — but these depend on the full credential_rules.json schema and the central framework's credential validation flow. Discovery only needs OME username/password validation.
Issue 6: Discovery callback_plugins Docstring Still References Common Path
callback_plugins = <relative-path-to>/common/callback_pluginsLine 28 of src/discovery/callback_plugins/omnia_default.py still references the old common path in the usage docstring. Minor but shows it was a blind copy.
Recommended Approach
Thin Domain Validation (Preferred — matches image_build_manager)
Each domain creates its own domain-specific validation module that:
- Only loads the schemas it needs (e.g.,
discovery_config.json) - Has domain-specific L2 validation logic
- Does NOT carry the full
input_validation/framework
discovery/library/
├── modules/
│ └── validate_discovery_config.py # Domain-specific, lean
└── module_utils/
└── discovery_validation/
├── schema/
│ └── discovery_config.json # Only schema discovery needs
└── discovery_validation_flow.py # Only L2 rules discovery needs
Summary Table
| Issue | Severity | Fix |
|---|---|---|
Full input_validation/ wholesale copy (3x duplication) |
BLOCKER | Only copy what domain needs, or create domain-specific validation |
| config.py has ALL domain configs (slurm, telemetry, etc.) in discovery | HIGH | Trim to domain-relevant config only |
| Empty orchestrator design docs | MEDIUM | Add content or remove empty files |
slurm_conf_utils.py in discovery |
HIGH | Remove — discovery has no slurm dependency |
| Credential vault map leaked across domains | MEDIUM | Trim get_vault_password() to domain-relevant entries |
| Callback plugin docstring still references common path | LOW | Update docstring |
3991824 to
84b6e15
Compare
Comprehensive PR Review Comments for #4880Based on the detailed gap analysis comparing orchestrator and discovery domains against the image_build_manager reference pattern, here are the consolidated PR review comments: 🔴 CRITICAL Issues (Must Fix Before Merge)1. Credential Scope Violation — Cross-Domain Credential ManagementLocation: src/orchestrator/roles/orchestrator_credentials/vars/main.yml Issue: The orchestrator_credentials role manages credentials for 16 services across ALL domains, violating self-containment principles. # Lines 17-35: Manages 3 credential files across 3 domains
credential_files:
- credential_type: "Omnia" # ← orchestrator's own
- credential_type: "Build Stream" # ❌ NOT orchestrator's responsibility
- credential_type: "Image Build" # ❌ belongs to image_build_manager domain!
# Lines 70-159: Credential definitions for services that don't belong
omnia_credentials:
gitlab: # ❌ build_stream domain
build_stream: # ❌ build_stream domain
prepare_oim: # ❌ prepare_oim domain
local_repo: # ❌ repo_manager domain
image_build_manager: # ❌ image_build_manager domain (has its own collect_build_credentials)
discovery: # ❌ discovery domain (has its own discovery_credentials)
build_aarch_image: # ❌ image_build_manager domainImpact:
Fix Required: # orchestrator_credentials/vars/main.yml — CORRECTED
credential_files:
- credential_type: "Omnia"
file_path: "{{ input_project_dir }}/orchestrator/omnia_config_credentials.yml"
vault_path: "{{ input_project_dir }}/orchestrator/.omnia_config_credentials_key"
# Remove build_stream and image_build entries
omnia_credentials:
provision:
mandatory:
- { password: provision_password }
- { username: bmc_username, password: bmc_password }
slurm:
mandatory:
- { password: slurm_db_password }
openldap:
mandatory:
- { username: openldap_db_username, password: openldap_db_password }
# ONLY orchestrator-specific servicesDelete templates:
2. Wholesale
|
| Missing Section | Status |
|---|---|
| §5 Input Validation Design (HLD) | ❌ MISSING entirely (7 subsections in reference) |
| §6 Credential Management Design (HLD) | |
| §10 Naming Convention | ❌ MISSING (full table in reference) |
Discovery DESIGN.md missing sections:
| Missing Section | Status |
|---|---|
| Validation Design (HLD) | ❌ MISSING |
| Credential Design (HLD) | ❌ MISSING |
| Tag Support | ❌ MISSING |
| Naming Convention | ❌ MISSING |
| Self-Containment Verification | ❌ MISSING (grep commands in reference) |
| Backward Compatibility | ❌ MISSING |
| Cleanup Playbook | ❌ No cleanup playbook exists |
Fix Required: Expand design docs to match image_build_manager depth.
🔵 LOW Priority Issues (Post-Merge OK)
12. Missing samples/ Directory
Both domains lack samples/ directory with example configs (image_build_manager has samples/build_status.yml).
13. Verb-First Role Naming Convention
Roles use noun-first naming (orchestrator_credentials, discovery_credentials) instead of verb-first (collect_orchestrator_credentials, collect_discovery_credentials).
14. Legacy CONTRACTS.md in Discovery
Discovery has both CONTRACTS.md (3147 bytes) AND INPUT_CONTRACT.md + OUTPUT_CONTRACT.md — redundant.
Fix: Delete src/discovery/CONTRACTS.md.
Summary: Blocking Issues for Merge
| # | Issue | Severity | Blocks Merge? |
|---|---|---|---|
| 1 | Credential scope violation | CRITICAL | ✅ YES |
| 2 | Wholesale validation framework copy | BLOCKER | ✅ YES |
| 3 | build_status.yml path mismatch |
HIGH | ✅ YES |
| 4 | Missing roles_path |
HIGH | |
| 5 | Missing tag validation | HIGH | |
| 6 | Dead code cleanup | HIGH | |
| 7 | Execution order mismatch | HIGH | |
| 8 | Credential file conflict | HIGH | ✅ YES (after #1 fixed) |
Recommendation: Fix issues #1, #2, #3, #8 before merge. Issues #4-#7 strongly recommended. Issues #9-#14 can be follow-up PRs.
84b6e15 to
df756b5
Compare
|
Let me review the current state of the orchestrator and discovery domains in the local repo to see what's changed since the previous review. Comprehensive PR Review Comments for #4880 — Updated ReviewWhat's Been Fixed Since Last Review
Significant improvement. The PR addressed most of the critical items. However, the credential scope fix introduced a new critical regression. 🔴 CRITICAL Issues (Must Fix Before Merge)1. credential_status.yml References
|
| # | Issue | Severity | Fixed from Prev? | Blocks Merge? |
|---|---|---|---|---|
| 1 | credential_status.yml index crash | CRITICAL | 🆕 NEW regression | ✅ YES |
| 2 | update_credentials_main.yml index crash | CRITICAL | 🆕 NEW regression | ✅ YES |
| 3 | Template has non-orchestrator creds | HIGH | ✅ YES | |
| 4 | fetch_credentials.yml cross-domain list | HIGH | ||
| 5 | S3 path default inconsistency | HIGH | ||
| 6 | skip_credential_tags not wired |
HIGH | 🆕 NEW | |
| 7 | Redundant CONTRACTS.md | MEDIUM | — | No |
| 8-10 | Design doc, build_stream, typo | MEDIUM | — | No |
Bottom line: Issues #1 and #2 are regression bugs introduced by the credential scope fix. The vars/main.yml was correctly scoped to 1 credential file, but the task files that consume credential_files[] were not updated. These will cause immediate runtime crashes. Fix #1, #2, and #3 before merge. #4-#6 are strongly recommended.
6f87ee5 to
557fcb8
Compare
- Orchestrator domain:
- Create roles: orchestrator_setup, orchestrator_functional_groups,
validate_orchestrator_input, orchestrator_credentials, orchestrator_common
- Localize library/modules (8 Python modules), module_utils, callback_plugins
- Create sub-playbooks: prepare, validate, credentials, cleanup, upgrade, rollback
- Update orchestrator.yml to use local roles (zero ../playbooks/ imports)
- Update ansible.cfg to use local paths (zero ../common/ references)
- git mv tasks/ files into orchestrator_common role, remove clone_dependencies
- Add ORCHESTRATOR_DESIGN.md, INPUT_CONTRACT.md, OUTPUT_CONTRACT.md
- Discovery domain:
- Create roles: discovery_setup, validate_discovery_input,
discovery_credentials, discovery_common
- Localize library/modules (8 Python modules), module_utils, callback_plugins
- Create sub-playbooks: validate, credentials
- Update discovery.yml to use local roles (zero ../playbooks/ imports)
- Update ansible.cfg to use local paths (zero ../common/ references)
- Add DISCOVERY_DESIGN.md, INPUT_CONTRACT.md, OUTPUT_CONTRACT.md
Both domains now follow the same self-containment pattern as
image_build_manager: zero external ../common/ or ../playbooks/ references.
Relates-to: dell#4849
Signed-off-by: Sujit Jadhav <sujit.jadhav@dell.com>
557fcb8 to
2512027
Compare
130182d
into
dell:issue-4849-omnia-modernization
2 participants
Fixes #4849
Summary
Make both the orchestrator and discovery domains fully self-contained, eliminating all external references to
../common/and../playbooks/. Both now follow the same pattern asimage_build_manager.Orchestrator Domain
New Roles
orchestrator_setup— absorbs upgrade_checkup, include_input_dir, create_container_grouporchestrator_functional_groups— absorbs generate_functional_groups utilityvalidate_orchestrator_input— absorbs validate_config.ymlorchestrator_credentials— absorbs full credential_utility (12 task files + 3 templates)orchestrator_common— shared task library (configure_s3_access, openchami_auth, decrypt_include_encrypt, check_kube_vip_reachability)New Sub-Playbooks
Localized Assets
library/modules/— 8 Python moduleslibrary/module_utils/input_validation/— full validation schemascallback_plugins/omnia_default.pyvars/common_vars.yml,vars/encrypt_files_vars.ymlDocumentation
Discovery Domain
New Roles
discovery_setup— absorbs inline path init, pip install, config load, validation tagsvalidate_discovery_input— absorbs validate_config.ymldiscovery_credentials— absorbs credential_utility (OME credentials only)discovery_common— shared task library (decrypt_include_encrypt)New Sub-Playbooks
Localized Assets
library/modules/— 8 Python modules (ome_server_inventory, generate_pxe_mapping, etc.)library/module_utils/input_validation/— full validation schemascallback_plugins/omnia_default.pyvars/common_vars.yml,vars/encrypt_files_vars.ymlDocumentation
Verification
../common/in ansible.cfg../playbooks/in entrypoint../common/in roles/Relates-to: #4849