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

Infrastructure-Deployment #43

Draft
wants to merge 111 commits into
base: main
Choose a base branch
from
Draft

Infrastructure-Deployment #43

wants to merge 111 commits into from

Conversation

MaxBed4d
Copy link

The basic changes needed to be able to deploy just the infrastructure of the multinode.

@Alex-Welsh
Copy link
Contributor

Is this ready for review or do you want some more time to work on it?

Copy link

@markgoddard markgoddard left a comment

Choose a reason for hiding this comment

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

The main thing to consider here is ensuring that people can continue to use this manually, without Azimuth. Whether that's using the new cluster_infra role or continuing to run TF manually.

@@ -0,0 +1,11 @@
[defaults]

Choose a reason for hiding this comment

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

I'd be interested to know what in here was actually necessary.

@@ -2,12 +2,12 @@
src_directory: "{{ ansible_env.HOME }}/src"

kayobe_config_repo: https://github.com/stackhpc/stackhpc-kayobe-config.git
kayobe_config_version: stackhpc/yoga
kayobe_config_version: "{{ openstack_version }}"

Choose a reason for hiding this comment

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

We need to consider non-azimuth users here who will not have this variable defined. The openstack_version variable name is also less accurate than kayobe_config_version, so I suggest you switch azimuth to use kayobe_config_version.

kayobe_config_name: kayobe-config
kayobe_config_environment: ci-multinode

kayobe_repo: https://github.com/stackhpc/kayobe.git
kayobe_version: stackhpc/yoga
kayobe_version: "{{ openstack_version }}"

Choose a reason for hiding this comment

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

The kayobe branch won't necessarily be the same as kayobe_config_version.

Copy link
Author

@MaxBed4d MaxBed4d Jan 31, 2024

Choose a reason for hiding this comment

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

This change has been removed and made in the Multinode-Deployment-App and is a more appropriately named variable.

@@ -1,4 +1,4 @@
resource "openstack_compute_keypair_v2" "keypair" {
name = var.multinode_keypair
public_key = file(var.ssh_public_key)
public_key = var.ssh_public_key

Choose a reason for hiding this comment

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

This will impact other multinode users. Could we instead write out the key to a file before getting here?

# Import the playbook to start configuring the multi-node hosts.
- name: Configure hosts and deploy ansible
import_playbook: ansible/configure-hosts.yml
when: openstack_deploy == true

Choose a reason for hiding this comment

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

We should do this unconditionally

Copy link
Author

Choose a reason for hiding this comment

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

Does this matter even when just deploying the VMs?
I am pretty sure that if we were to enable this for just the infrastructure, it would create issues for deploying OpenStack on the system, as an Azimuth patch, at a later date due to the changes made by some of the playbooks called within configure-hosts.yml. This step is partially responsible for the idempotency issue.

required: false
immutable: true

- name: kayobe_version_branch

Choose a reason for hiding this comment

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

I don't see this variable used anywhere.

Suggested change
- name: kayobe_version_branch
- name: kayobe_branch

Choose a reason for hiding this comment

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

There is a related issue here, which is that in deploy-openstack.sh we will install whatever version of kayobe is referenced in SKC's requirements.txt, so this input (and the ansible variable it maps to is not really being used).

It might be better to make a separate change to stop installing kayobe from source, and always install it from SKC requirements.txt.

Copy link
Author

Choose a reason for hiding this comment

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

I don't see this variable used anywhere.

These variables are used in the full multinode deployment version of the App (Multinode-Deployment-App) and those changes are not included in this version due to relating to the deployment of OpenStack on the multinode, rather than just the infrastructure.

There is a related issue here, which is that in deploy-openstack.sh we will install whatever version of kayobe is referenced in SKC's requirements.txt, so this input (and the ansible variable it maps to is not really being used).

If they aren't really being used, can we remove them all together in /ansible/vars/defaults.yml?

- name: kayobe_version_branch
label: Kayobe GitHub Branch.
description: |
Please provide the OpenStack GitHub Branch name you wish to deploy.

Choose a reason for hiding this comment

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

Suggested change
Please provide the OpenStack GitHub Branch name you wish to deploy.
Please provide the Kayobe GitHub Branch name you wish to deploy.

- name: kayobe_config_branch
label: Kayobe configuration GitHub Branch.
description: |
Please provide the GitHub Branch with the OpenStack configuration you wish to deploy.

Choose a reason for hiding this comment

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

Suggested change
Please provide the GitHub Branch with the OpenStack configuration you wish to deploy.
Please provide the StackHPC Kayobe Configuration GitHub Branch you wish to deploy.

Comment on lines +153 to +156
- rocky9-lvm
kind: "string"
required: true
default: "rocky9-lvm"

Choose a reason for hiding this comment

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

Suggested change
- rocky9-lvm
kind: "string"
required: true
default: "rocky9-lvm"
- Rocky9-lvm
kind: "string"
required: true
default: "Rocky9-lvm"

Copy link
Author

@MaxBed4d MaxBed4d Jan 31, 2024

Choose a reason for hiding this comment

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

Needs to be spelled as such to be able to call the image. Isn't the image name case-sensitive?

Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't a lowercase Rocky 9 lvm image in SMS:

$ openstack image list --all -c Name -f value | grep -i rocky9-lvm
Rocky9-lvm
Rocky9-lvm-06092022
Rocky9-lvm-2022-11-08
Rocky9-lvm-20221107
Rocky9-lvm-rc
Rocky9-lvm.2023-03-20

Comment on lines 165 to 168
- cloud_user (rocky9-lvm)
kind: "string"
required: true
default: "cloud_user"

Choose a reason for hiding this comment

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

Suggested change
- cloud_user (rocky9-lvm)
kind: "string"
required: true
default: "cloud_user"
- cloud-user (Rocky9-lvm)
kind: "string"
required: true
default: "cloud-user"

Copy link
Author

Choose a reason for hiding this comment

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

Applied the default change and want to keep the comment about the image consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants