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

(PUP-2153) Add support for FreeBSD profiles #8607

Closed
wants to merge 1 commit into from

Conversation

smortex
Copy link
Contributor

@smortex smortex commented May 21, 2021

When a FreeBSD rc-script has support for profiles, when running it with
the rcvar parameter the output is modified to list the profiles.

With a single profile:

root@localhost # /usr/local/etc/rc.d/fcgiwrap rcvar
===> fcgiwrap profile: nagios
# fcgiwrap
#
fcgiwrap_enable="YES"
#   (default: "")

With multiple profiles:

root@localhost # /usr/local/etc/rc.d/fcgiwrap rcvar
===> fcgiwrap profile: nagios1
# fcgiwrap
#
fcgiwrap_enable="YES"
#   (default: "")

===> fcgiwrap profile: nagios2
# fcgiwrap
#
fcgiwrap_enable="YES"
#   (default: "")

===> fcgiwrap profile: nagios3
# fcgiwrap
#
fcgiwrap_enable="YES"
#   (default: "")

Compare with the output for another service that has no support for
profiles:

root@localhost # /usr/local/etc/rc.d/nagios rcvar
# nagios
#
nagios_enable="YES"
#   (default: "")

Because the code assumes the rcvar is on a fixed line, the presence of
the ===> <service> profile: <profile> line breaks the parsing of the
output.

Adjust this by removing the profile information lines too.

When a FreeBSD rc-script has support for profiles, when running it with
the `rcvar` parameter the output is modified to list the profiles.

With a single profile:
```
root@localhost # /usr/local/etc/rc.d/fcgiwrap rcvar
===> fcgiwrap profile: nagios
# fcgiwrap
#
fcgiwrap_enable="YES"
#   (default: "")
```

With multiple profiles:
```
root@localhost # /usr/local/etc/rc.d/fcgiwrap rcvar
===> fcgiwrap profile: nagios1
# fcgiwrap
#
fcgiwrap_enable="YES"
#   (default: "")

===> fcgiwrap profile: nagios2
# fcgiwrap
#
fcgiwrap_enable="YES"
#   (default: "")

===> fcgiwrap profile: nagios3
# fcgiwrap
#
fcgiwrap_enable="YES"
#   (default: "")
```

Compare with the output for another service that has no support for
profiles:

```
root@localhost # /usr/local/etc/rc.d/nagios rcvar
# nagios
#
nagios_enable="YES"
#   (default: "")
```

Because the code assumes the rcvar is on a fixed line, the presence of
the `===> <service> profile: <profile>` line breaks the parsing of the
output.

Adjust this by removing the profile information lines too.
@smortex smortex requested review from a team May 21, 2021 05:27
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@smortex

This comment has been minimized.

@GabrielNagy
Copy link
Contributor

Thanks for the contribution @smortex! 🎉

Pasting from the ticket description, is this still a concern?

Adding rcvar.delete_if {\|str\| str =~ /^===*$/} will help a bit, but there is a case where it will get confused - if the first profile is set to not enabled in rc.conf.d/someservice file, regardless if any other is set to enabled, this function will determine that service should be disabled as it only checks the first "enable", and there will be multiple "enable" lines with multiple profiles. It should ideally check all occurrences of _enable and, if at least one is set to "YES", enable the entire thing and let the
actual start script handle what to start and what not to start. Or maybe it should just skip the entire process if rc.conf.d file is already a puppet managed resource? But it doesn't have to be.

Can you also add a spec test to reflect this behavior? I'm thinking somewhere here: https://github.com/puppetlabs/puppet/blob/main/spec/unit/provider/service/freebsd_spec.rb#L28

Also, if you'd like this to get into puppet 6 as well, please rebase and retarget the PR to the 6.x branch.

@smortex
Copy link
Contributor Author

smortex commented May 25, 2021

Hum, I forgot about this and in my testing I removed unused profile instead of keeping them and disabling them.

The profile management code is not a feature provided by the rc subsystem and used by some services, but directly something implemented into some services (with slight differences). The general logic is:

if a profile is provided
  manage the provided profile
else
  for each configured profile
    run the current command with the current profile

Also, some services have an optional support for profiles (e.g. postgresql)… A trivial one-size-fit-all solution seems complicated to find.

However, detecting if a rc-script is using profiles or not seems to be feasible.
A solution could be to can add an optional profile parameter to the service type:

  • if the service is not using profiles and no profile is set we can continue as usual;
  • if the service is using profiles and no profile is set we can fail;
  • if the service is not using profiles and a profile is set we can fail;
  • if the service is using profiles and a profile is set we can manage just this profile.

e.g.

service { 'pounce-freenode':
  ensure  => stopped,
  enable  => false,
  name    => 'pounce',
  profile => 'freenode',
}
service { 'pounce-libera':
  ensure  => running,
  enable  => true,
  name    => 'pounce',
  profile => 'libera',
}
service { 'pounce-tilde':
  ensure  => running,
  enable  => true,
  name    => 'pounce',
  profile => 'tilde',
}

It would also make sense to manage the profiles from puppet too (with a feature to add manage custom variable specific to a service):

service { 'pounce':
  vars => {
    profiles => 'freenode libera tilde',
  }
}

I can for sure work on this if it makes sense. @GabrielNagy can you please tell me what you think about this?

@GabrielNagy
Copy link
Contributor

Having a new profile or profiles param would be the easier way, and we can have the value be either an array (of profiles) or a string.

However this would be a FreeBSD-only feature, which is why the vars approach might make more sense, similar to the :package_settings property from the package type (which is, interestingly, not implemented by any provider). This way more providers can implement the feature if need be. On the other hand, we don't have the bandwith for investigating possible use cases of a new parameter like vars, so we might end up having a generalized parameter that will only ever be implemented by the FreeBSD provider.

Either way I think we'd end up with a new feature in the type that will only be implemented by FreeBSD for now, so maybe it would be best to go with the first approach and implement a top-level profile parameter?

  • if the service is using profiles and no profile is set we can fail;

In this case shouldn't it start all profiles?

@smortex
Copy link
Contributor Author

smortex commented May 26, 2021

I've been thinking about this and come to the conclusion that a single vars allows to setup all profiles with a single resource instead of four:

service { 'pounce':
  ensure => running,
  enable => true,
  vars   => {
    profiles        => 'freenode libera tilde',
    freenode_enable => 'NO',
    freenode_user   => 'romain',
    libera_user     => 'romain',
    tilde_user      => 'romain',
  }
}

This should produce the following config:

pounce_enable="YES"
pounce_profiles="freenode libera tilde"
pounce_freenode_enable="NO"
pounce_freenode_user="romain"
pounce_libera_user="romain"
pounce_tilde_user="romain"

and ensure the two enabled profiles are running and the third one is stopped.

As a bonus, existing services which are a pain to configure on FreeBSD because the service provider cannot manage all custom variable they use could be enhanced…

The code for managing old releases of FreeBSD might make things a bit funny, but I'll start with these requirements and try to do something with them. I'll update this PR when I am happy with the results. Thanks!

@GabrielNagy
Copy link
Contributor

Sounds good, I didn't know multiple variables/settings could be managed per service, so the vars approach seems like a huge improvement in that case!

@CLAassistant
Copy link

CLAassistant commented Sep 1, 2021

CLA assistant check
All committers have signed the CLA.

@joshcooper joshcooper added the enhancement New feature or request label Nov 15, 2023
@joshcooper
Copy link
Contributor

@smortex checking to see if this something you're still wanting to work on as there are some merge conflicts.

@smortex
Copy link
Contributor Author

smortex commented Mar 13, 2024

Hum, this is somewhat a niche topic, and while I hit this in 2021, I have lived without support for it since then and am not maintaining local patches so it can probably be ignored for now.

@smortex smortex closed this Mar 13, 2024
@rvstaveren
Copy link

Oh that is too bad. I'm monkey patching some of my rc.d files (uwsgi e.g.) to let puppet's service class work with them. I will have a look then

@smortex
Copy link
Contributor Author

smortex commented Mar 15, 2024

@rvstaveren oh! According to the examples above, I looked into this when using irc/pounce at the time freenode exploded and wanted to have that old config around just in case. It looks like there are much more ports that have support for profiles today, a quick grep in /usr/local/etc/rc.d shows www/apache24, dns/dnsdist, mail/opendkim, databases/postgresql*-server, dns/unbound on one of my nodes.

The root issue is that when a profile is stopped, it prevents other profiles from being visible, e.g.

romain@agrajag /usr/local/etc/rc.d % sudo service pounce status
===> pounce profile: debian
pounce is running as pid 66374.
===> pounce profile: libera
pounce is running as pid 66384.
===> pounce profile: tilde
pounce is running as pid 66394.
romain@agrajag /usr/local/etc/rc.d % sudo service pounce stop debian
Stopping pounce.
Waiting for PIDS: 66374.
romain@agrajag /usr/local/etc/rc.d % sudo service pounce status     
===> pounce profile: debian
pounce is not running.
romain@agrajag /usr/local/etc/rc.d (1) % sudo service pounce start debian
Starting pounce.
romain@agrajag /usr/local/etc/rc.d % sudo service pounce status      
===> pounce profile: debian
pounce is running as pid 89668.
===> pounce profile: libera
pounce is running as pid 66384.
===> pounce profile: tilde
pounce is running as pid 66394.
romain@agrajag /usr/local/etc/rc.d % 

After stopping the "debian" profile, status show this profile as not running and does not list the other two which are still running.

Some work on the FreeBSD side seems required, and then work on the Puppet side must be done to cope with this. Given the number of services having profiles today, maybe it makes sense? What is your "monkey patching" doing? Does it work around the above issue? Maybe creating a review on FreeBSD's fabricator would make sense to review it and integrate it?

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

Successfully merging this pull request may close these issues.

None yet

7 participants