🌱 Develop User-Friendly baremetalhost creation for Tilt interface #1483

peppi-lotta · 2023-12-07T13:39:25Z

Signed-off-by: peppi-lotta [email protected]

Tilt improved for easier baremetalhost creation

What this PR does / why we need it:
Fixes #1478

tuminoid

A lot of nits.

tuminoid · 2023-12-07T14:23:35Z

docs/dev-setup.md

+
+After Tilt is up it is possible to make baremetalhosts by pressing a button in the Tilt localhost interface (right upper corner). This button runs the content of file 
+```sh
+tools/bmh_test/create_bmh.sh {NAME} {VBMC_PORT}


Suggested change

tools/bmh_test/create_bmh.sh {NAME} {VBMC_PORT}

tools/bmh_test/create_bmh.sh <NAME> <VBMC_PORT>

Usually mandatory parameters are marked with <> and optioanl parameters [] in documentation.

tuminoid · 2023-12-07T14:24:29Z

tools/bmh_test/bmo_net.xml

+      <bootp file='http://192.168.222.199:6180/boot.ipxe'/>
+    </dhcp>
+  </ip>
+</network>


Looks like missing linefeed at the end of file.

tuminoid · 2023-12-07T14:24:42Z

tools/bmh_test/clean_local_bmh_test_setup.sh

+
+# Clear network
+sudo virsh -c qemu:///system net-destroy bmo-test-network
+sudo virsh -c qemu:///system net-undefine bmo-test-network


Missing linefeed at the end of file.

tuminoid · 2023-12-07T14:26:22Z

tools/bmh_test/create_bmh.sh

@@ -0,0 +1,100 @@
+#!/bin/bash -x


Suggested change

#!/bin/bash -x

#!/usr/bin/env bash

If not executed in container, let user decide which bash he/she wants to use.

You also have set -x later, it is unnecessary here. Any options are better added as a separate line anyways, as in case of use executing this as bash ./script.sh, the -x would not take effect as shebang is not read.

tuminoid · 2023-12-07T14:28:20Z

tools/bmh_test/create_bmh.sh

+#                 VBMC runing
+# -------------------------------------------------------------------------------------------
+
+set -xe


Suggested change

set -xe

set -eux

You generally want to use -eux, ie. also add failing for undefined variables.
If you'd also use pipes | in the script, you'd also want to add -o pipefail by default.

tuminoid · 2023-12-07T14:45:23Z

tools/bmh_test/run_local_bmh_test_setup.sh

+
+# Install virtinst
+sudo apt update
+sudo apt install virtinst


Suggested change

sudo apt install virtinst

sudo apt-get -y install virtinst

Same

tuminoid · 2023-12-07T14:45:34Z

tools/bmh_test/run_local_bmh_test_setup.sh

+virsh -c qemu:///system net-start bmo-test-network
+
+# Install libvirt clients and daemon system, python3-pip and pip install vbmc
+sudo apt-get install libvirt-clients libvirt-daemon-system


Suggested change

sudo apt-get install libvirt-clients libvirt-daemon-system

sudo apt-get -y install libvirt-clients libvirt-daemon-system

tuminoid · 2023-12-07T14:46:04Z

tools/bmh_test/run_local_bmh_test_setup.sh

+
+# Install libvirt clients and daemon system, python3-pip and pip install vbmc
+sudo apt-get install libvirt-clients libvirt-daemon-system
+sudo apt-get install python3-pip


Suggested change

sudo apt-get install python3-pip

sudo apt-get -y install python3-pip

This and other apt-get should be a single line instead of 4.

tuminoid · 2023-12-07T14:47:08Z

tools/bmh_test/run_local_bmh_test_setup.sh

+# Install libvirt clients and daemon system, python3-pip and pip install vbmc
+sudo apt-get install libvirt-clients libvirt-daemon-system
+sudo apt-get install python3-pip
+pip install vbmc


Suggested change

pip install vbmc

pip install vbmc

pip installing stuff into the user's environment is not nice. How about creating virtualenv first?

tuminoid · 2023-12-07T14:48:15Z

tools/bmh_test/run_local_bmh_test_setup.sh

Since this utilizes sudo many times, consider checking if user is not root, and failing, and documenting it that it needs to be run sudo ./run_local_bmh_test_setup.sh instead.

dtantsur · 2023-12-08T15:50:10Z

The virsh/vbmc code gets copied over and over again. There is metal3-dev-env, there is now code in test/e2e. Could you maybe find a common place, ideally in metal3-dev-env? Maybe split some playbooks out of it?

Also, I'd recommend using sushy-tools instead of vbmc.

lentzi90 · 2023-12-11T07:46:01Z

The virsh/vbmc code gets copied over and over again. There is metal3-dev-env, there is now code in test/e2e. Could you maybe find a common place, ideally in metal3-dev-env? Maybe split some playbooks out of it?

Also, I'd recommend using sushy-tools instead of vbmc.

I explicitly want to get rid of the need for metal3-dev-env for these things. If it was just providing the VMs and BareMetalHosts fine, but it does way too much, which makes it extremely hard to work with.
However, I agree that we should not replicate this all over the place. I hope we can have a common way to initialize the BMO e2e tests and this tilt/dev setup. In the long term I think we may end up writing a little go-tool for it. Similar to make-bm-worker and make-virt-host.

tuminoid

Couple small nits.

tuminoid · 2023-12-11T07:55:49Z

tools/bmh_test/create_bmh.sh

+# Set default values
+NAME="${1:?}"
+VBMC_PORT="${2:?}"
+CONSUMER="${3}"


Btw, now that you have set -u, this will fail unless $3 and $4 are provided.

You want to have

Suggested change

CONSUMER="${3}"

CONSUMER="${3:-}"

So at least empty value is substituted, which works nicely with the [[ -n ... ]] which we have later, since it is checking undefined or empty value.

tuminoid · 2023-12-11T07:55:56Z

tools/bmh_test/create_bmh.sh

+NAME="${1:?}"
+VBMC_PORT="${2:?}"
+CONSUMER="${3}"
+CONSUMER_NAMESPACE="${4}"


tuminoid · 2023-12-11T07:57:48Z

tools/bmh_test/run_local_bmh_test_setup.sh

+set -euxo pipefail
+
+# Check if the script is run as root
+if [[ $UID != 0 ]]; then


Suggested change

if [[ $UID != 0 ]]; then

if [[ ${EUID} != 0 ]]; then

You should be checking effective UID, ${EUID} as that cannot be faked.

tuminoid · 2023-12-11T07:58:03Z

tools/bmh_test/run_local_bmh_test_setup.sh

+
+# Check if the script is run as root
+if [[ $UID != 0 ]]; then
+    echo "This script must be run as root" 


Suggested change

echo "This script must be run as root"

echo "ERROR: This script must be run as root"

lentzi90

Great progress @peppi-lotta !
This will make it much more enjoyable to work with BMO once finished 🙂

lentzi90 · 2023-12-11T07:54:43Z

tools/bmh_test/bmo_net.xml

Could we reuse hack/e2e/net.xml instead of adding this?

lentzi90 · 2023-12-11T07:56:30Z

tools/bmh_test/clean_local_bmh_test_setup.sh

+VM_LIST=$(virsh -c qemu:///system list --all --name)
+
+# Loop through the list and delete each virtual machine
+for vm_name in ${VM_LIST}; do
+    kubectl delete baremetalhost "${vm_name}"
+    virsh -c qemu:///system destroy --domain "${vm_name}"
+    virsh -c qemu:///system undefine --domain "${vm_name}" --remove-all-storage
+done


This is too dangerous. We should at least filter the VMs based on some prefix. How about we add a prefix to all created VMs and also mention this in the tilt UI so it is clear? Then we can filter for VMs with that prefix here.

lentzi90 · 2023-12-11T07:58:42Z

tools/bmh_test/create_bmh.sh

Could you try to split this so that we have one script for creating the VM and one for applying the BMH? Then we could reuse the first script in ci-e2e.sh and save some duplication 🙂

peppi-lotta · 2023-12-12T12:46:26Z

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

mquhuy · 2023-12-14T08:31:15Z

/test-ubuntu-integration-main

mquhuy · 2023-12-14T08:34:25Z

A minor comment: This PR currently has 4 commits, but most of them either fix issues from previous ones, or merge another branch onto this one. Maybe consider squashing them all to one?

mquhuy

Some nits

mquhuy · 2023-12-14T08:42:01Z

tools/bmh_test/run_local_bmh_test_setup.sh

+
+# Install virtinst, libvirt clients and daemon system
+apt-get -y update
+apt-get -y install virtinst libvirt-clients libvirt-daemon-system


Have we decided to only support Tilt in Ubuntu/Debian? If so, could you update the README with that information?

mquhuy · 2023-12-14T08:45:40Z

Tiltfile

+        icon_name='add_box',
+        text='Add New baremetalhost',
+        inputs=[
+            text_input('NAME', '"bmh-tes-" is automatically added to the begining of the name. This naming convention is later used to clean the local testing environment.'),


Typo. It should be "bmh-test-"

mquhuy · 2023-12-14T08:47:21Z

docs/dev-setup.md

+sudo tools/bmh_test/run_local_bmh_test_setup.sh
+```
+
+The network, VBMC, virtual vbmc environment and virtual machines can be run down with


imo "clean up" might be a better wording than "run down", at least I wouldn't understand that term.

peppi-lotta · 2023-12-15T10:17:39Z

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

lentzi90

Nice progress! I was able to test this locally and it worked! 🎉
A couple of comments below

lentzi90 · 2023-12-15T11:27:50Z

docs/dev-setup.md

+tools/bmh_test/create_bmh.sh <NAME> <VBMC_PORT>
+```
+
+and adds the values given to the button as arguments. Controlplane host can be created with


This works but was a bit hard to understand I think. Or maybe I am using it wrong. 😅 Could you make it so the drop down where we add the value also has a button for creating?
Current flow:

drop down and fill in values

"go back" and press the +

What I was thinking is like in CAPI:

drop down and fill in values

below the values press "Create"

I wasn't able to find a way to make this work in the second way. For what I was able to find Tilt doesn't support adding buttons to the drop down.

Ok, I think I was confusing myself. It looks different in different places apparently. The button is good 👍

lentzi90 · 2023-12-15T11:36:34Z

tools/bmh_test/run_local_bmh_test_setup.sh

+# Check if the script is run as root
+if [[ $EUID != 0 ]]; then
+    echo "ERROR: This script must be run as root" 
+    exit 1
+fi
+
+# Install virtinst, libvirt clients and daemon system
+apt-get -y update
+apt-get -y install virtinst libvirt-clients libvirt-daemon-system


I would rather check that virt-install is available end error out if not. So instead of trying to install these things, explain to the developer what is needed and let them set it up as they want.
We can also skip the root requirement then.

peppi-lotta · 2023-12-21T08:06:05Z

/test-ubuntu-integration-main
/test-centos-e2e-integration-main

lentzi90

Awesome! I was able to confirm that this is working nicely now!
Just one nit below

tools/bmh_test/run_local_bmh_test_setup.sh

lentzi90 · 2023-12-21T10:28:58Z

Looks like something happened when rebasing so now there are 2 commits 🤔
Maybe some merge conflict? Can you try fixing that and push again?
Other than that it looks great!

Signed-off-by: peppi-lotta <[email protected]>

peppi-lotta · 2023-12-22T08:25:30Z

/test-ubuntu-integration-main
/test-centos-e2e-integration-main

lentzi90

/lgtm

maxrantil · 2023-12-22T11:43:54Z

LGTM

kashifest

/approve
Nits only!
/hold

kashifest · 2024-01-08T06:55:42Z

docs/dev-setup.md

@@ -174,6 +174,32 @@ refer to
 [the development setup guide of CAPM3](https://github.com/metal3-io/cluster-api-provider-metal3/blob/main/docs/dev-setup.md#tilt-for-dev-in-capm3)
 and specially the [Baremetal Operator Integration](https://github.com/metal3-io/cluster-api-provider-metal3/blob/main/docs/dev-setup.md#including-baremetal-operator-and-ip-address-manager)

+### Making (virtual) BareMetalHosts with Tilt interface
+
+Virtinst, libvirt-clients, libvirt-daemon-system, and [Virtualbmc](https://pypi.org/project/virtualbmc/) are required to to create BareMetalHosts this way. The network and VBMC needed for making a BareMetalHosts can be initialized with


Suggested change

Virtinst, libvirt-clients, libvirt-daemon-system, and [Virtualbmc](https://pypi.org/project/virtualbmc/) are required to to create BareMetalHosts this way. The network and VBMC needed for making a BareMetalHosts can be initialized with

Virtinst, libvirt-clients, libvirt-daemon-system, and [Virtualbmc](https://pypi.org/project/virtualbmc/) are required to create BareMetalHosts this way. The network and VBMC needed for making a BareMetalHosts can be initialized with

kashifest · 2024-01-08T06:57:32Z

docs/dev-setup.md

+tools/bmh_test/create_bmh.sh <NAME> <VBMC_PORT>
+```
+
+and adds the values given to the button as arguments. Controlplane host can be created with


I am now forgetting the convention of this but I think we should rather write ControlPlane instead of Controlplane but please check it.

metal3-io-bot · 2024-01-08T07:01:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

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:

~~OWNERS~~ [kashifest]

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

lentzi90 · 2024-01-17T13:34:46Z

@peppi-lotta do you want to fix the nits here or in a follow up? This looks good to merge just need to remove the hold

peppi-lotta · 2024-01-18T07:59:09Z

@peppi-lotta do you want to fix the nits here or in a follow up? This looks good to merge just need to remove the hold

I'll do it on a follow up.

lentzi90 · 2024-01-18T08:10:14Z

/unhold

lentzi90 · 2024-01-18T08:10:26Z

/metal3-bmo-e2e-test

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 7, 2023

tuminoid reviewed Dec 7, 2023

View reviewed changes

peppi-lotta force-pushed the feature/add-bmh-button-tilt-PEPPI branch from 1ce8422 to 993339b Compare December 11, 2023 07:51

tuminoid reviewed Dec 11, 2023

View reviewed changes

lentzi90 reviewed Dec 11, 2023

View reviewed changes

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023

peppi-lotta force-pushed the feature/add-bmh-button-tilt-PEPPI branch from 58ccde7 to 26ddbe5 Compare December 12, 2023 09:09

metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023

mquhuy reviewed Dec 14, 2023

View reviewed changes

peppi-lotta force-pushed the feature/add-bmh-button-tilt-PEPPI branch 2 times, most recently from d59909a to 943d3d0 Compare December 15, 2023 10:10

lentzi90 reviewed Dec 15, 2023

View reviewed changes

lentzi90 mentioned this pull request Dec 15, 2023

🌱 Add BMO e2e upgrade test #1451

Merged

peppi-lotta force-pushed the feature/add-bmh-button-tilt-PEPPI branch from 93d21fe to 43e1af8 Compare December 18, 2023 11:42

lentzi90 reviewed Dec 21, 2023

View reviewed changes

tools/bmh_test/run_local_bmh_test_setup.sh Outdated Show resolved Hide resolved

peppi-lotta force-pushed the feature/add-bmh-button-tilt-PEPPI branch from 266bb6c to 925c12c Compare December 21, 2023 10:26

Tilt improved for easier baremetalhost creation

79af5f8

Signed-off-by: peppi-lotta <[email protected]>

peppi-lotta force-pushed the feature/add-bmh-button-tilt-PEPPI branch from 925c12c to 79af5f8 Compare December 21, 2023 10:55

lentzi90 reviewed Dec 22, 2023

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2023

kashifest reviewed Jan 8, 2024

View reviewed changes

metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 8, 2024

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2024

metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2024

metal3-io-bot merged commit 3f691c1 into metal3-io:main Jan 18, 2024
18 checks passed

metal3-io-bot deleted the feature/add-bmh-button-tilt-PEPPI branch January 18, 2024 08:53

adilGhaffarDev changed the title ~~✨ Develop User-Friendly baremetalhost creation for Tilt interface~~ 🌱 Develop User-Friendly baremetalhost creation for Tilt interface Apr 17, 2024

	tools/bmh_test/create_bmh.sh {NAME} {VBMC_PORT}
	tools/bmh_test/create_bmh.sh <NAME> <VBMC_PORT>

	sudo apt-get install libvirt-clients libvirt-daemon-system
	sudo apt-get -y install libvirt-clients libvirt-daemon-system

	sudo apt-get install python3-pip
	sudo apt-get -y install python3-pip

	echo "This script must be run as root"
	echo "ERROR: This script must be run as root"

	Virtinst, libvirt-clients, libvirt-daemon-system, and [Virtualbmc](https://pypi.org/project/virtualbmc/) are required to to create BareMetalHosts this way. The network and VBMC needed for making a BareMetalHosts can be initialized with
	Virtinst, libvirt-clients, libvirt-daemon-system, and [Virtualbmc](https://pypi.org/project/virtualbmc/) are required to create BareMetalHosts this way. The network and VBMC needed for making a BareMetalHosts can be initialized with

🌱 Develop User-Friendly baremetalhost creation for Tilt interface #1483

🌱 Develop User-Friendly baremetalhost creation for Tilt interface #1483

Conversation

peppi-lotta commented Dec 7, 2023

tuminoid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtantsur commented Dec 8, 2023

lentzi90 commented Dec 11, 2023

tuminoid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lentzi90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppi-lotta commented Dec 12, 2023

mquhuy commented Dec 14, 2023

mquhuy commented Dec 14, 2023

mquhuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppi-lotta commented Dec 15, 2023

lentzi90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppi-lotta commented Dec 21, 2023

lentzi90 left a comment

Choose a reason for hiding this comment

lentzi90 commented Dec 21, 2023

peppi-lotta commented Dec 22, 2023

lentzi90 left a comment

Choose a reason for hiding this comment

maxrantil commented Dec 22, 2023

kashifest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metal3-io-bot commented Jan 8, 2024

lentzi90 commented Jan 17, 2024

peppi-lotta commented Jan 18, 2024

lentzi90 commented Jan 18, 2024

lentzi90 commented Jan 18, 2024