-
Notifications
You must be signed in to change notification settings - Fork 144
[adoption_osp_deploy] Add DCN adoption support #3570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[adoption_osp_deploy] Add DCN adoption support #3570
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/164182cfd2c244d8a1cf9d4c183185cc ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 50m 59s |
| # DCN network routes extracted from TripleO scenario | ||
| {% set _dcn1_stack = cifmw_adoption_osp_deploy_scenario.stacks | selectattr('stackname', 'equalto', 'dcn1') | first | default({}) %} | ||
| {% if _dcn1_stack.network_routes is defined %} | ||
| edpm_dcn1_routes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[related to first commit]
(blocking) suggestion: I see here a good candidate for a refactor:
{% for site in ['dcn1', 'dcn2'] %}
{% set _stack = cifmw_adoption_osp_deploy_scenario.stacks | selectattr('stackname', 'equalto', site) | first | default({}) %}
{% if _stack.network_routes is defined %}
edpm_{{ site }}_routes:
{% for subnet_name, routes in _stack.network_routes.items() %}
{{ subnet_name }}:
{% for route in routes %}
- destination: {{ route.ip_netmask }}
nexthop: {{ route.next_hop }}
{% endfor %}
{% endfor %}
{% endif %}
{% endfor %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a reasonable suggestion. I'm testing it in my own here:
https://github.com/fultonj/ci-framework/tree/dcn_adoption_pr_evallesp_refactor
|
Where's dcn_nostorage file? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general (second commit). I've found something that might trigger an error when we starting to have more than one stack.
If not, my comment might be skipped and eventually we can fix this in a following PR.
| addresses: | ||
| - ip_netmask: {{ net[ip_version|default('ip_v4')] }}/{{ net[prefix_length_version|default('prefix_length_v4')] }} | ||
| {% if _stack.network_routes is defined and network_name in _stack.network_routes %} | ||
| routes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(non-blocking) concern: As this is not touched by this PR, I'm marking this comment as non-blocking.
I was checking the output of the routes, and in this PR it seems correct.
But above, at L17, the "routes:" are inside the "{%- for route in _stack.routes %}" which might end up by having two differente routes section like:
routes:
- ip_netmask: first
routes:
- ip_netmask: second
I see here that "routes:" is properly set outside the for loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a reasonable suggestion. I'm testing it in my own here:
https://github.com/fultonj/ci-framework/tree/dcn_adoption_pr_evallesp_refactor
| - osp_trunk | ||
| # Let's remove the default computes, since we want to adopt the | ||
| # OSP ones | ||
| compute: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(non-blocking) question: is this something required to be there to avoid a runtime error? Should we create a task for solving this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required to avoid a conflict. Setting amount of computes to 0 like this:
# Let's remove the default computes, since we want to adopt the
# OSP ones
compute:
amount: 0is necessary because:
- The base scenario would otherwise create new compute VMs
- For adoption, we're reusing the existing OSP compute nodes (osp-compute) instead of creating new ones
- Without
amount: 0, we'd have both the default computes AND the osp-compute nodes, which would cause conflicts
This isn't a bug or workaround that needs a task - it's the correct pattern for adoption scenarios. The libvirt_manager_patch_layout is intentionally overriding the default compute configuration to disable it while defining the OSP-specific compute nodes separately.
The comment in the code explains the intent clearly. No additional task needed.
|
This PR is stale because it has been for over 15 days with no activity. |
|
@olliewalsh would you please update the PR commits so that they look like this? Unfortunately it's a requirement which is causing a CI failure. Here's a quick way to do it. In the editor that opens, change pick to reword (or just r) for these 3 commits: git will then prompt you for each commit and you can paste |
This PR has |
|
@olliewalsh I tested this patch using the refactoring that @evallesp suggested and it still worked. Feel free to |
Adds support for deploying DCN with local storage (which is essentially multi-stack plus spine & leaf networking) to the adoption_osp_deploy role. Signed-off-by: Oliver Walsh <owalsh@redhat.com>
… for DCN deployments
Modify os_net_config_overcloud.yml.j2 template to render per-network
routes from stack configuration instead of hardcoded empty routes.
Problem:
- Template had hardcoded "routes: []" for all VLAN networks (line 40)
- DCN compute nodes need routes to reach central site services
- Routes defined in data-plane-adoption's network_data.yaml.j2 were
being ignored because ci-framework pre-generates os-net-config files
before TripleO Heat deployment runs
Solution:
- Check if _stack.network_routes is defined and contains routes for
the current network
- If routes exist, render them with ip_netmask and next_hop
- Otherwise, fall back to empty routes array
This enables DCN scenarios to configure cross-site routes via the
network_routes field in stack definitions (dcn_nostorage.yaml).
Example usage in stack config:
network_routes:
internalapidcn1:
- ip_netmask: 172.17.0.0/24
next_hop: 172.17.10.1
Related: Requires corresponding data-plane-adoption change to add
network_routes to dcn1/dcn2 stack definitions.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: John Fulton <fulton@redhat.com>
…o into adoption vars Add edpm_dcn1_routes and edpm_dcn2_routes variables to adoption_vars.yaml template. These variables extract network routes defined in the TripleO scenario file (dcn_nostorage.yaml) and make them available for EDPM ansible configuration. Routes are extracted from stack.network_routes for dcn1 and dcn2 stacks, with each route containing destination (ip_netmask) and nexthop fields. This enables proper inter-site connectivity in DCN deployments where compute nodes in edge sites need routes to reach control plane services in the central site. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: John Fulton <fulton@redhat.com>
…s bug Refactor adoption_vars.yaml.j2: Replace duplicate dcn1/dcn2 route extraction blocks with a single loop over site names. This follows DRY principles and scales automatically if more DCN sites are added. Fix os_net_config_overcloud.yml.j2: Move 'routes:' outside the `for` loop to prevent generating duplicate YAML keys when multiple routes exist. This mirrors the correct pattern already used for network_routes at lines 40-48. Signed-off-by: John Fulton <fulton@redhat.com>
65cdb65 to
05ef069
Compare
Adds support for deploying DCN with local storage (which is essentially
multi-stack plus spine & leaf networking) to the adoption_osp_deploy role.