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

Normalize virtual grain for LXC #60196

Conversation

piterpunk
Copy link

What does this PR do?

Fixes the virtual grains in LXC containers. Now it always set grains["virtual"] = "container" and grains["virtual_subtype"] = "LXC", independent of the command used for detection and even running inside another virtual machine.

What issues does this PR fix or reference?

Fixes #53868
Fixes #59573

Previous Behavior

The grains["virtual"] is inconsistent depending with the available commands for probing:

root@example:~# which systemd-detect-virt
root@example:~# which virt-what
root@example:~# salt-call grains.get virtual
local:
    kvm
root@example:~# which systemd-detect-virt
/usr/bin/systemd-detect-virt
root@example:~# which virt-what
root@example:~# salt-call grains.get virtual
local:
    LXC
root@example:~# which systemd-detect-virt
/usr/bin/systemd-detect-virt
root@example:~# which virt-what
/usr/sbin/virt-what
root@example:~# salt-call grains.get virtual
local:
    kvm

New Behavior

The grains["virtual"] and grains["virtual_subtype"] shows always the same output regardless the probe command.

root@example:~# which systemd-detect-virt
root@example:~# which virt-what
root@example:~# salt-call grains.get virtual
local:
    container
root@example:~# salt-call grains.get virtual_subtype
local:
    LXC
root@example:~# which systemd-detect-virt
/usr/bin/systemd-detect-virt
root@example:~# which virt-what
root@example:~# salt-call grains.get virtual
local:
    container
root@example:~# salt-call grains.get virtual_subtype
local:
    LXC
root@example:~# which systemd-detect-virt
/usr/bin/systemd-detect-virt
root@example:~# which virt-what
/usr/sbin/virt-what
root@example:~# salt-call grains.get virtual
local:
    container
root@example:~# salt-call grains.get virtual_subtype
local:
    LXC

Merge requirements satisfied?

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

Commits signed with GPG?

No

piterpunk added 2 commits May 15, 2021 19:25
- Consistently report grains["virtual"] = "container" and
  grains["virtual_subtype"] = "LXC"
- Makes virt-what parsing to stop after the first match
- Adds an additional verification for new LXC versions
- Moves the probing for containers to the end of detection routine,
  so the results wasn't overwrite by the hypervisor detection.
@piterpunk piterpunk requested a review from a team as a code owner May 16, 2021 00:42
@piterpunk piterpunk requested review from twangboy and removed request for a team May 16, 2021 00:42
@sagetherage sagetherage added severity-high 2nd top severity, seen by most users, causes major problems Silicon v3004.0 Release code name labels May 18, 2021
@sagetherage
Copy link
Contributor

@piterpunk
Re-running those mac tests, but they have been failing all over and so unlikely that it is due to your PR - I will check on the fix for those tests tomorrow and request reviews, today and will check back tomorrow, here. Thank you for your contribution!

@piterpunk
Copy link
Author

re-run pr-debian-9-amd64-py3-pytest

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

I don't think it /needs/ changing, but a couple of comments around the tests.

Looks good 👍

@@ -1232,6 +1232,90 @@ def test_lxc_virtual(self):
grains.get("virtual"), "container",
)

with patch.object(os.path, "isdir", MagicMock(return_value=False)):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking) patch actually creates a MagicMock already. Also it's typically a good idea to create_autospec. Also in this case, patching object is probably less-desirable. All-in-all, this could be written like so:

with patch("os.path.isdir", autospec=True, return_value=False):
    with patch("os.path.isfile", autospec=True, side_effect=lambda x: x in (...)): 

(note, no need to True if <something> else False - just <something> or bool(something) [but, thing in stuff produces a bool value anyway)

Copy link
Author

Choose a reason for hiding this comment

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

Hi @waynew ,

I just used the test_lxc_virtual as basis to create the new tests, that's why the code looks this way. Take a look in lines 1218 to 1223.

@Ch3LL Ch3LL merged commit 598b255 into saltstack:master Jun 2, 2021
@piterpunk piterpunk deleted the normalize_virtual_grain_for_LXC-Fix_Issue_59573 branch June 19, 2021 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity-high 2nd top severity, seen by most users, causes major problems Silicon v3004.0 Release code name
Projects
None yet
5 participants