Skip to content
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

Hybrid deployments #563

Merged
merged 1 commit into from
Feb 4, 2025
Merged

Hybrid deployments #563

merged 1 commit into from
Feb 4, 2025

Conversation

radez
Copy link
Collaborator

@radez radez commented Oct 3, 2024

Baremetal control plane and virtual workers.
I think this will also do baremetal control plane and workers and virtual workers.

@radez
Copy link
Collaborator Author

radez commented Oct 3, 2024

Working on testing this. I think it "works" but I wanted some feedback of what I'm missing or not implementing correctly.

@radez radez force-pushed the hybrid branch 2 times, most recently from 1731073 to 2d70df6 Compare October 8, 2024 12:58
@radez radez self-assigned this Oct 8, 2024
@radez
Copy link
Collaborator Author

radez commented Oct 8, 2024

re: issue #536

@radez radez linked an issue Oct 8, 2024 that may be closed by this pull request
@radez radez force-pushed the hybrid branch 2 times, most recently from faee549 to 9e0ea2c Compare October 15, 2024 19:27
@radez radez force-pushed the hybrid branch 2 times, most recently from c656c10 to 22cec71 Compare December 5, 2024 17:57
Comment on lines 27 to 30
- role: boot-iso
vars:
inventory_group: hv_vm
index: "{{ hybrid_worker_count }}"
Copy link
Member

@akrzos akrzos Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the context of someone running an ACM scale test where all of the hv_vm entries are actually say SNOs, does this task dump thousands of lines of skipped tasks or does it just skip the role? If it dumps thousands of lines, I think we should revisit how this is performed perhaps using a different inventory group.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right it does dump thousands of lines. I had looked at adding a loop_var in this at one point to make the output more meaningful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we could instead of including another boot-iso role here over hv_vm workers, maybe we just copy the desired number of hv_vm we want to use under workers instead. I will think of a more automated way to accomplish this as well. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried a deployment where I removed this and just copied and pasted my vm workers to workers section. This deployed successfully and I think is a better approach in the near term. It allows us to remove the hybrid_worker_count var which I think will cause confusion for many consumers of jetlag without including a background on setting up hypervisors and when the var is to be used. WDYT?

@radez radez force-pushed the hybrid branch 2 times, most recently from 530689d to 36d31d4 Compare January 16, 2025 17:26
Copy link
Member

@akrzos akrzos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to start building some clusters where my controlplane nodes were bare metal and my worker nodes were VMs however I have some feedback.

ansible/roles/boot-iso/tasks/libvirt.yml Outdated Show resolved Hide resolved
ansible/roles/boot-iso/tasks/libvirt.yml Outdated Show resolved Hide resolved
ansible/roles/boot-iso/tasks/libvirt.yml Outdated Show resolved Hide resolved
ansible/roles/boot-iso/tasks/libvirt.yml Outdated Show resolved Hide resolved
ansible/roles/create-ai-cluster/tasks/main.yml Outdated Show resolved Hide resolved
ansible/roles/wait-hosts-discovered/tasks/main.yml Outdated Show resolved Hide resolved
Comment on lines 27 to 30
- role: boot-iso
vars:
inventory_group: hv_vm
index: "{{ hybrid_worker_count }}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we could instead of including another boot-iso role here over hv_vm workers, maybe we just copy the desired number of hv_vm we want to use under workers instead. I will think of a more automated way to accomplish this as well. WDYT?

ansible/roles/wait-hosts-discovered/tasks/main.yml Outdated Show resolved Hide resolved
ansible/roles/create-ai-cluster/tasks/main.yml Outdated Show resolved Hide resolved
@radez radez marked this pull request as ready for review January 21, 2025 15:49
@openshift-ci openshift-ci bot requested review from akrzos and rsevilla87 January 21, 2025 15:50
@akrzos
Copy link
Member

akrzos commented Jan 21, 2025

I think this might need to be reopened so it doesn't include the commit that is already merged.

akrzos
akrzos previously requested changes Jan 23, 2025
ansible/roles/boot-iso/tasks/libvirt.yml Outdated Show resolved Hide resolved
@radez radez dismissed akrzos’s stale review January 23, 2025 19:58

resolved the requested change

@radez radez requested a review from akrzos January 23, 2025 19:59
README.md Show resolved Hide resolved
{% set ctr.vm = ctr.vm + 1 %}
{% endfor %}
{% endif %}

{% endfor %}

[hv_vm:vars]
role=worker
Copy link
Member

@akrzos akrzos Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To extend on the thought of workers always under [workers] we could add to the template here that hybrid_worker_count generates the worker entries under [worker]. The trade off here would be "Semi-duplicated" entries per worker - one under hv_vm for vm creation and one entry under worker for that VM to become a worker. With that we can treat the libvirt workers more like traditional workers and eliminate some of the other touch points in this PR.

It would also eliminate the need to duplicate these worker specific entries under [hv_vm:vars]

Copy link
Member

@akrzos akrzos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci openshift-ci bot added the lgtm label Feb 4, 2025
Copy link

openshift-ci bot commented Feb 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrzos, radez

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@akrzos akrzos merged commit 2cd932f into redhat-performance:main Feb 4, 2025
1 check passed
@radez radez deleted the hybrid branch February 5, 2025 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hypervisor support for MNO clusters
4 participants