-
Notifications
You must be signed in to change notification settings - Fork 170
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
Add bootc image builder case #5540
Add bootc image builder case #5540
Conversation
chunfuwen
commented
Mar 23, 2024
•
edited
Loading
edited
1e1f1b8
to
747307d
Compare
747307d
to
39ac336
Compare
6f999d2
to
7644adb
Compare
""" | ||
if not os.path.exists("/var/lib/libvirt/images/output"): | ||
os.makedirs("/var/lib/libvirt/images/output") | ||
cmd = "sudo podman run --rm -it --privileged --pull=newer --security-opt label=type:unconfined_t -v /var/lib/libvirt/images/output:/output" |
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.
Usually we run our tests as root, is this different here, do we need the sudo
?
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.
bootc image builder was used as normal user by default. But it need privileged permission, so from this point view, there is very tiny difference between normal user and root. Here use sudo is to keep compatibility if one day all environment need to switch to normal user.
""" | ||
public_key_path = os.path.join(os.path.expanduser("~/.ssh/"), "id_rsa.pub") | ||
if not os.path.exists(public_key_path): | ||
LOG.debug("public key doesn't exist, please create one") |
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.
It seems in the next step you are creating one, so "public rsa key doesn't exist, creating one..."
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.
changed to "public key doesn't exist, will help create one" to avoid confusion
etc_config = '' | ||
dnf_vmware_tool = '' | ||
|
||
# create VmWare tool folder |
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.
VMware https://www.vmware.com/
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.
updated
|
||
download_vmware_config_cmd = "curl https://gitlab.com/bootc-org/" \ | ||
"examples/-/raw/main/vmware/etc/vmware-tools/tools.conf > %s/tools.conf" % vmware_tool_path | ||
process.run(download_vmware_config_cmd, shell=True, verbose=True, ignore_status=True) |
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'd move these lines from if add_vmware_tool
into a separate function
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.
download_vmware_config_cmd is specific command to some tasks, this is not general one. The actually working one is actually process.run, which is already is one atomic function. Here download_vmware_config_cmd can be regarded as one input parameter for primitive function:process.run
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 understand. I meant there should be a function 'add_vmware_tool' to help with readability, maintaining the same level of abstraction
def add_vmware_tool(params):
....
vmware_tool_path = os.path.join(folder, "etc/vmware-tools/")
if not os.path.exists(vmware_tool_path):
os.makedirs(vmware_tool_path)
...
process.run(download_vmware_config_cmd, shell=True, verbose=True, ignore_status=True)
Same for following
def enable_fips(params):
...
dnf_fips_install = "RUN cat > /usr/lib/bootc/kargs.d/01-fips.toml <<'EOF'\n" \
"kargs = ['fips=1']\n" \
"match-architectures = ['x86_64']\n" \
"EOF\n" \
"RUN dnf install -y crypto-policies-scripts && " \
"update-crypto-policies --no-reload --set FIPS "
def setup_custom_repo(params):
...
repo_path = pathlib.Path(folder) / "rhel-9.4.repo"
...
crb_repo_content = f"""
[{repo_prefix}-crb]
name=beaker-CRB\n
baseurl={compose_url}/compose/CRB/{vm_arch_name}/os/
enabled=1
gpgcheck=0
sslverify=0\n
"""
try: | ||
install_vmware_govc_tool() | ||
setup_vCenter_env(params) | ||
delete_vm_if_existed(params) |
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.
if_existed
-> if_existing
or if_present
?
Or maybe just try_delete_vm
.
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.
changed to delete_vm_if_present
govc_install_cmd = "curl -L -o - 'https://github.com/vmware/govmomi/releases/latest/download/govc_Linux_x86_64.tar.gz' " \ | ||
"| tar -C /usr/local/bin -xvzf - govc" | ||
if not os.path.exists("/usr/local/bin/govc"): | ||
process.run(govc_install_cmd, shell=True, ignore_status=False) |
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.
IMO, the CI environment should provide this / a package should be provided via EPEL or internal repos.
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've created this draft PR for rpm generation - vmware/govmomi#3403
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 would like to keep code logic here, since it firstly check whether govc exist or not. And in case CI not handle it gracefully, we can still work. But it is true, we can firstly add package dependence in CI jobs, let CI handle it
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.
L. 248, couldn't target path for the command change? If you know the package name, I think we usually would use utils_package
and there is_installed
. If you don't want to use the package manager I think which gcov
returning != 0
would be more reliable.
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.
Both which gov and current way are reliable ways. I notice in many shell scripts, guy often use take further actions depending on whether specific folder exists or not
|
||
container_path = pathlib.Path(folder) / "Containerfile_tmp" | ||
shutil.copy("/etc/yum.repos.d/beaker-BaseOS.repo", folder) | ||
shutil.copy("/etc/yum.repos.d/beaker-AppStream.repo", folder) |
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.
Should these be rather configurable if we want to test on systems that are not provisioned by beaker?
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.
Currently there is no plan to use other provisioned system ,other than beaker. Maybe once there is new provision system, we can consider make it more flexible.
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 see that most of this code below is downstream-only, so not blocking, but in order to engage with the upstream community these paths should be moved to the config file, IMO. Even if you only want to run those yourself downstream, by moving them to a config file you make it easier for people upstream (or future test rounds) to update their configuration to their needs. By having these paths hardcoded in the script files, any upstream contributor who'd like to try another setup will first have to run into a test error, debug and fix the code, where as looking at the cfg file they can already expect to change those. The tests will even break if somebody tries to run these with Fedora where we only have a single repo file for all packages.
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.
Thank for understanding this. Once we has upstream uncompatiibility one day, I can update that when it comes
7644adb
to
5a3a92c
Compare
|
||
power_on_vm(params) | ||
add_vmware_tool = "yes" == params.get("add_vmware_tool") | ||
if add_vmware_tool: |
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.
You only use this once, so maybe it's shorter to just put
if params.get("add_vmware_tool") == "yes":
...
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.
updated
os.environ["DATA_CENTER"] = params.get("DATA_CENTER") | ||
os.environ["DATA_STORE"] = params.get("DATA_STORE") | ||
os.environ["GOVC_INSECURE"] = "true" | ||
process.run("govc about", shell=True, ignore_status=False) |
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.
Less duplication with:
for env_parm in ["GOVC_URL", "GOVC_USERNAME",...]:
os.environ[env_parm] = parasm.get(env_parm)
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.
updated
""" | ||
create empty vmdk on VM | ||
|
||
@param params: one dictionary wrapping various parameter |
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.
'various parameters'
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.
You only use "vm_name_bootc", maybe only pass that one instead of the full dict. This way you also won't have to re-read it in other functions such as import_iso_to_vCenter
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 met some situation where at the beginning, it may need one or two parameters only. But when there is requirements to expand this function, suddenly finding it need more than 4 or 5 parameters. So it seems that it is useful that we set parameter as dict at the beginning.
""" | ||
power on VM in vCenter | ||
|
||
@param params: one dictionary wrapping various parameter |
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.
'various parameters'
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.
updated
state_cmd = "govc vm.info -json %s |jq -r .virtualMachines[0].summary.runtime.powerState" % vm_name | ||
result = process.run(state_cmd, shell=True, verbose=True, ignore_status=False).stdout_text.strip() | ||
if result not in "poweredOn": | ||
raise exceptions.TestFail('The VM state is not powered on,actually is: %s') % result |
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.
",actually" -> ", actually"
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.
updated
Use virt install tool to install vm | ||
|
||
@param params: wrapped dictionary containing parameters | ||
""" |
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.
Why "wrapped"?
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.
remove "wrapped"
|
||
def create_qemu_vm(params, env, test): | ||
""" | ||
prepare environment,virt install,and login vm |
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.
",virt install,and" --> ", virt install, and"
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.
updated
""" | ||
prepare environment,virt install,and login vm | ||
|
||
@param params: one dictionary wrapping various parameter |
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.
various parameters
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.
updated
vm.start() | ||
user = params.get("os_username") | ||
passwd = params.get("os_password") | ||
ip_address = vm.wait_for_get_address(nic_index=0) |
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.
You don't seem to be using user
nor passwd
here, please remove them.
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.
updated
|
||
enable_tls_verify = "true" | ||
ownership = params.get("ownership") | ||
try: |
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'd rather you'd break down the try-block into smaller functions so it's easier to understand what is tested:
try:
prepare_environment(...)
build_container(...)
validate_build_output(...)
create_vm(...)
finally:
....
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.
updated
5a3a92c
to
85962fa
Compare
64df766
to
ccec8f4
Compare
12ca85f
to
2155970
Compare
2e0ce5a
to
7b1f7c1
Compare
c912f0f
to
a012cea
Compare
a012cea
to
6d61a13
Compare
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.
@chunfuwen Thanks for this. Some comments from me, please check. I found it a bit difficult to review, so please be patient with me.
One nitpick I've seen repeatedly is the different ways to document the params
parameter which I sometimes found confusing. After fixing the "@param" you might just use the same docstring for all. I think for the params
test parameters you should follow the lines of :param test: test object
and just call all instances :param params: test parameters
to avoid questions about 'wrapped' et al. sed -ei 's/:param params:.*/:param params: test parameters/g' <new code>
.
Secondly, the configuration files and some of the script files looks duplicated and there are ways to reduce this, s. my comment.
Finally, I think you should also move VMWare / govc related functions into their own file vmware_utils.py, same as you already did for aws_utils.py
Thank you. Sebas
container_url = params.get("container_url") | ||
local_container = "yes" == params.get("local_container") | ||
build_container = params.get("build_container") | ||
add_vmware_tool = "yes" == params.get("add_vmware_tool") |
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.
Seems this variable is not used anymore.
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.
remove it
govc_install_cmd = "curl -L -o - 'https://github.com/vmware/govmomi/releases/latest/download/govc_Linux_x86_64.tar.gz' " \ | ||
"| tar -C /usr/local/bin -xvzf - govc" | ||
if not os.path.exists("/usr/local/bin/govc"): | ||
process.run(govc_install_cmd, shell=True, ignore_status=False) |
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.
L. 248, couldn't target path for the command change? If you know the package name, I think we usually would use utils_package
and there is_installed
. If you don't want to use the package manager I think which gcov
returning != 0
would be more reliable.
|
||
container_path = pathlib.Path(folder) / "Containerfile_tmp" | ||
shutil.copy("/etc/yum.repos.d/beaker-BaseOS.repo", folder) | ||
shutil.copy("/etc/yum.repos.d/beaker-AppStream.repo", folder) |
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 see that most of this code below is downstream-only, so not blocking, but in order to engage with the upstream community these paths should be moved to the config file, IMO. Even if you only want to run those yourself downstream, by moving them to a config file you make it easier for people upstream (or future test rounds) to update their configuration to their needs. By having these paths hardcoded in the script files, any upstream contributor who'd like to try another setup will first have to run into a test error, debug and fix the code, where as looking at the cfg file they can already expect to change those. The tests will even break if somebody tries to run these with Fedora where we only have a single repo file for all packages.
""" | ||
Common method to check whether image build output exists | ||
|
||
:param params: one collective object representing wrapped parameters |
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.
here and elsewhere in this file: why "wrapped"? in general we just refer here to the "test parameters" or nitpicking, instance of "class Params representing the test parameters", mabye...
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.
updated
""" | ||
Download VmWare govc tool and install it | ||
|
||
:param params: wrap up all parameters |
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.
test parameters
?
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.
updated
|
||
def run(test, params, env): | ||
""" | ||
Test boot container image builder. |
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 you should update the docstring to describe on a high level what is being tested. AFAICT:
- Add existing bootable image to podman (cover rhel, fedora).
- Build image from bootable image with podman (AWS, QCOW2 et al.).
- Validate podman build image command output.
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.
updated
cleanup_files = [] | ||
|
||
|
||
def validate_bib_output(params, test): |
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 compared this validate_bib_output
with the one of the other script file. This function here rather seems to update / get updated test parameters instead of validating. The part "if not os.path.exists" also seems already covered in bootc_disk_image_build.py
, not sure if it's still necessary here; maybe this function should rather be called update_bib_env_info
or similar?
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.
updated
|
||
def run(test, params, env): | ||
""" | ||
Test install disk image generated by boot container image builder. |
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 you should update the docstring to describe on a high level what is being tested. AFAICT:
- Add existing bootable image to podman (cover rhel, fedora).
- Build image from bootable image with podman (AWS, QCOW2 et al.).
- Install the image and confirm it boots (on AWS cloud, locally or VMWare vCenter).
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.
updatated
vm_arch_name = params.get("vm_arch_name", "x86_64") | ||
aws_install_cmd = f"curl https://awscli.amazonaws.com/awscli-exe-linux-{vm_arch_name}.zip -o awscliv2.zip " \ | ||
f" && unzip awscliv2.zip -d ./ && ./aws/install " | ||
if not os.path.exists("/usr/local/bin/aws"): |
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.
Not blocking, just a comment:
Using snap packages (officially supported by AWS) or generating our own rpm packages removes the need for this kind of package cleanup logic.
https://docs.aws.amazon.com/cli/latest/userguide/getting-started-install.html
https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/9/html/administering_the_system_using_the_gnome_desktop_environment/assembly_installing-applications-using-flatpak_administering-the-system-using-the-gnome-desktop-environment
or
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.
Thank for bringing up this idea.Current solution is AWS official site to introduce how we install tools on linux, see https://docs.aws.amazon.com/cli/latest/userguide/getting-started-install.html
@@ -0,0 +1,158 @@ | |||
- bootc_image_builder.bib.disk_image_generation: |
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.
Not blocking, just a comment: the configuration looks mostly duplicated with the image_install.cfg. Maybe it would have been better to have a single config and script file making it easier to review and reducing duplication: the prepare_env_and_execute_bib
is duplicated between both script files.
Another way to reduce at least the duplication in the test config could be to reference different script files in the same test config: You can change the script file in your configuration file, though I haven't tried this myself, I've seen it in https://github.com/autotest/tp-qemu/blob/master/qemu/tests/cfg/s390x_cpu_model.cfg
- bootc_image_builder.bib:
...
- disk_image_generation:
type = bootc_disk_image_build
...
- disk_image_image_install:
type = bootc_disk_image_install
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.
image_install and image_generate has basic overraping in bib version, outputs image formats. But they vary a lot in later validation(see relative cfg files). For example, build doesn't care for uefi and bios boot difference since they share same image. But in install part, we need differ them whether it is biso or uefi install. And even in AWS, we need create different type ec2 instances. So it is better to keep them separate to allow flexibility to meet different reqirements
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.
Thanks for clarifying.
ca137dc
to
c6c92eb
Compare
c6c92eb
to
dc67026
Compare
dc67026
to
e8c5238
Compare
utilities and case scripts are provided, and cover Image type:ami vmdk qcow2 anaconda-iso raw container image reference: quay.io/centos-bootc/centos-bootc:stream9, quay.io/centos-bootc/fedora-bootc:eln, localhost/bootc:eln Signed-off-by: Chunfu Wen <[email protected]>
e8c5238
to
9101d4c
Compare
@smitterl , your comments are addressed, please help double check 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.
LGTM, thank you @chunfuwen
|
||
download_vmware_config_cmd = "curl https://gitlab.com/bootc-org/" \ | ||
"examples/-/raw/main/vmware/etc/vmware-tools/tools.conf > %s/tools.conf" % vmware_tool_path | ||
process.run(download_vmware_config_cmd, shell=True, verbose=True, ignore_status=True) |
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 understand. I meant there should be a function 'add_vmware_tool' to help with readability, maintaining the same level of abstraction
def add_vmware_tool(params):
....
vmware_tool_path = os.path.join(folder, "etc/vmware-tools/")
if not os.path.exists(vmware_tool_path):
os.makedirs(vmware_tool_path)
...
process.run(download_vmware_config_cmd, shell=True, verbose=True, ignore_status=True)
Same for following
def enable_fips(params):
...
dnf_fips_install = "RUN cat > /usr/lib/bootc/kargs.d/01-fips.toml <<'EOF'\n" \
"kargs = ['fips=1']\n" \
"match-architectures = ['x86_64']\n" \
"EOF\n" \
"RUN dnf install -y crypto-policies-scripts && " \
"update-crypto-policies --no-reload --set FIPS "
def setup_custom_repo(params):
...
repo_path = pathlib.Path(folder) / "rhel-9.4.repo"
...
crb_repo_content = f"""
[{repo_prefix}-crb]
name=beaker-CRB\n
baseurl={compose_url}/compose/CRB/{vm_arch_name}/os/
enabled=1
gpgcheck=0
sslverify=0\n
"""
@@ -0,0 +1,158 @@ | |||
- bootc_image_builder.bib.disk_image_generation: |
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.
Thanks for clarifying.
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