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

fix(multipath): enable service but disable socket #2290

Merged
merged 1 commit into from Apr 11, 2023
Merged

fix(multipath): enable service but disable socket #2290

merged 1 commit into from Apr 11, 2023

Conversation

cthbleachbit
Copy link
Contributor

Changes

This fixes a hang in the cleanup hook: running multipath binary triggers activation of multipathd.service after it is (and must remain) stopped as dracut prepares to switch root in initrd-cleanup.service.

An earlier fix from PR #2010 was based on an old systemd behavior where missing socket will prevent the service from being enabled. This does not seem to be the case anymore. For completeness, I'm disabling the socket alone after enabling service.

At the end of the day we should probably remove Also=multipathd.socket from multipathd.service and upsteam this change. The whole point of having a socket enabled separately from the service is that systemd can start the service as-needed. Binding enable/disable status together would not be very sane.

Checklist

  • I have tested it locally
  • I have reviewed and updated any documentation if relevant
  • I am providing new code and test(s) for it

Fixes #2289, #2175

@github-actions github-actions bot added modules Issue tracker for all modules multipath Issues related to the multipath module labels Mar 25, 2023
@mwilck
Copy link
Contributor

mwilck commented Mar 25, 2023

See opensvc/multipath-tools#65

I think we should get rid of the entire multipathd-needshutdown logic. It isn't needed any more.

@mwilck
Copy link
Contributor

mwilck commented Mar 25, 2023

I don't know why #2210 was made because I wasn't involved, and there was no bug report linked.
I suppose the issue was related to udev; when multipath is running from udev rules it might try to access the socket.
But the startup sequence for multipathd has been changed such that it starts before udev ist starting, so that should not be a problem any more.

I don't think we should rely on socket activation for multipathd in the initrd.

@mwilck
Copy link
Contributor

mwilck commented Mar 25, 2023

I suppose the issue was related to udev

That can't be the case; multipath -u and multipath -U as run from udev rules don't access the socket.
Wrong. multipath -u must access the socket because if multipathd isn't running, no path will be considered valid.

Another thing worth trying here would be to run multipath with the -D option. This avoids that multipath tries to connect to the multipathd socket.

@cthbleachbit
Copy link
Contributor Author

I don't think we should rely on socket activation for multipathd in the initrd.

Absolutely not. The only reason that the socket got involved is due to it declared in Also= from the service. This cause systemd to sync status to the socket unit when enabling/disabling the service. On a hindsight the service probablty should not even have this Also=multipathd.socket declared.

The -D option doesn't seem to be documented in the manual nor in the command help prints as of multipath-tools 0.9.3...

@mwilck
Copy link
Contributor

mwilck commented Mar 25, 2023

I don't think we should rely on socket activation for multipathd in the initrd.

Absolutely not. The only reason that the socket got involved is due to it declared in Also= from the service. This cause systemd to sync status to the socket unit when enabling/disabling the service. On a hindsight the service probablty should not even have this Also=multipathd.socket declared.

Well, as I said above, either multipathd must be running, or socket activation must be enabled, otherwise multipath activation in the initrd will fail. The best idea would be to start multipathd early in the intird.

The Also= line has been added 10y ago and never been questioned. You are right that Also= is wrong here: If multipathd is enabled, enabling the socket unit makes little sense. Likewise, the socket shouldn't be disabled just because someone disables the service. If socket activation is desired, the socket should be enabled and service should be disabled.

The -D option doesn't seem to be documented in the manual nor in the command help prints as of multipath-tools 0.9.3...

Right. It's an undocumented feature :-) . It isn't meant for end users. But it might be useful for dracut. It would not make a difference for multipath -u though (see above). I need to double-check this again.

@cthbleachbit
Copy link
Contributor Author

either multipathd must be running, or socket activation must be enabled

So dracut enables multipathd.service - fine on its own. Enabling service enables multipathd.socket as well because the service has Also=multipathd.socket.

@mwilck
Copy link
Contributor

mwilck commented Mar 25, 2023

either multipathd must be running, or socket activation must be enabled

So dracut enables multipathd.service - fine on its own. Enabling service enables multipathd.socket as well because the service has Also=multipathd.socket.

Understood. The Also= is wrong indeed.

mwilck added a commit to openSUSE/multipath-tools that referenced this pull request Mar 25, 2023
This "Also=" directive is wrong. It was meant for enabling socket activation,
but it actually does the opposite. "Also=multipathd.socket" means that
enabling/disabling the service will enable/disable the socket, too. This is
not what we want: socket activation means that we can enable the socket
while the service is disabled and will be activated by the socket on demand.

See dracutdevs/dracut#2290,
opensvc#65

Fixes: ca985df ("multipathd: switch to socket activation for systemd")
Signed-off-by: Martin Wilck <mwilck@suse.com>
@mwilck
Copy link
Contributor

mwilck commented Mar 25, 2023

Just sent a patch to dm-devel@redhat.com for the upstream service unit.

@mwilck
Copy link
Contributor

mwilck commented Mar 25, 2023

What I said about -D above was wrong, too, sorry.

-D only has an effect for map creation (multipath) or flushing (multipath -f $map, multiapth -F).
But the dracut scripts in question only run multipath -l, which doesn't connect to the socket.

So it remains unclear to me why the socket is activated.

@jiayi0118
Copy link

This patch works in my system.

LGTM

@aafeijoo-suse
Copy link
Member

IIUC, if Martin's patch removing the Also= from multipathd.service is accepted, I'd just simply revert 02e646f

@cthbleachbit
Copy link
Contributor Author

I'd guess that dracut needs to account for older versions of mp-tools.

Maybe I should just revert #2010 and remove Also= for the copy of multipathd.service in dracut's repo as well.

mwilck added a commit to openSUSE/multipath-tools that referenced this pull request Mar 28, 2023
This "Also=" directive is wrong. It was meant for enabling socket activation,
but it actually does the opposite. "Also=multipathd.socket" means that
enabling/disabling the service will enable/disable the socket, too. This is
not what we want: socket activation means that we can enable the socket
while the service is disabled and will be activated by the socket on demand.

See dracutdevs/dracut#2290,
opensvc#65

Fixes: ca985df ("multipathd: switch to socket activation for systemd")
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
mwilck added a commit to openSUSE/multipath-tools that referenced this pull request Mar 28, 2023
This "Also=" directive is wrong. It was meant for enabling socket activation,
but it actually does the opposite. "Also=multipathd.socket" means that
enabling/disabling the service will enable/disable the socket, too. This is
not what we want: socket activation means that we can enable the socket
while the service is disabled and will be activated by the socket on demand.

See dracutdevs/dracut#2290,
opensvc#65

Fixes: ca985df ("multipathd: switch to socket activation for systemd")
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
@mwilck
Copy link
Contributor

mwilck commented Apr 3, 2023

IIUC, if Martin's patch removing the Also= from multipathd.service is accepted, I'd just simply revert 02e646f

It has got positive review and is scheduled to be merged with opensvc/multipath-tools#64

This reverts commit e39ff40, removes
an incorrect `Also=` directive from multipathd.service.

`Also=multipathd.socket` is not the correct behavior for a
socket-activated service. This directive has been removed upstream
and dracut should do the same.

This fixes #2289, #2175 where in the cleanup hook running multipath
binary triggers activation of multipathd.service after it is stopped
as dracut prepares to switch root in initrd-cleanup.service.
@cthbleachbit
Copy link
Contributor Author

Following upstream. My old fix reverted and Also= line removed.

Copy link
Collaborator

@LaszloGombos LaszloGombos left a comment

Choose a reason for hiding this comment

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

LGTM

@LaszloGombos LaszloGombos added this to the dracut-060 milestone Apr 11, 2023
@aafeijoo-suse aafeijoo-suse merged commit 297525c into dracutdevs:master Apr 11, 2023
68 of 74 checks passed
@mwilck
Copy link
Contributor

mwilck commented Sep 21, 2023

FTR: I had a similar discussion about a different unit recently, shedding some more light on the issue.2

At the end of the day we should probably remove Also=multipathd.socket from multipathd.service and upsteam this change. The whole point of having a socket enabled separately from the service is that systemd can start the service as-needed. Binding enable/disable status together would not be very sane.

Using Also=$X.socket in a service unit $X makes sense if and only if the service unit has no [Install] section. In that case, systemctl enable $X.service will install the socket unit instead of the service unit, which makes sense for a daemon that has no purpose except serving the socket. But multipathd serves many more purposes, and therefore using Also=multipathd.socket is wrong.

@aafeijoo-suse
Copy link
Member

Using Also=$X.socket in a service unit $X makes sense if and only if the service unit has no [Install] section. In that case, systemctl enable $X.service will install the socket unit instead of the service unit, which makes sense for a daemon that has no purpose except serving the socket. But multipathd serves many more purposes, and therefore using Also=multipathd.socket is wrong.

Thanks for this background explanation Martin, sometimes systemd is too tricky...

mwilck added a commit to openSUSE/multipath-tools that referenced this pull request Feb 27, 2024
This "Also=" directive is wrong. It was meant for enabling socket activation,
but it actually does the opposite. "Also=multipathd.socket" means that
enabling/disabling the service will enable/disable the socket, too. This is
not what we want: socket activation means that we can enable the socket
while the service is disabled and will be activated by the socket on demand.

See dracutdevs/dracut#2290,
opensvc#65

Fixes: ca985df ("multipathd: switch to socket activation for systemd")
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
(cherry picked from commit 53b215e)
mwilck added a commit to openSUSE/multipath-tools that referenced this pull request Feb 27, 2024
This "Also=" directive is wrong. It was meant for enabling socket activation,
but it actually does the opposite. "Also=multipathd.socket" means that
enabling/disabling the service will enable/disable the socket, too. This is
not what we want: socket activation means that we can enable the socket
while the service is disabled and will be activated by the socket on demand.

See dracutdevs/dracut#2290,
opensvc#65

Fixes: ca985df ("multipathd: switch to socket activation for systemd")
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
(cherry picked from commit 53b215e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modules Issue tracker for all modules multipath Issues related to the multipath module regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

initrd is blocked because of conflict between multipathd.service and initrd-cleanup.service
5 participants