Add osac.workflows collection with override support and integration tests - #220
Conversation
79dec74 to
73fe5d9
Compare
eranco74
left a comment
eranco74
left a comment
There was a problem hiding this comment.
Review Comments
1. Finalizer override safety concern
In the cluster delete workflow, the override guide lists step_remove_finalizer_override as a phase override. Is removing finalizers really safe to override? If someone overrides this and skips it, the resource will be stuck with a dangling finalizer. Consider making finalizer removal CRITICAL (not overrideable), consistent with how it's handled in the compute_instance workflows.
2. Vendor directory sync with osac-templates
This PR modifies files under vendor/ansible_collections/osac/templates/ (ocp_virt_vm, ocp_4_17_small). These vendored templates are also in osac-templates. Are these changes in sync with osac-templates PR #14? If both PRs land, we need to make sure the vendored copy matches what's in the templates repo.
3. ocp_virt_vm refactoring scope
The create.yaml went from 338 lines of inline logic to 67 lines dispatching to 7 sub-task files. The logic looks preserved, but this is a significant refactor hidden inside a "workflow collection" PR. Would it make sense to split the template refactoring into a separate PR in osac-templates first, then vendor it here? That would make each PR easier to review and bisect if issues arise.
4. Naming inconsistency across layers
Workflow-level overrides use hook_workflow_start, template install steps use install_step_*_override, and VM create steps use create_step_*_override. The prefixes differ across layers — this makes sense conceptually but could be confusing for consumers. The override_guide.md documents it well, but is a more uniform naming scheme feasible?
5. template_parameters: {} in compute_instance/create.yml
This hardcodes an empty dict for template_parameters. The existing compute instance playbook passes template parameters from the CRD. Is this intentional, or should it be populated from the ComputeInstance spec?
6. No error handling on hook failures
If a hook (e.g., hook_workflow_start) fails, the workflow will fail but won't clean up any partial state (e.g., finalizer already added, partial infrastructure created). The existing workflows have the same limitation so this isn't a regression, but worth tracking as follow-up work — especially now that external code (overrides) can inject failures into the workflow.
adriengentil
left a comment
adriengentil
left a comment
There was a problem hiding this comment.
few comments, I mainly reviewed the compute instance part, I think it's going to the right direction overall
There was a problem hiding this comment.
these files are vendored form osac.templates collections here: https://github.com/osac-project/osac-templates. you'll need to open a PR there, and re-vendor the dependencies here in osac-aap repo.
But I think we can consider these templates generic now, and should be part of osac-aap, and collections specific to an infrastructure (e.g.: MOC/ESI), should live in their own repo.
There was a problem hiding this comment.
why not use ansible end-to-end?
There was a problem hiding this comment.
config-as-code is not launched by osac-operator, not sure we need it as part of a workflow as I don't think we want the service provider to customize the "osac" configuration of AAP.
There was a problem hiding this comment.
You're right - undoing the changes
There was a problem hiding this comment.
nit: I know it's ugly, but I find it easier to navigate when files are ordered in the workflow order, so have create_01_ , create_02_...
There was a problem hiding this comment.
Thanks for the suggestion! I prefer semantic names since the orchestration file already defines the execution order. Numbering would require renaming files whenever steps are reordered or inserted. Happy to reconsider if there's strong preference for it though.
There was a problem hiding this comment.
not sure if this stuff should be part of the workflow
There was a problem hiding this comment.
You're right - removed
|
Disclaimer: However, I wanted to try one of the AI review tools - Qodo - which was made available to developers at RedHat. I used this PR as a guinea pig for setting up and running Qodo locally. Below I'm capturing what Qodo provided me. You are obviously much more familiar with the ansible code than I am. So, perhaps these suggestions will make more sense to you. Please feel to completely discard as well . I hope you don't mind me sharing the output here. If you think this was helpful, please do lmk, mainly to understand if Qodo is a good enough review tool in addition to human reviews. Thanks for indulging me 🙏 . SummaryThis PR adds an osac.workflows collection with override support, refactors template roles, and adds integration tests. The override pattern and test approach are sound. Several regressions and test gaps need attention before merge. High-severity Issues
Medium-severity Issues
Low-severity Issues
What this PR does well
Suggested Action Items (priority order)
Appendix — Evidence locations (selected)
Developer guidance: why these are problems and better approaches
Alternative design notes (high-level)
|
| - name: Step - Create cluster infrastructure | ||
| ansible.builtin.include_role: | ||
| name: "{{ (install_step_cluster_infra_override | default(install_step_cluster_infra_default)).name }}" | ||
| tasks_from: "{{ (install_step_cluster_infra_override | default(install_step_cluster_infra_default)).tasks_from }}" | ||
| vars: | ||
| cluster_infra_state: present | ||
| cluster_infra_name: "{{ cluster_order.metadata.name }}" | ||
| cluster_infra_namespace: "{{ cluster_working_namespace }}" | ||
| cluster_infra_node_requests: "{{ cluster_order.spec.nodeRequests | unique(attribute='resourceClass') }}" |
There was a problem hiding this comment.
What if the user's override step needs different vars ?
There was a problem hiding this comment.
The vars block is the interface contract, not a restriction. Override roles receive these vars but can ignore them and use their own logic. They also have access to all workflow-level variables. This documents what's available while allowing flexibility.
|
|
||
| - name: Step - Post-install hook | ||
| ansible.builtin.include_role: | ||
| name: "{{ (install_step_post_install_hook_override | default(install_step_post_install_hook_default)).name }}" | ||
| tasks_from: "{{ (install_step_post_install_hook_override | default(install_step_post_install_hook_default)).tasks_from }}" |
There was a problem hiding this comment.
Do we need post create hook and also post template hook ? they are executed right after the other no ?
There was a problem hiding this comment.
Yes, different layers:
- Template hook (
install_step_post_install_hook) - Runs at end of template execution, for template-specific customization - Workflow hook (
hook_workflow_complete) - Runs at end of entire workflow (after template + finalizers), for workflow-wide concerns like notifications
They serve different purposes: template hook for infrastructure-specific logic, workflow hook for cross-cutting concerns.
danmanor
left a comment
danmanor
left a comment
There was a problem hiding this comment.
@avishayt So if I understand correctly OSAC operator will trigger the workflows from the new collection instead of the global playbooks. Few questions:
- You didn't remove them because of backward compatibility ?
- Lets say a user wants to override a step in the template - you wrote they need to import the role and set the var to the their step instead. How exactly they should be doing it ? where should they define the step ?
Backwards compatibility: Yes, global playbooks remain as thin wrappers calling How to use overrides: Users create their own playbooks that import the workflow collection and pass override vars: # User's playbook (in their own repo)
- name: Create cluster with custom infra
ansible.builtin.import_playbook: osac.workflows.cluster.create
vars:
cluster_order: "{{ ansible_eda.event.payload }}"
install_step_cluster_infra_override:
name: my_company.custom_infra
tasks_from: main.ymlThen point their AAP job template to their playbook. Full examples in |
|
Addressed all @eranco74 review comments:
Addressed all @adriengentil review comments:
Addressed all @akshaynadkarni review comments:
|
Add osac.workflows collection with cluster/create.yml workflow. Enables customization via generic hooks (workflow start/complete), modification hooks (HostedCluster/NodePool YAML), and phase overrides. Update osac.service.hosted_cluster to use build→hook→apply pattern for HostedCluster and NodePool resource creation.
Add cluster/delete.yml workflow with same customization pattern as create workflow. Provides generic hooks (workflow start/complete) and phase overrides for defaults, namespace, template execution, and finalizer removal.
Add cluster/post_install.yml workflow for post-installation cluster configuration. Provides generic hooks and phase overrides. Sets KUBECONFIG environment from admin_kubeconfig variable for cluster access.
Add hostpool/create.yml and hostpool/delete.yml workflows for host pool infrastructure management. Provides only generic hooks (workflow_start, workflow_complete) as extension points. All other steps are critical.
Add reporting/cluster_status.yml and reporting/hostpool_status.yml workflows for annotating CRDs with workflow status. Provides only generic hooks as extension points. All status reporting is critical.
Wraps osac.config_as_code.subscription and osac.config_as_code.configure playbooks with generic workflow hooks at boundaries. Extension points: - hook_workflow_start: Custom pre-workflow tasks - hook_workflow_complete: Custom post-workflow tasks
Add maintenance/cleanup_stale_network_resources.yml workflow for cleaning up orphaned network resources. Provides generic hooks as extension points.
Add osac.test_overrides collection containing hook roles for testing workflow override mechanisms. Includes roles for workflow hooks, cluster hooks, VM create/delete hooks, and a test template. All hooks log execution to /tmp/osac_test_overrides.log for verification.
Add complete integration test suite for osac.workflows collection: - Test targets for all workflows (cluster, compute_instance, hostpool, reporting, maintenance) - Each target has baseline and override tests - Test fixtures for ClusterOrder, ComputeInstance, and HostPool CRDs - Test environment setup script (kind cluster, KubeVirt/CDI CRDs) - Test runner script with summary reporting - Documentation for template override patterns Tests use real templates with selective overrides to prevent actual infrastructure provisioning while validating workflow logic.
Add Makefile with targets for running integration tests: - test-setup: Create kind cluster and install CRDs - test-run: Run all integration tests - test-teardown: Delete kind cluster - test: Run complete test cycle
These workflows don't belong in the workflows collection: - config_as_code is for AAP platform configuration, not tenant provisioning - cleanup_stale_network_resources is a maintenance task, not a provisioning workflow The workflows collection should only contain tenant resource provisioning workflows (cluster, compute_instance, hostpool). Config-as-code and maintenance tasks are called separately, not triggered by osac-operator.
Removed diagnostic tasks that run kubectl and print sensitive information: - kubectl cluster-info execution (external dependency, flaky) - K8S_AUTH_KUBECONFIG logging (potential credential leak) - Cluster order debug output before/after defaults These tasks were left over from development and should not run in production.
Added local noop.yml to the template and updated install.yaml and delete.yaml to reference it instead of osac.workflows.workflow_helpers. This removes the circular dependency where a vendored template required the workflows collection from the same repository. Vendored templates should be standalone and not depend on other collections in the same repo. This allows them to be used independently. TODO: Apply this fix upstream in osac-templates repository
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.ansible-lint-ignore (2)
31-33: Track workflows collection metadata completion.The comment indicates changelog and runtime metadata will be added when the workflows collection is published. Since osac.workflows is production code (unlike test_overrides), ensure there's a tracking mechanism (issue, checklist, or milestone) to complete this metadata before the collection is released or consumed by downstream users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ansible-lint-ignore around lines 31 - 33, Add a formal tracking item to ensure the workflows collection's missing metadata (changelog and runtime entries referenced for collections/ansible_collections/osac/workflows/galaxy.yml) is completed before release: create a ticket/issue or milestone titled e.g. "osac.workflows galaxy.yml metadata" and attach a small checklist (add changelog, add runtime metadata, verify schema) and link that ticket from the comment or repo release checklist so maintainers must close it before publishing/consuming the collection.
11-30: Extensive risky-file-permissions suppressions for test logging.While the 19 suppressions are justified for test-only logging code, consider adding explicit
mode:parameters (e.g.,mode: '0644') to thelineinfiletasks for consistency with security best practices, even in test code. This would eliminate the need for these suppressions and maintain uniform standards across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ansible-lint-ignore around lines 11 - 30, The PR adds many "risky-file-permissions" suppressions for test-only lineinfile tasks; instead edit each affected lineinfile task (names include apply_defaults, modify_hosted_cluster, modify_nodepool, delete, install, post_install, modify_vm_spec, post_create_hook, pre_create_hook, resources, secrets, wait_annotate, post_delete_hook, pre_delete_hook, hostpool_infra, maintenance, workflow_complete, workflow_start) to explicitly set a secure file mode (e.g., mode: '0644') on the target file operations so the tasks convey intended permissions and you can remove the ansible-lint suppressions. Ensure the mode field is added under the same task that uses the lineinfile module and follow Ansible YAML formatting for the mode string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.ansible-lint-ignore:
- Around line 31-33: Add a formal tracking item to ensure the workflows
collection's missing metadata (changelog and runtime entries referenced for
collections/ansible_collections/osac/workflows/galaxy.yml) is completed before
release: create a ticket/issue or milestone titled e.g. "osac.workflows
galaxy.yml metadata" and attach a small checklist (add changelog, add runtime
metadata, verify schema) and link that ticket from the comment or repo release
checklist so maintainers must close it before publishing/consuming the
collection.
- Around line 11-30: The PR adds many "risky-file-permissions" suppressions for
test-only lineinfile tasks; instead edit each affected lineinfile task (names
include apply_defaults, modify_hosted_cluster, modify_nodepool, delete, install,
post_install, modify_vm_spec, post_create_hook, pre_create_hook, resources,
secrets, wait_annotate, post_delete_hook, pre_delete_hook, hostpool_infra,
maintenance, workflow_complete, workflow_start) to explicitly set a secure file
mode (e.g., mode: '0644') on the target file operations so the tasks convey
intended permissions and you can remove the ansible-lint suppressions. Ensure
the mode field is added under the same task that uses the lineinfile module and
follow Ansible YAML formatting for the mode string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9634e355-a234-4b46-b293-b93e3e75e978
📒 Files selected for processing (2)
.ansible-lint-ignore.ansible-lint.yml
✅ Files skipped from review due to trivial changes (1)
- .ansible-lint.yml
1. Exclude massopencloud/esi collection (third-party code) 2. Skip parser-error globally (false positive on multi-play test playbooks) 3. Skip fqcn[keyword] (workflows intentionally use collections: for resolution) 4. Downgrade risky-file-permissions to warning (test logging uses lineinfile) 5. Configure kinds mapping for integration test playbooks 6. Add ignore entries for test-only galaxy.yml and workflows galaxy.yml metadata Production code in osac.workflows and osac.service still gets full linting. Test code gets meaningful checks but problematic test-specific patterns downgraded. Note: 62 var-naming violations remain in osac.service roles (pre-existing code). These should be addressed in a separate PR to avoid scope creep.
|
Fixed CodeRabbit comments:
|
|
Fixed additional CodeRabbit comments:
|
6e2c5f9 to
06b5dd2
Compare
|
@avishayt some of these fixes are in the vendor directory, right? do you plan to open a PR in osac-templates, and re-vendor? |
1. Add kubernetes.core dependency to workflows/galaxy.yml 2. Fix kubeconfig fallback to handle empty env vars with default(..., true) - Applied to all 9 workflow playbooks 3. Add POD_NAMESPACE fallbacks in cluster/create.yml and hostpool/create.yml 4. Make workflow_helpers/tasks/main.yml executable by importing noop.yml 5. Remove stale config_as_code references from documentation 6. Reduce logging verbosity in hostpool/create.yml All 18 integration tests pass. Note: Timestamp comparison issue in vendored create_resources.yaml needs to be fixed in upstream osac-templates repository, not here.
Re-vendored osac.templates after merging upstream PR osac-project#21 which fixes timestamp comparison in create_resources.yaml to use to_datetime filter. Upstream fix: osac-project/osac-templates#21
06b5dd2 to
28e9777
Compare
|
Re-vendored osac.templates with timestamp comparison fix:
The timestamp comparison now properly uses Jinja's |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, avishayt The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
Add osac.workflows collection with override support and integration tests
Summary
This PR introduces the osac.workflows collection, which provides reusable workflow playbooks with built-in customization points. This enables deployments like MOC (Mass Open Cloud) to customize infrastructure integration, networking, and templates without duplicating workflow code.
All 11 OSAC workflows are now available as importable playbooks with override support:
Key Changes
Workflow Collection Structure
Override Pattern
All workflows support customization via override variables:
Three levels of customization:
hook_workflow_start,hook_workflow_complete(all workflows)step_hostpool_infra_override)create_step_modify_vm_spec_override)Template Enhancements
Templates now support override patterns:
VM Template (
ocp_virt_vm):build_specis NOT overrideable (usemodify_vm_spechook instead)Cluster Template (
ocp_4_17_small):Hostpool Workflows:
step_hostpool_infra_override)Integration Tests
Complete test suite with 20 passing tests (10 workflows × 2 tests each):
Test structure:
osac.test_overridesprovides hook implementations that log executionTest coverage:
Benefits
For MOC:
For other deployments:
For maintainers:
Testing
All integration tests pass:
Documentation
collections/ansible_collections/osac/workflows/README.md- Collection overview and usagecollections/ansible_collections/osac/workflows/docs/override_guide.md- Complete override referencetests/integration/README.md- Test setup and execution guidetests/integration/TEMPLATE_OVERRIDES.md- Template override patternsMigration Impact
For existing osac-aap users:
For osac-aap-moc:
Follow-up Work
MOC-specific Ansible to separate repository:
Summary by CodeRabbit
New Features
Tests
Documentation
Chores