Skip to content

Update init.rb to support Nvidia's Cumulus Linux #9453

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update init.rb to support Nvidia's Cumulus Linux #9453

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lib/puppet/provider/service/init.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def self.defpath
end

# Debian and Ubuntu should use the Debian provider.
confine :false => %w[Debian Ubuntu].include?(Puppet.runtime[:facter].value('os.name'))
confine :false => %w[Debian Ubuntu Cumulus].include?(Puppet.runtime[:facter].value('os.name'))
Copy link
Contributor

@joshcooper joshcooper Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We already set systemd as the default provider in

puppet/lib/puppet/provider/service/systemd.rb

Line 33 in dc0e3ba

defaultfor 'os.name' => :cumuluslinux, 'os.release.major' => %w[3 4]
so curious why we need to "exclude" init? Is the os.name incorrect in the systemd provider?

Copy link
Author

@dpkgbofh dpkgbofh Sep 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is what is what facter returns on Cumulus:

root@edd-sw30:~# facter -p os.name
Cumulus
root@edd-sw30:~# facter -p os.release.major
12
root@edd-sw30:~#

The exclusion solution was suggested somewhere on serverfault and is working for me.

Copy link
Contributor

@joshcooper joshcooper Sep 9, 2024

Choose a reason for hiding this comment

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

The exclusion solution was suggested somewhere on serverfault

The pattern we've been following is to specify systemd as the default for a given platform and then exclude older versions in situations where the platform uses init or something else. For example, we do this for debian

puppet/lib/puppet/provider/service/systemd.rb

Lines 27 to 28 in dc0e3ba

defaultfor 'os.name' => :debian
notdefaultfor 'os.name' => :debian, 'os.release.major' => %w[5 6 7] # These are using the "debian" method

This way it's future compatible (so long as systemd remains the default) and in theory it helps when documenting which providers are enabled by default on each platform. If we just exclude Cumulus from init, it's not clear what provider is "next" precedence-wise.

I think the root issue is this line is expecting the os.name fact to return cumuluslinux (case-insensitively)

puppet/lib/puppet/provider/service/systemd.rb

Line 33 in dc0e3ba

defaultfor 'os.name' => :cumuluslinux, 'os.release.major' => %w[3 4]

Maybe it did that in previous versions of Cumulus? Or maybe facter 4 is reporting the os.name fact differently on Cumulus? What is the output of facter os? Could you try installing puppet-agent 6 on Cumulus and see what facter os reports?

Related commits

15ed9d1
686d5ca
7ba4df5

Copy link
Author

@dpkgbofh dpkgbofh Sep 10, 2024

Choose a reason for hiding this comment

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

root@edd-sw30:~# puppet --version
7.23.0

root@edd-sw30:~# facter os
{
architecture => "amd64",
distro => {
codename => "cumulus",
description => "Cumulus Linux",
id => "Cumulus-linux",
release => {
full => "12.5",
major => "12",
minor => "5"
}
},
family => "Debian",
hardware => "x86_64",
name => "Cumulus",
release => {
full => "12.5",
major => "12",
minor => "5"
},
selinux => {
enabled => false
}
}

Copy link
Contributor

@joshcooper joshcooper Sep 10, 2024

Choose a reason for hiding this comment

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

Thanks, the root cause is because facter 3 used to report os.name => "CumulusLinux"

So systemd would be selected as the default due to

puppet/lib/puppet/provider/service/systemd.rb

Line 33 in dc0e3ba

defaultfor 'os.name' => :cumuluslinux, 'os.release.major' => %w[3 4]

However, facter 4 reports os.name => "Cumulus", hence this issue. To ensure compatibility with facter 3 and 4, could you add the following to the systemd provider:

defaultfor 'os.name' => :cumulus, 'os.release.major' => %w[5]

I'm assuming cumulus 3 and 4 are EOL?

Copy link
Author

@dpkgbofh dpkgbofh Sep 11, 2024

Choose a reason for hiding this comment

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

Thanks Josh,
agreed!

Copy link
Contributor

@joshcooper joshcooper Sep 16, 2024

Choose a reason for hiding this comment

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

@dpkgbofh I unresolved this based on my comment:

To ensure compatibility with facter 3 and 4, could you add the following to the systemd provider

defaultfor 'os.name' => :cumulus, 'os.release.major' => %w[5]

At which point, I don't think the change to the init provider is needed?

# RedHat systems should use the RedHat provider.
confine :false => Puppet.runtime[:facter].value('os.family') == 'RedHat'

Expand Down