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.

Sign up for GitHub

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

Jump to bottom

Port #47620 to master #56817

Closed
wants to merge 6 commits into from
Closed

Port #47620 to master #56817

wants to merge 6 commits into from

Conversation

Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Apr 21, 2020

Port #47620 to master

@Ch3LL Ch3LL requested a review from a team as a code owner April 21, 2020 22:48
@ghost ghost requested review from dwoz and removed request for a team April 21, 2020 22:48
@Ch3LL
Copy link
Contributor Author

Ch3LL commented Apr 23, 2020

ping @cbosdo would you mind reviewing here. I had to rebase with the changes you recently got merged and make some changes from the original PR to ensure loader worked with the new defined function as well.

@cbosdo
Copy link
Contributor

cbosdo commented Apr 24, 2020

@Ch3LL I completely forgot about that PR... and there are some bits in the UEFI / loader handling that would need to be rethinked.

  • For libvirt before 5.4.0 the user would need to provide both the loader and the nvram template for most UEFI cases (maybe not bhyve) and after, just a simple flag saying UEFI is wanted will suffice. For this I would move the virt.init() loader parameter in the boot one. This would allow adding nvram and other boot-related params later.

  • This PR only touches the virt.init() function, but not the virt.update() one... and thus there will be problems when reapplying the virt.running or virt.defined states.

How do you see this backport PR? Just stick to what was in the original PR and then file another PR to fix the above mentioned points or add the above points to this PR? In the latter case I would probably have to take over your PR to complete and fix it.

cbosdo
cbosdo suggested changes Apr 24, 2020
View reviewed changes
Copy link
Contributor

@cbosdo cbosdo left a comment

Choose a reason for hiding this comment

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

Apart from the missing documentation in the new virt.defined state, the backport is matching the original PR. However as mentioned in the previous comment, the original PR would have needed changes. These may be lifted in a subsequent PR.

salt/modules/virt.py
Path to the UEFI firmware binary

Optionally, you can provide arbitrary attributes such as ``readonly`` or ``type``. See
the libvirt documentation for all supported loader parameters.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like the way this parameter is handled. This really flexible, but doesn't help clarifying what users can put here. Furthermore it doesn't handle the nvram templates needed for most UEFI loaders... and should be merged with the boot parameter.

salt/modules/virt.py
if key == "path":
continue
loader_attributes.append("{key}='{val}'".format(key=key, val=val))
loader["_attributes"] = " ".join(loader_attributes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Opens the door to undocumented parameters

salt/states/virt.py
See the **Loader Definition** section of the :py:func:`virt.init
<salt.modules.virt.init>` function for more details on this dictionary.

.. versionadded:: Sodium
Copy link
Contributor

Choose a reason for hiding this comment

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

That parameter documentation should also be in the defined() state

tests/unit/modules/test_virt.py
)
root = ET.fromstring(xml_data)
self.assertFalse("loader" in root.find("os"))

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a test case for virt.update with loader defined

@cbosdo
Copy link
Contributor

cbosdo commented Apr 24, 2020

For reference the UEFI part of this PR overlaps with merged PR #56613

@cbosdo
Copy link
Contributor

cbosdo commented Apr 24, 2020

@Ch3LL PR #56882 is my attempt at the backport removing all the loader parameters since those have been added in the boot parameter already. The only missing thing is the possibility to set the secure="yes" flag. But I think I will have a go at it later

@cbosdo
Copy link
Contributor

cbosdo commented Apr 27, 2020

since the other PR has been merged, this one can be closed

@Ch3LL
Copy link
Contributor Author

Ch3LL commented Apr 27, 2020

Thanks your awesome!

@Ch3LL Ch3LL closed this Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbosdo cbosdo cbosdo requested changes

@dwoz dwoz Awaiting requested review from dwoz

Assignees
No one assigned
Labels
master-port
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ch3LL @cbosdo @jeroen92