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

systemd: allow only a single daemon-reload at the same time #6243

Closed
wants to merge 5 commits into from

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Nov 29, 2018

This is an experiment to see if the "mount protocol error" reported in systemd/systemd#10872 can be worked around by serializing the mount unit adding/removal. Proposing to get full spread runs.

@mvo5
Copy link
Contributor Author

mvo5 commented Nov 30, 2018

This was green in https://travis-ci.org/snapcore/snapd/jobs/461360585

Co-Authored-By: mvo5 <michael.vogt@gmail.com>
@mvo5
Copy link
Contributor Author

mvo5 commented Nov 30, 2018

And green again

@bboozzoo
Copy link
Collaborator

Restarted once more.

@mvo5
Copy link
Contributor Author

mvo5 commented Nov 30, 2018

I just ran this on google:ubuntu-18.04-64 again and it passed there too.

@zyga
Copy link
Collaborator

zyga commented Nov 30, 2018

I'm very optimistic about this :)

@mvo5
Copy link
Contributor Author

mvo5 commented Nov 30, 2018

Run it yet again in google:ubuntu-18.04-64 - still no error. I guess we can start thinking about how we can do something like this the right way(tm) now so that we can integrate it proper.

@zyga
Copy link
Collaborator

zyga commented Dec 4, 2018

I think this is good to go. We want to have this, either in the current form or in some changed form down the line. LGTM

Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

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

Yay! One suggestion to consider before landing, otherwise looks good!

@@ -32,7 +33,12 @@ import (
"github.com/snapcore/snapd/systemd"
)

var GML = &sync.Mutex{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a comment describing the problem it solves?

@mvo5 mvo5 removed the ⛔ Blocked label Dec 12, 2018
This works around the systemd bug
systemd/systemd#10872 by ensuring that
there is only a single operation that manipulates mount units
at the same time.
// mountLock ensures there is only a single concurrent mountunit
// operation. This works around this systemd bug:
// https://github.com/systemd/systemd/issues/10872
mountLock sync.Mutex
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think go vet explodes on this, we really need to pass Backend around as a pointer with this change

@mvo5 mvo5 modified the milestones: 2.36, 2.37 Dec 13, 2018
@mvo5
Copy link
Contributor Author

mvo5 commented Dec 13, 2018

Needs more work as Samuele pointed out that we may need to disallow all daemon-reloads - its not clear if the nature of the bug is that there can be no extra mount units added (but multiple daemon-reloads are ok) or if when a mount unit is added only a single daemon reload is ok.

@pedronis
Copy link
Collaborator

pedronis commented Dec 13, 2018

This was the discussion @mvo5 mentioned:

<pedronis> mvo: my main comment there, thinking a bit further, is that the description is misleading, we do daemon-reload in other places?
<pedronis> mvo: if to fix it we really need to serialize daemon-reload then that is not enough, no?
<mvo> pedronis: let me see, I think you are right
<pedronis> so it would need more work, and doesn't feel a .3 thing
<mvo> pedronis: hm, in the overlord we really only do the daemon reload there afaict - but yeah, lets push it out
<pedronis> let me double check
<pedronis> mvo: we do reloads in wrappers which is used indirectly by overload for example
<pedronis> sorry, overlord
<mvo> pedronis: aha, its the only explicit place. indeed
<pedronis> also interfaces systemd backend does it
<pedronis> let me put this chat in the PR

@mvo5
Copy link
Contributor Author

mvo5 commented Jan 7, 2019

Closing this one as the rabbit hole is deeper and we need to ensure we don't do any daemon reloads in parallel with writing mount units. I will push a new PR soon.

@mvo5 mvo5 closed this Jan 7, 2019
mvo5 added a commit to mvo5/snappy that referenced this pull request Jan 7, 2019
This is an RFC PR to see if the "mount protocol error" reported in
systemd/systemd#10872 can be worked around by serializing the mount unit
adding/removal. Proposing to get full spread runs.

This is similar to snapcore#6243 but it goes further by ensuring a single daemon
reload on the systemd go package level. Note that there is still a
chance that the protocol error happens if something else (like dpkg or
the user) runs "systemd daemon-reload" while we write a mount unit.
But the risk should be hughely smaller.
mvo5 added a commit to mvo5/snappy that referenced this pull request Jan 31, 2019
This is an RFC PR to see if the "mount protocol error" reported in
systemd/systemd#10872 can be worked around by serializing the mount unit
adding/removal. Proposing to get full spread runs.

This is similar to snapcore#6243 but it goes further by ensuring a single daemon
reload on the systemd go package level. Note that there is still a
chance that the protocol error happens if something else (like dpkg or
the user) runs "systemd daemon-reload" while we write a mount unit.
But the risk should be hughely smaller.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants