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.

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

Jump to bottom

MDEV-19210: use environment file in systemd units #2726

Merged
merged 4 commits into from
May 29, 2024
Merged

MDEV-19210: use environment file in systemd units #2726

merged 4 commits into from
May 29, 2024

Conversation

eworm-de
Copy link
Contributor

@eworm-de eworm-de commented Aug 10, 2023 •
edited

  • The Jira issue number for this PR is: MDEV-19210

Description

We used to run systemctl set-environment to pass _WSREP_START_POSITION. This is bad because:

  • it clutter systemd's environment (yes, pid 1)
  • it requires root privileges
  • options (like LimitNOFILE=) are not applied

Let's just create an environment file in ExecStartPre=, that is read before ExecStart= kicks in. We have _WSREP_START_POSITION around for the main process without any downsides.

How can this PR be tested?

Well, good question... This should not add new regressions and continue as expected.

Special care is to be taken for Debian, which adds extra commands to the systemd service file.

Basing the PR against the correct MariaDB version

  • This is a new feature and the PR is based against the latest MariaDB development branch.
  • This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

This was referenced Aug 10, 2023
Closed
Merged
@illuusio
Copy link
Contributor

Ok I like nice adjustements with + sigh for Debian but then I just like to ask why should _WSREP_START_POSITION be on systemd file? As most of the people does not need it (like me who don't use galera but likes to have normal replication)? It would be easier to manage to have this systemd privilege thing in another PR ja wsrep/replication in another.
Should this be worked to work on RPM side too?

@eworm-de
Copy link
Contributor Author

Ok I like nice adjustements with + sigh for Debian but then I just like to ask why should _WSREP_START_POSITION be on systemd file?

Currently the service file handles single server and Galera cluster. I do not change that, I just change the way to pass environment variables.

As most of the people does not need it (like me who don't use galera but likes to have normal replication)? It would be easier to manage to have this systemd privilege thing in another PR ja wsrep/replication in another.

This is not about adding functionality, this is just about changing an implementation detail.

Should this be worked to work on RPM side too?

Only if there is any mechanism that adds more ExecStartPre= or ExecStartPost= for RPM packages that needs root privileges. I do not think there is anything like that.

@LinuxJedi
Copy link
Contributor

I pushed this to a bb branch to run through old buildbot to make sure it doesn't fail, results: https://buildbot.mariadb.net/buildbot/grid?category=main&branch=bb-10.4-linuxjedi-MDEV-19210

@LinuxJedi LinuxJedi requested a review from sysprg August 11, 2023 08:29
@LinuxJedi
Copy link
Contributor

Requesting a review from @sysprg for this.

@eworm-de
Copy link
Contributor Author

Thanks a lot! Looks promising I think.

@eworm-de
Copy link
Contributor Author

Any news on this?

I could not find failures related to service startup. So should be ok now?

@eworm-de
Copy link
Contributor Author

eworm-de commented Sep 5, 2023

Ping... Any news or is this going to stall mode for years again?

@LinuxJedi
Copy link
Contributor

Sorry @eworm-de, I have on a combination of vacation and working on some other high-priority things. I'm starting to catch up.

Unfortunately I cannot merge this without approval from @sysprg, due to the ownership of the Galera section of the codebase. This message will have pinged him again.

@grooverdan grooverdan self-requested a review September 8, 2023 09:36
@grooverdan
Copy link
Member

pushed to bb-10.4-MDEV-19210-environment-file-pkgtest to create files for testing. A 11.3/11.4 might be a more realistic target.

@grooverdan grooverdan self-assigned this Oct 22, 2023
@eworm-de
Copy link
Contributor Author

Well, this issue is that old... I think I started with 10.2. 😜 That's why it is stuck at 10.4 right now.

But I am free to anything else... Let me know and I will change the target. On the other side - I would like to see it in a GA release any time soon.

@grooverdan grooverdan changed the base branch from 10.4 to 11.4 November 14, 2023 05:31
@eworm-de
Copy link
Contributor Author

Uh, 11.4 is a long way to go... Well, let's hope we will see some progress now at least.

@grooverdan
Copy link
Member

@eworm-de hi,

Started testing on https://github.com/MariaDB/mariadb.org-tools/blob/master/daniel/MDEV-19210 - so an alma linux

I just want to make sure I've covered main test cases:

  • galera_new_cluster
  • nodes can join
  • add data
  • disconnect node
  • add mode data
  • rejoin node performs SST and pulls recovery position

What else do I need to test? (I'll do the same of a few more distro types).

As you may not have expected on Alma/RHEL - datadir is @mysqlunixdir@. So do you think that needs a modification?
From https://github.com/devexp-db/mysql-selinux/blob/master/mysql.fc - /var/run/mariadb might be safer.

Aside: have you thought about https://systemd.io/FILE_DESCRIPTOR_STORE/ - would require much more server implementation.

grooverdan
grooverdan reviewed Nov 23, 2023
View reviewed changes
support-files/mariadb.service.in Outdated Show resolved Hide resolved
scripts/galera_new_cluster.sh Outdated Show resolved Hide resolved
support-files/mariadb.service.in Show resolved Hide resolved
@eworm-de
Copy link
Contributor Author

Rebased to drop last bits of support for instantiated services...

@eworm-de
Copy link
Contributor Author

Rebased to rename galera_new_cluster.sh to galera_new_cluster.sh.in as it is a template now.

@eworm-de eworm-de force-pushed the MDEV-19210-environment-file branch from 04e8035 to 3f365ad Compare March 18, 2024 11:31
@eworm-de eworm-de force-pushed the MDEV-19210-environment-file branch from 3f365ad to 9ae01fe Compare April 9, 2024 08:20
@eworm-de
Copy link
Contributor Author

The next and last version from 11.4 branch will be released in a few days? Guess this will not make it then?

Should I rebase on 11.5?

@LinuxJedi
Copy link
Contributor

The next and last version from 11.4 branch will be released in a few days? Guess this will not make it then?

Should I rebase on 11.5?

Hi @eworm-de, it will need rebasing against 11.5 or 11.6 (depending on which one it will land it). We can do this after review if it helps.

I'll take a look at the more recent changes shortly and see if I can push this along.

@eworm-de eworm-de force-pushed the MDEV-19210-environment-file branch from 9ae01fe to dc9fa6a Compare April 18, 2024 08:06
@CLAassistant
Copy link

CLAassistant commented Apr 18, 2024 •
edited

CLA assistant check
All committers have signed the CLA.

@eworm-de eworm-de changed the base branch from 11.4 to 11.5 April 18, 2024 08:06
@eworm-de eworm-de force-pushed the MDEV-19210-environment-file branch from dc9fa6a to cd37253 Compare April 18, 2024 08:09
@eworm-de
Copy link
Contributor Author

Rebased and switched to 11.5...

LinuxJedi
LinuxJedi approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@LinuxJedi LinuxJedi left a comment

Choose a reason for hiding this comment

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

Looks like everything is addressed and it looks like a nice improvement to me.

@LinuxJedi
Copy link
Contributor

Whilst I have approved this, the MTR is showing Galera failures in old Buildbot for the branch @grooverdan created for it (https://buildbot.mariadb.net/buildbot/grid?category=main&category=package&branch=bb-11.4-pr2726-MDEV-19210-environment-file-pkgtest). I don't know whether this is related or not. Someone with Galera knowledge would need to check.

@eworm-de eworm-de force-pushed the MDEV-19210-environment-file branch from cd37253 to e5f15a2 Compare May 15, 2024 14:05
@eworm-de
Copy link
Contributor Author

Any news on this? Perhaps pushing the current branch to the buildbot makes sense...

@LinuxJedi
Copy link
Contributor

Any news on this? Perhaps pushing the current branch to the buildbot makes sense...

Hi @eworm-de, 11.6 is open, so we will work towards getting it in there. I'll try to sort out a second reviewer for you.

@eworm-de
Copy link
Contributor Author

Oh, I had just rebased this on 11.5 a month ago... Should I switch and rebase on 11.6 now?

@LinuxJedi
Copy link
Contributor

Oh, I had just rebased this on 11.5 a month ago... Should I switch and rebase on 11.6 now?

Sorry, I was wrong, there isn't an 11.6 just yet, but it should exist any day now. It will need rebasing to that once it exists.

@eworm-de eworm-de force-pushed the MDEV-19210-environment-file branch from e5f15a2 to eeeb9d4 Compare May 27, 2024 13:29
@eworm-de eworm-de changed the base branch from 11.5 to 11.6 May 28, 2024 09:15
@eworm-de
MDEV-19210: use environment file in systemd units for _WSREP_START_PO…
834fdfe 
…SITION

We used to run `systemctl set-environment` to pass
_WSREP_START_POSITION. This is bad because:

* it clutter systemd's environment (yes, pid 1)
* it requires root privileges
* options (like LimitNOFILE=) are not applied

Let's just create an environment file in ExecStartPre=, that is read
before ExecStart= kicks in. We have _WSREP_START_POSITION around for the
main process without any downsides.
@eworm-de
MDEV-19210: update galera_new_cluster to use environment file
ca7a9b2 
Now that the systemd unit files use an environment file to pass
_WSREP_START_POSITION we have to update galera_new_cluster as well.
@eworm-de
MDEV-19210: drop support for instantiated services from galera_new_cl…
8bc7306 
…uster

Support in service file was dropped in 91f1694
(systemd: multi-instance not for Galera, User/Group flexible).
@eworm-de
MDEV-19210: do not run pre and post scripts as root
2f456af 
Now that we do not pollute systemd's environment but write private
environment files running these as root is not longer required. So
let's drop `PermissionsStartOnly=true`.

Debian adds extra `ExecStartPre=` and `ExecStartPost=`, though.
Use special executable prefix for full privileges there. (See
systemd.service(5) for details.)
@eworm-de eworm-de force-pushed the MDEV-19210-environment-file branch from eeeb9d4 to 2f456af Compare May 28, 2024 09:16
@eworm-de
Copy link
Contributor Author

Rebased on 11.6. Any chance for some progress now?

@LinuxJedi
Copy link
Contributor

Rebased on 11.6. Any chance for some progress now?

There is a test failing (even after retriggers) which I don't think is related to your work, but I physically cannot merge it until it passes. We will look into this.

@LinuxJedi LinuxJedi merged commit aeffec6 into MariaDB:11.6 May 29, 2024
16 of 18 checks passed
@LinuxJedi
Copy link
Contributor

All good, thanks again!

@grooverdan
Copy link
Member

🥳 🎈 . Thank you.

@eworm-de eworm-de deleted the MDEV-19210-environment-file branch June 11, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grooverdan grooverdan grooverdan approved these changes

@LinuxJedi LinuxJedi LinuxJedi approved these changes

@sysprg sysprg Awaiting requested review from sysprg

Assignees

@grooverdan grooverdan

Labels
None yet
Milestone
No milestone
5 participants
@eworm-de @illuusio @LinuxJedi @grooverdan @CLAassistant