Skip to content

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented Oct 6, 2025

This builds on the previous work to define all paths in the RunMode class and has a few interesting aspects.

The parts that make it Linux specific is that it prefers systemd environment variables to determine locations. When those aren't available, it instead uses hardcoded paths for root and follows the XDG specification for non-root.

This is a draft because it's more of a proposal. It has some implications that may or may not be good. For example, is it a good thing for all of them to be changeable through env vars? It can change the behavior on whether it's a service or a CLI tool.

It's porting over the work I started in puppetlabs/puppet#8636. It's simpler because I no longer rely on install.rb in Fedora's packaging that I'm working on.

ekohl added 2 commits October 7, 2025 00:01
This builds on the previous work to define all paths in the RunMode
class and has a few interesting aspects.

The parts that make it Linux specific is that it prefers systemd
environment variables to determine locations. When those aren't
available, it instead uses hardcoded paths for root and follows the XDG
specification for non-root.

Currently this mode is not used yet, but by having it in the codebase
distribution packagers can easily use it without having to change all
paths.

Link: https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#RuntimeDirectory=
Link: https://specifications.freedesktop.org/basedir-spec/latest/
Copy link
Contributor Author

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

@anarcat and @smortex were involved in the original PR and may have opinions.

private

def packaging_name
'puppet'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to make it easy to switch to OpenVox paths. Perhaps it should be defined in the base class so all packaging can be switched over in 1 go.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for having it a single time.

end

def code_dir
File.join(conf_dir, 'code')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense for non-root users to ignore the /etc code directory?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so: when compiling catalogs, doesn't the ccode run as the puppet user?

Copy link

Choose a reason for hiding this comment

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

perhaps .config should be prioritized over /etc, at least when running as a non-root user, though? is that the case here?

at least that's the way it works now, i think...

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 so: when compiling catalogs, doesn't the ccode run as the puppet user?

No, the agent runs as root.

perhaps .config should be prioritized over /etc, at least when running as a non-root user, though? is that the case here?

That should indeed be the case, but today if you install OpenVox AIO packages you get an empty configuration file (only comments) so the built in defaults matter.

#
# @see https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#RuntimeDirectory=
# @see https://specifications.freedesktop.org/basedir-spec/latest/
class LinuxRunMode < RunMode
Copy link
Member

Choose a reason for hiding this comment

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

The UnixRunMode is puppetlabs-centric (the WindowsRunMode too). I wonder if we should rather make a hierarchy of classes where we override functions as needed.

A SystemdRunMode would basically be a specialization of a LinuxRunMode class, where functions call super unless the ad-hoc env variable is set.

I also think that the AIO RunMode should be a specialization a of more generic class rather than a base class like it is today, basically some tree of classes like:

  • RunMode
    • PosixRunMode
      • FreebsdRunMode
      • LinuxRunMode
        • AIOLinuxRunMode (what is called UnixRunMode today)
        • SystemdRunMode
    • WindowsRunMode

I am not sure about what levels of hierarchy are required, I put FreeBSD there because I know I would benefit from it. If it help, here is the special config we set in the generated puppet.conf on FreeBSD so that the various bits are in what we consider the "right" place (where $ETCDIR is /usr/local/etc/puppet): https://github.com/freebsd/freebsd-ports/blob/main/sysutils/openvox-agent8/Makefile#L116

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 was indeed thinking we need something like that hierarchy. I think the splitting up of LinuxRunMode may not be needed at all because I'd suggest to drop the specific AIO paths. Only use normal paths. The application code could still be shipped in /opt if desired.

Copy link

Choose a reason for hiding this comment

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

hmm... i'm not familiar with this, but we have UnixRunMode up there which we seemed to be using before this patch, what is it if not PosixRunMode here?

or, to put this another way, why don't we treat all UnixRunMode as following the XDG standard? i think it's mostly compatible with FreeBSD, for example, except for the /usr/local bits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is XDG a POSIX standard? I don't mind applying that to all platforms.

But the /usr/local bit vs / is one I have a hard time unifying.

Copy link

Choose a reason for hiding this comment

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

XDG is, obviously, not in POSIX.

but then if you derive Linux from POSIX, you're kind of kidding yourself as well... Linux is not, strictly speaking, POSIX either: https://en.wikipedia.org/wiki/POSIX#POSIX-oriented_operating_systems

but we're splitting hairs here: i was mostly wondering where the UnixRunMode fits in your class hierarchy there, i overlooked the AIOLinuxRunMode bits there...

Copy link
Member

Choose a reason for hiding this comment

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

I agree that XDG can be handled quite low in the hierarchy: it is indeed available on FreeBSD.

end

def code_dir
File.join(conf_dir, 'code')
Copy link
Member

Choose a reason for hiding this comment

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

I don't think so: when compiling catalogs, doesn't the ccode run as the puppet user?

private

def packaging_name
'puppet'
Copy link
Member

Choose a reason for hiding this comment

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

+1 for having it a single time.

Comment on lines 16 to 22
if Puppet::Util::Platform.windows?
@run_modes[name] ||= WindowsRunMode.new(name)
elsif Puppet::Util::Platform.linux?
@run_modes[name] ||= LinuxRunMode.new(name)
else
@run_modes[name] ||= UnixRunMode.new(name)
end
Copy link
Member

Choose a reason for hiding this comment

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

Multiple run modes will require further logic here de determine which one to prefer. For some operating systems, a simple test of the os is enough, but for Linux it will probably need something smarted, e.g. detect if AIO or not (to run in AIO RunMode), test if parent PID is 1 or not (for SystemdRunMode), etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be more of an OpenVox 9 question, but do we continue to push the AIO model with paths in /opt at all? If so, should the patching patch this class to achieve that instead?

@anarcat
Copy link

anarcat commented Oct 7, 2025

for what it is worth, i have no recollection of the original PR nor do i quite understand what this is about. :)

@ekohl
Copy link
Contributor Author

ekohl commented Oct 7, 2025

for what it is worth, i have no recollection of the original PR nor do i quite understand what this is about. :)

The intention is that we agree on how OpenVox is packaged on systems. That way both Debian and Fedora behave the same and ideally don't need patches. If any, very tiny ones.

Copy link

@anarcat anarcat left a comment

Choose a reason for hiding this comment

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

i see. then i think this looks good to me, in general... as i said in the original PR (apparently!) it seems generally sane to me...

end

def code_dir
File.join(conf_dir, 'code')
Copy link

Choose a reason for hiding this comment

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

perhaps .config should be prioritized over /etc, at least when running as a non-root user, though? is that the case here?

at least that's the way it works now, i think...

#
# @see https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#RuntimeDirectory=
# @see https://specifications.freedesktop.org/basedir-spec/latest/
class LinuxRunMode < RunMode
Copy link

Choose a reason for hiding this comment

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

hmm... i'm not familiar with this, but we have UnixRunMode up there which we seemed to be using before this patch, what is it if not PosixRunMode here?

or, to put this another way, why don't we treat all UnixRunMode as following the XDG standard? i think it's mostly compatible with FreeBSD, for example, except for the /usr/local bits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants