(PA-4569) Changes to use bundled virt-what

[skyamgarp]

This is to resolve issue with the jira: https://perforce.atlassian.net/browse/PA-4569.
Reporter has mentioned we are not using our bundled virt-what. Our bundled virt-what seems to be returning correct value for user, with this PR we can use our bundled virt-what. This change will help when there is no virt-what installed on the system(This is assumption based on the jira issue). The component is built only for linux systems.

[joshcooper]

Could you expand on what the expected behavior is? Some questions that come to mind are:

What happens if we call our virt-what instead of using an unqualified path? How will facter's behavior change?

I don't know if we've updated our virt-what recently. How likely is it going to be older than what folks have already installed?

Do you have an idea of what facts may change as a result? Are there cases where virt-what resolves things differently than facter's built in resolvers?

[skyamgarp]

@joshcooper

Could you expand on what the expected behaviour is? Some questions that come to mind are:

-> This is to resolve issue with the jira: https://perforce.atlassian.net/browse/PA-4569.
Reporter has mentioned we are not using our bundled virt-what.Our bundled virt-what seems to be returning correct value for user, with this PR we can use our bundled virt-what. This change will help when there is no virt-what installed on the system(This is assumption based on the jira issue). The component is built only for linux systems.
Though I feel we need exact system details for which the user has reported the issue. Please provide your thoughts on this.

What happens if we call our virt-what instead of using an unqualified path? How will facter's behavior change?

-> We are trying to use our built virt-what here with /opt/puppetlabs/puppet/bin/virt-what.

I don't know if we've updated our virt-what recently. How likely is it going to be older than what folks have already installed?

-> I see recent update to our component. puppetlabs-toy-chest/puppet-runtime#588.

Do you have an idea of what facts may change as a result? Are there cases where virt-what resolves things differently than facter's built in resolvers?

-> We can close this PR in case we would want to have more system details. Here Assuming facter could not resolve to KVM but our bundled virt-what did as mentioned in the jira. Possible Impacted facts where we are using virt-what component:

• xen
• hypervisors.kvm
• hypervisors.virtualbox
• hypervisors.vmware

[joshcooper]

Thanks @skyamgarp, I think the code changes are fine, but I'm concerned about the scope of the change and our lack of test coverage to assess the risk.

For example, virt-what 1.19 is available in Debian 11. With this change, facter would run our virt-what 1.25 instead. Does our virt-what work on Debian 11? It should, but we don't test it (via beaker) anywhere in CI, so I don't really know.

The other risk I see are cases where virt-what isn't currently installed on the system, so previously we would have used facter to resolve those facts. But with this change, we'll start using virt-what 1.25. I would expect virt-what to more accurately detect the hypervisor, but I don't really know.

[bastelfreak]

I think to use the vendored version makes sense (why was it vendored when it wasn't used it? Is that a bug or do other parts rely in virt-what?). However I agree with @joshcooper and think this needs to be tested on some platforms to understand how/where this has potential impact.

And from a packager point of view: I build facter/puppet for other platforms and it would be great if you could document this logic somewhere in the docs.

[joshcooper]

Also forgot to mention that facter 3.x used to delegate to our vendored virt-what

facter/lib/src/facts/linux/virtualization_resolver.cc

Line 107 in 6d7ffc6

string virt_what = agent::which("virt-what");

but that behavior was lost when facter 4 was written. So I'm generally a fan of using our vendored virt-what again.

[ekohl]

Should facter ensure that /opt/puppetlabs/puppet/bin is in its PATH instead? That would make it friendlier for non-AIO builds.

[skyamgarp]

Update: This is Debian 11 Intel. I don't see virt-what present already but bundled virt-what works.

[skyamgarp]

I think to use the vendored version makes sense (why was it vendored when it wasn't used it? Is that a bug or do other parts rely in virt-what?). However I agree with @joshcooper and think this needs to be tested on some platforms to understand how/where this has potential impact.

And from a packager point of view: I build facter/puppet for other platforms and it would be great if you could document this logic somewhere in the docs.

@bastelfreak
I did not understand the last point about packager point of view. Can you please elaborate more on that?

[skyamgarp]

Should facter ensure that /opt/puppetlabs/puppet/bin is in its PATH instead? That would make it friendlier for non-AIO builds.

We don't want to share the private bin directory.
cc: @joshcooper

[ekohl]

I did not understand the last point about packager point of view. Can you please elaborate more on that?

Facter is also packaged outside of Puppet's AIO. For example:

• Arch Linux
• Debian
• Fedora
• Ubuntu

We don't want to share the private bin directory.

IMHO this current PR already does that.

[ekohl]

If anything, you need to make sure it's executable

Suggested change

	command = if File.readable?('/opt/puppetlabs/puppet/bin/virt-what')
	command = if File.executable?('/opt/puppetlabs/puppet/bin/virt-what')

[bastelfreak]

We don't want to share the private bin directory.

You don't have to set it in /etc/profile.d/puppet-agent.sh. But if facter manipulates it's own PATH variable at startup, the whole code could use relative paths and that would make it easier for people that package facter/puppet, but without vendored virt-what, to use system virt-what.

[joshcooper]

But if facter manipulates it's own PATH variable at startup, the whole code could use relative paths...

We have some precedence in

facter/lib/facter/resolvers/augeas.rb

Lines 26 to 30 in e776b42

	command = if File.readable?('/opt/puppetlabs/puppet/bin/augparse')
	'/opt/puppetlabs/puppet/bin/augparse'
	else
	'augparse'
	end

so I'd prefer to keep the "check for the absolute AIO path and fallback to relative path" in this PR. I think the "add private bin dir to the in-process PATH env variable so that we don't have to special case the directory" should be filed as a separate issue.

I think the remaining issue with this PR is whether we can actually merge it without introducing a breaking change? I'm less certain about that. Maybe it has to wait for Facter 5?

[skyamgarp]

@joshcooper , @bastelfreak , @ekohl

Based on our discussion:

1. We can either proceed with the changes in this PR and release Facter 4.9.0, or
2. We can close this PR since there are no other similar issues, and wait for Facter 5.

[skyamgarp]

Closing this PR as we would be doing this change in facter 5.
cc: @joshcooper , @amitkarsale