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

Support Debian 11 #415

Closed
wants to merge 1 commit into from
Closed

Conversation

root-expert
Copy link
Member

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

@root-expert root-expert added the enhancement New feature or request label Aug 29, 2021
@root-expert root-expert marked this pull request as draft August 29, 2021 18:05
@ekohl
Copy link
Member

ekohl commented Aug 30, 2021

Looks like it fails on installing bolt because the package is not available. I couldn't find an issue on https://tickets.puppet.com but that may just be my limited JIRA search skills.

@root-expert
Copy link
Member Author

Looks like it fails on installing bolt because the package is not available. I couldn't find an issue on https://tickets.puppet.com but that may just be my limited JIRA search skills.

Yea, there isn't a Puppet Server package as well. Let's give them some time 😄

Signed-off-by: Christos Papageorgiou <christos.papageorgioy@gmail.com>
@@ -16,13 +16,19 @@
$pid_file = '/var/run/redis/redis-server.pid'
$workdir = '/var/lib/redis'
$bin_path = '/usr/bin'
$daemonize = true
$daemonize = $facts['os']['distro']['codename'] ? {
Copy link
Member

Choose a reason for hiding this comment

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

I'm generally not a fan of using the codename fact. It's generally not forward compatible. Can we deal with this similar to $sentinel_pid_file below?

Comment on lines +28 to +31
$sentinel_daemonize = $facts['os']['distro']['codename'] ? {
'bullseye' => false,
default => true,
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$sentinel_daemonize = $facts['os']['distro']['codename'] ? {
'bullseye' => false,
default => true,
}
$sentinel_daemonize = $daemonize,

@root-expert
Copy link
Member Author

The problem with this PR seems to be deeper, that's why I haven't progress it further.

For example daemonize = yes doesn't play well with systemd Type=notify which makes sense. I'm really wondering how this is working for other OSes.

Setting Type=forking makes test to pass. Don't if we should change this for the rest of the OSes or make Type field configurable by the user. That way they can select their Type/daemonize combination that fits their needs.

@ekohl
Copy link
Member

ekohl commented Oct 21, 2021

My ideal goal for this module was to leverage instances of redis-server@.service which is shipped on Debian & Ubuntu. For example on Ubuntu 18.04:

# dpkg -L redis-server | grep .service
/lib/systemd/system/redis-server.service
/lib/systemd/system/redis-server@.service

It's not shipped on Debian 9, but we can drop Debian 9 support now that it's only maintained as LTS (as opposed to the Debian project itself).

Then we would get out of the business of managing systemd files. An additional benefit is that you would get the security hardening that's in place.

Sadly on Red Hat-based distros this isn't shipped. That can be solved by shipping a redis@.service just for EL though.

The migration may also be complex so I never got around to it, but that's the vision I had. We should probably write it down in an issue.

@root-expert
Copy link
Member Author

I don't see those systemd files shipped on my Ubuntu 20.04 boxes.

Also dunno how flexible is that when you want to run multiple redis servers on the same box.
PR #424 low key adds supports to it by writing lock and PID files to separate directories I think.

@ekohl
Copy link
Member

ekohl commented Oct 21, 2021

I don't see those systemd files shipped on my Ubuntu 20.04 boxes.

That's odd because I just installed redis-server on an Ubuntu 20.04 container and they were there.

@kenyon
Copy link
Member

kenyon commented Oct 21, 2021

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

3 participants