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

(FACT-3202) Add is_virtual and virtual support for crio #2574

Merged
merged 2 commits into from
Jun 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/facter/framework/core/file_loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,7 @@
when 'linux'
require_relative '../../util/linux/dhcp'
require_relative '../../util/linux/if_inet6'
require_relative '../../util/linux/proc'
require_relative '../../util/linux/routing_table'
require_relative '../../util/linux/socket_parser'

Expand Down
56 changes: 38 additions & 18 deletions lib/facter/resolvers/containers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,45 +14,65 @@ class << self
private

def post_resolve(fact_name, _options)
@fact_list.fetch(fact_name) { read_cgroup(fact_name) }
@fact_list.fetch(fact_name) do
read_environ(fact_name) || read_cgroup(fact_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

If /proc/1/environ contains container=<value> where <value> is non-empty and is not one of the values in the case, then we set vm=container_other and never fall back to read_cgroup .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, perhaps read_environ() should only log a warning and not set the value to container_other

Copy link
Contributor

Choose a reason for hiding this comment

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

@lollipopman prior to your change, we checked /proc/1/cgroups for docker

And then 'lxc' or fell back to checking the container environment variable in /proc/1/environ

vm = 'lxc' if output_lxc || lxc_from_environ

Your commit reversed the order, so it looks in /proc/1/environ first.

read_environ(fact_name) || read_cgroup(fact_name)

Was that intentional? Would it be a problem if I reversed it?

One thing I didn't realize when I merged this is the VirtualDetector calls

Facter::Resolvers::Containers.resolve(:vm)

So the Containers resolver can't hard code container_other https://github.com/puppetlabs/facter/blob/0656d9a34ce4790129f1fd6eba5bb4d49a9b9ad1/lib/facter/resolvers/containers.rb#L60C19-L60C34

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I had a strong reason for changing the order, and my fault for not adding a comment if I did. I assume my rationale was that the environment variable was more authoritative, but I don't think that argument is very strong at present. As for the hard coding of container_other I agree that should be dropped and just return nil for the fact.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries @lollipopman, thanks for confirming!

end
end

def read_cgroup(fact_name)
output_cgroup = Facter::Util::FileHelper.safe_read('/proc/1/cgroup', nil)
output_environ = Facter::Util::FileHelper.safe_read('/proc/1/environ', nil)
return unless output_cgroup && output_environ
return unless output_cgroup

output_docker = %r{docker/(.+)}.match(output_cgroup)
output_lxc = %r{^/lxc/([^/]+)}.match(output_cgroup)
lxc_from_environ = /container=lxc/ =~ output_environ

info, vm = extract_vm_and_info(output_docker, output_lxc, lxc_from_environ)
info, vm = extract_for_nspawn(output_environ) unless vm
info, vm = extract_vm_and_info(output_docker, output_lxc)
@fact_list[:vm] = vm
@fact_list[:hypervisor] = { vm.to_sym => info } if vm
@fact_list[fact_name]
end

def read_environ(fact_name)
begin
container = Facter::Util::Linux::Proc.getenv_for_pid(1, 'container')
rescue StandardError => e
log.warn("Unable to getenv for pid 1, '#{e}'")
return nil
end
return if container.nil? || container.empty?

info = {}
case container
when 'lxcroot'
vm = 'lxc'
when 'podman'
vm = 'podman'
lollipopman marked this conversation as resolved.
Show resolved Hide resolved
when 'crio'
vm = 'crio'
when 'systemd-nspawn'
vm = 'systemd_nspawn'
info = { 'id' => Facter::Util::FileHelper.safe_read('/etc/machine-id', nil).strip }
else
vm = 'container_other'
lollipopman marked this conversation as resolved.
Show resolved Hide resolved
log.warn("Container runtime, '#{container}', is unsupported, setting to, '#{vm}'")
end
@fact_list[:vm] = vm
@fact_list[:hypervisor] = { vm.to_sym => info } if vm
@fact_list[fact_name]
end

def extract_vm_and_info(output_docker, output_lxc, lxc_from_environ)
def extract_vm_and_info(output_docker, output_lxc)
vm = nil
if output_docker
vm = 'docker'
info = output_docker[1]
elsif output_lxc
vm = 'lxc'
info = output_lxc[1]
end
vm = 'lxc' if output_lxc || lxc_from_environ
info = output_lxc[1] if output_lxc

[info ? { INFO[vm] => info } : {}, vm]
end

def extract_for_nspawn(output_environ)
nspawn = /container=systemd-nspawn/ =~ output_environ
if nspawn
vm = 'systemd_nspawn'
info = Facter::Util::FileHelper.safe_read('/etc/machine-id', nil)
end
[info ? { 'id' => info.strip } : {}, vm]
end
end
end
end
Expand Down
6 changes: 4 additions & 2 deletions lib/facter/util/file_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@ def safe_read(path, default_return = '')
default_return
end

def safe_readlines(path, default_return = [])
return File.readlines(path, encoding: Encoding::UTF_8) if File.readable?(path)
# rubocop:disable Style/SpecialGlobalVars
def safe_readlines(path, default_return = [], sep = $/, chomp: false)
return File.readlines(path, sep, chomp: chomp, encoding: Encoding::UTF_8) if File.readable?(path)
joshcooper marked this conversation as resolved.
Show resolved Hide resolved

log_failed_to_read(path)
default_return
end
# rubocop:enable Style/SpecialGlobalVars

# This previously acted as a helper method for versions of Ruby older
# than 2.5, before Dir.children was added. As it isn't a private
Expand Down
21 changes: 21 additions & 0 deletions lib/facter/util/linux/proc.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# frozen_string_literal: true

module Facter
module Util
module Linux
class Proc
class << self
def getenv_for_pid(pid, field)
path = "/proc/#{pid}/environ"
lines = Facter::Util::FileHelper.safe_readlines(path, [], "\0", chomp: true)
lines.each do |line|
key, value = line.split('=', 2)
return value if key == field
end
nil
end
end
end
end
end
end
25 changes: 18 additions & 7 deletions spec/facter/resolvers/containers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
allow(Facter::Util::FileHelper).to receive(:safe_read)
.with('/proc/1/cgroup', nil)
.and_return(cgroup_output)
allow(Facter::Util::FileHelper).to receive(:safe_read)
.with('/proc/1/environ', nil)
allow(Facter::Util::FileHelper).to receive(:safe_readlines)
.with('/proc/1/environ', [], "\0", chomp: true)
.and_return(environ_output)
end

Expand All @@ -18,7 +18,7 @@

context 'when hypervisor is docker' do
let(:cgroup_output) { load_fixture('docker_cgroup').read }
let(:environ_output) { 'PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin' }
let(:environ_output) { ['PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin'] }
let(:result) { { docker: { 'id' => 'ee6e3c05422f1273c9b41a26f2b4ec64bdb4480d63a1ad9741e05cafc1651b90' } } }

it 'return docker for vm' do
Expand All @@ -32,7 +32,7 @@

context 'when hypervisor is nspawn' do
let(:cgroup_output) { load_fixture('cgroup_file').read }
let(:environ_output) { 'PATH=/usr/local/sbin:/bincontainer=systemd-nspawnTERM=xterm-256color' }
let(:environ_output) { ['PATH=/usr/local/sbin:/bin', 'container=systemd-nspawn', 'TERM=xterm-256color'] }
let(:result) { { systemd_nspawn: { 'id' => 'ee6e3c05422f1273c9b41a26f2b4ec64bdb4480d63a1ad9741e05cafc1651b90' } } }

before do
Expand All @@ -52,7 +52,7 @@

context 'when hypervisor is lxc and it is discovered by cgroup' do
let(:cgroup_output) { load_fixture('lxc_cgroup').read }
let(:environ_output) { 'PATH=/usr/local/sbin:/sbin:/bin' }
let(:environ_output) { ['PATH=/usr/local/sbin:/sbin:/bin'] }
let(:result) { { lxc: { 'name' => 'lxc_container' } } }

it 'return lxc for vm' do
Expand All @@ -66,7 +66,7 @@

context 'when hypervisor is lxc and it is discovered by environ' do
let(:cgroup_output) { load_fixture('cgroup_file').read }
let(:environ_output) { 'container=lxcroot' }
let(:environ_output) { ['container=lxcroot'] }
let(:result) { { lxc: {} } }

it 'return lxc for vm' do
Expand All @@ -80,7 +80,7 @@

context 'when hypervisor is neighter lxc nor docker' do
let(:cgroup_output) { load_fixture('cgroup_file').read }
let(:environ_output) { 'PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin' }
let(:environ_output) { ['PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin'] }
let(:result) { nil }

it 'return lxc for vm' do
Expand All @@ -91,4 +91,15 @@
expect(containers_resolver.resolve(:hypervisor)).to eq(result)
end
end

context 'when hypervisor is an unknown container runtime discovered by environ' do
let(:cgroup_output) { load_fixture('cgroup_file').read }
let(:environ_output) { ['container=UNKNOWN'] }
let(:logger) { Facter::Log.class_variable_get(:@@logger) }

it 'return container_other for vm' do
expect(logger).to receive(:warn).with(/Container runtime, 'UNKNOWN', is unsupported, setting to, 'container_other'/)
expect(containers_resolver.resolve(:vm)).to eq('container_other')
end
end
end
7 changes: 5 additions & 2 deletions spec/facter/util/file_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
let(:path) { '/Users/admin/file.txt' }
let(:entries) { ['file.txt', 'a'] }
let(:content) { 'file content' }
# rubocop:disable Style/SpecialGlobalVars
let(:sep) { $/ }
# rubocop:enable Style/SpecialGlobalVars
let(:error_message) do
"Facter::Util::FileHelper - #{Facter::CYAN}File at: /Users/admin/file.txt is not accessible.#{Facter::RESET}"
end
Expand Down Expand Up @@ -114,7 +117,7 @@

describe '#safe_read_lines' do
before do
allow(File).to receive(:readlines).with(path, anything).and_return(array_content)
allow(File).to receive(:readlines).with(path, sep, anything).and_return(array_content)
end

context 'when successfully read the file lines' do
Expand All @@ -133,7 +136,7 @@
it 'File.readlines is called with the correct path' do
file_helper.safe_readlines(path)

expect(File).to have_received(:readlines).with(path, anything)
expect(File).to have_received(:readlines).with(path, sep, anything)
end

it "doesn't log anything" do
Expand Down
38 changes: 38 additions & 0 deletions spec/facter/util/linux/proc_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# frozen_string_literal: true

describe Facter::Util::Linux::Proc do
describe '#getenv_for_pid' do
before do
allow(Facter::Util::FileHelper).to receive(:safe_readlines)
.with('/proc/1/environ', [], "\0", { chomp: true })
.and_return(proc_environ.readlines("\0", chomp: true))
end

context 'when field exists' do
let(:proc_environ) { load_fixture('proc_environ_podman') }

it 'returns the field' do
result = Facter::Util::Linux::Proc.getenv_for_pid(1, 'container')
expect(result).to eq('podman')
end
end

context 'when field does not exist' do
let(:proc_environ) { load_fixture('proc_environ_podman') }

it 'returns nil' do
result = Facter::Util::Linux::Proc.getenv_for_pid(1, 'butter')
expect(result).to eq(nil)
end
end

context 'when field is empty' do
let(:proc_environ) { load_fixture('proc_environ_no_value') }

it 'returns an empty string' do
result = Facter::Util::Linux::Proc.getenv_for_pid(1, 'bubbles')
expect(result).to eq('')
end
end
end
end
Binary file added spec/fixtures/proc_environ_no_value
Binary file not shown.
Binary file added spec/fixtures/proc_environ_podman
Binary file not shown.