Add efi parameter GSoC #57396

gqlo · 2020-05-20T15:13:33Z

What does this PR do?

Allows user to specify efi parameter so that uefi firmware can be auto selected.
This PR also fix a bug on removing loader and nvram path. Previous code will not work because None is handled as a string in sls file. We need to use boot_tag_value == "None" to check that.

What issues does this PR fix or reference?

In response to feature request #57397
Fixes: a bug on changing back to BIOS boot.

Previous Behavior

Removing UEFI firmware path will not work.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Docs
Changelog - https://docs.saltstack.com/en/master/topics/development/changelog.html
Tests written/updated

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

cbosdo

See my comments

cbosdo · 2020-05-20T15:33:29Z

salt/modules/virt.py

@@ -1627,7 +1659,8 @@ def init(
        This is an optional parameter, all of the keys are optional within the dictionary. The structure of
        the dictionary is documented in :ref:`init-boot-def`. If a remote path is provided to kernel or initrd,
        salt will handle the downloading of the specified remote file and modify the XML accordingly.
-        To boot VM with UEFI, specify loader and nvram path.
+        To boot VM with UEFI, specify loader and nvram path or specify 'efi': ``True`` if your libvirtd version
+        is >= 5.4.0.


Should we also add the minimal qemu version here? I have no idea which it is however

Yeah, I will note 3.0.0 as the firmware metadata specification documented

cbosdo · 2020-05-20T15:57:00Z

salt/modules/virt.py

+    efi_value = boot.get("efi", None) if boot else None
+    parent_tag = desc.find("os")
+    if efi_value is not None:
+        if efi_value is True:
+            parent_tag.set("firmware", "efi")
+        elif efi_value is False:
+            parent_tag.attrib.pop("firmware", None)
+        else:
+            SaltInvocationError("Invalid efi value")
+        return True
+    return False


This function should compare the firmware value in the XML and the boot value and only change if needed. Here it forces the boot.get("efi") value into the XML instead of only when needed

I just discovered that os attribute will be gone after we run the newly defined VM. if we just defined a new VM without starting it. nvram and loader will not be filled yet. Just updated the code to check against those cases.

cbosdo · 2020-05-20T15:57:59Z

salt/modules/virt.py

-            if boot_tag_value is None and boot is not None and parent_tag is not None:
+            if boot_tag_value == "None" and boot is not None and parent_tag is not None:


strange... why "None" as a string? that shouldn't happen

It seems like passing None in the sls file is handled as a string.

@gqlo can you provide some insight into a situation where you would be passing None as an argument in an SLS file?

@garethgreenaway Hi garethgreen I added the feature to support booting VM with uefi in this PR #56613, in the case where we want to change back to BIOS boot or remove booting directly from a specified kernel, passing None as an argument would work. Do you have better ideas on this ?

The SLS / Yaml mapping for python's None is Null

* Fix various spelling mistakes in master branch (saltstack#55954) * Fix typo of additional Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of against Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of amount Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of argument Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of attempt Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of bandwidth Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of caught Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of compatibility Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of consistency Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of conversions Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of corresponding Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of dependent Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of dictionary Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of disabled Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of adapters Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of disassociates Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of changes Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of command Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of communicate Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of community Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of configuration Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of default Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of absence Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of attribute Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of container Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of described Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of existence Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of explicit Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of formatted Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of guarantees Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of hexadecimal Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of hierarchy Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of initialize Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of label Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of management Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of mismatch Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of don't Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of manually Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of getting Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of information Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of meant Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of nonexistent Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of occur Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of omitted Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of normally Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of overridden Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of repository Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of separate Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of separator Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of specific Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of successful Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of succeeded Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of support Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of version Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of that's Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of "will be removed" Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of release Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of synchronize Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of python Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of usually Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of override Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of running Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of whether Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of package Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of persist Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of preferred Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of present Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix typo of run Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix spelling mistake of "allows someone to..." "Allows to" is not correct English. It must either be "allows someone to" or "allows doing". Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix spelling mistake of "number of times" Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix spelling mistake of msgpack Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix spelling mistake of daemonized Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix spelling mistake of daemons Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix spelling mistake of extemporaneous Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix spelling mistake of instead Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix spelling mistake of returning Signed-off-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> * Fix literal comparissons * virt: Convert cpu_baseline ElementTree to string In commit 0f5184c (Remove minidom use in virt module) the value of `cpu` become `xml.etree.ElementTree.Element` and no longer has a method `toxml()`. This results in the following error: $ salt '*' virt.cpu_baseline host2: The minion function caused an exception: Traceback (most recent call last): File "/usr/lib/python3.7/site-packages/salt/minion.py", line 1675, in _thread_return return_data = minion_instance.executors[fname](opts, data, func, args, kwargs) File "/usr/lib/python3.7/site-packages/salt/executors/direct_call.py", line 12, in execute return func(*args, **kwargs) File "/usr/lib/python3.7/site-packages/salt/modules/virt.py", line 4410, in cpu_baseline return cpu.toxml() AttributeError: 'xml.etree.ElementTree.Element' object has no attribute 'toxml' Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org> * PR#57374 backport virt: pool secret should be undefined in pool_undefine not pool_delete virt: handle build differently depending on the pool type virt: don't fail if the pool secret has been removed * PR saltstack#57396 backport add firmware auto select feature * virt: Update dependencies Closes: saltstack#57641 Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org> * use null in sls file to map None object add sls file example reword doc * Update virt module and states and their tests to python3 * PR saltstack#57545 backport Move virt.init boot_dev parameter away from the kwargs virt: handle boot device in virt.update() virt: add boot_dev parameter to virt.running state * PR saltstack#57431 backport virt: Handle no available hypervisors virt: Remove unused imports * Blacken salt * Add method to remove circular references in data objects and add test (saltstack#54930) * Add method to remove circular references in data objects and add test * remove trailing whitespace * Blacken changed files Co-authored-by: xeacott <jeacott@saltstack.com> Co-authored-by: Frode Gundersen <fgundersen@saltstack.com> Co-authored-by: Daniel A. Wozniak <dwozniak@saltstack.com> * PR saltstack#58332 backport virt: add debug log with VM XML definition Add xmlutil.get_xml_node() helper function Add salt.utils.data.get_value function Add change_xml() function to xmlutil virt.update: refactor the XML diffing code virt.test_update: move some code to make test more readable Co-authored-by: Benjamin Drung <benjamin.drung@cloud.ionos.com> Co-authored-by: Pedro Algarvio <pedro@algarvio.me> Co-authored-by: Radostin Stoyanov <rstoyanov@fedoraproject.org> Co-authored-by: Firefly <leevn2011@hotmail.com> Co-authored-by: Blacken Salt <jenkins@saltstack.com> Co-authored-by: Joe Eacott <31625359+xeacott@users.noreply.github.com> Co-authored-by: xeacott <jeacott@saltstack.com> Co-authored-by: Frode Gundersen <fgundersen@saltstack.com> Co-authored-by: Daniel A. Wozniak <dwozniak@saltstack.com>

gqlo requested a review from a team as a code owner May 20, 2020 15:13

ghost requested review from twangboy and removed request for a team May 20, 2020 15:13

gqlo force-pushed the add_efi_parameter branch from 22d2346 to d614182 Compare May 20, 2020 15:38

cbosdo suggested changes May 20, 2020

View reviewed changes

gqlo force-pushed the add_efi_parameter branch from be819d9 to 801b454 Compare May 21, 2020 06:49

add firmware auto select feature

5aac915

gqlo force-pushed the add_efi_parameter branch from 801b454 to 5aac915 Compare May 21, 2020 08:12

dwoz approved these changes May 22, 2020

View reviewed changes

dwoz merged commit 589762c into saltstack:master May 22, 2020

sagetherage added the ZRelease-Sodium retired label label May 22, 2020

gqlo changed the title ~~Add efi parameter~~ Add efi parameter GSoC Aug 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add efi parameter GSoC #57396

Add efi parameter GSoC #57396

gqlo commented May 20, 2020 •

edited

Loading

cbosdo left a comment

cbosdo May 20, 2020

gqlo May 21, 2020 •

edited

Loading

cbosdo May 20, 2020

gqlo May 21, 2020 •

edited

Loading

cbosdo May 20, 2020

gqlo May 21, 2020

garethgreenaway May 21, 2020

gqlo May 22, 2020

cbosdo May 25, 2020

		if boot_tag_value is None and boot is not None and parent_tag is not None:
		if boot_tag_value == "None" and boot is not None and parent_tag is not None:

Add efi parameter GSoC #57396

Add efi parameter GSoC #57396

Conversation

gqlo commented May 20, 2020 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

Merge requirements satisfied?

Commits signed with GPG?

cbosdo left a comment

Choose a reason for hiding this comment

cbosdo May 20, 2020

Choose a reason for hiding this comment

gqlo May 21, 2020 • edited Loading

Choose a reason for hiding this comment

cbosdo May 20, 2020

Choose a reason for hiding this comment

gqlo May 21, 2020 • edited Loading

Choose a reason for hiding this comment

cbosdo May 20, 2020

Choose a reason for hiding this comment

gqlo May 21, 2020

Choose a reason for hiding this comment

garethgreenaway May 21, 2020

Choose a reason for hiding this comment

gqlo May 22, 2020

Choose a reason for hiding this comment

cbosdo May 25, 2020

Choose a reason for hiding this comment

gqlo commented May 20, 2020 •

edited

Loading

gqlo May 21, 2020 •

edited

Loading

gqlo May 21, 2020 •

edited

Loading