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

tuned: Rework logic to gracefully handle the service starting #20748

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SludgeGirl
Copy link
Contributor

Previously starting the service would lead to a server disconnected message, these changes allow us to instead fully wait until the service has started before showing the profile selection, meaning we no longer encounter that error

@SludgeGirl SludgeGirl marked this pull request as draft July 12, 2024 07:14
Previously starting the service would lead to a server disconnected
message, these changes allow us to instead fully wait until the
service has started before showing the profile selection, meaning
we no longer encounter that error
@SludgeGirl SludgeGirl force-pushed the fix-tuned-server-disconnected-error branch from de9ab3e to 466b3dc Compare July 12, 2024 07:39
@SludgeGirl SludgeGirl marked this pull request as ready for review July 12, 2024 07:40
@martinpitt
Copy link
Member

Hello @SludgeGirl !

Previously starting the service would lead to a server disconnected message

This sounds like a bug in tuned? The card already explicitly starts tuned.service before trying to talk to it. Once the service is running, it must be ready to accept D-Bus calls, otherwise there's nothing (other than repeatedly trying) that can be done on the caller side. But "server disconnected" often happens when the service crashes. Do you have some details about that? Can that be observed with running the tests against the tumbleweed image?

This PR introduces a lot of extra UI (a whole dialog, etc) but doesn't really address the race other than by "slowing down" the user.

I'd like to first understand what's going on. Thanks!

@martinpitt martinpitt added the question Further information is requested label Jul 15, 2024
@SludgeGirl
Copy link
Contributor Author

Hey @martinpitt!!

Alright so, I've had another dig through just to refresh myself (sorry for the wait). There's a couple things this aimed to handle

First is that the current behavior feels undefined and unexpected. From everything I've seen of cockpit so far, it's very explicit and asks permission for every action it wants to do that's extra from the intended user action, this is a stance I have alot of respect for. Clicking "performance: None" isn't clear that it would start tuned. The extra dialog I've added aimed to make this clearer along with helping to handle the issue (Sorry I forgot to make this clear in my initial explanation)

In terms of the technical issue, let me prefix this by saying please let me know if anything I say doesn't make sense or is wrong. I've done my best here but with all the react state changes, systemd service status changes and dbus then everything changing alot through it's execution, it's been a nightmare to try and get my head around.

But I believe as this executes and we start tuned inside the dialog, the some of the references like poll, and tunedDbus don't get updated inside the dialog because it's not re-rendered:

const TunedDialog = ({
updateButton,
poll,
tunedDbus,
tunedService,
}) => {

So when we then go to call those once the service.start promise returns we fail and return back the error because the dbus object is disconnected but is still used in poll during that point of the execution:

tunedService.start()
.then(updateButton)
.then(withTuned)
.catch(setError)
.finally(() => setLoading(false));

@jelly
Copy link
Member

jelly commented Oct 1, 2024

I can reproduce the issue just starting a Fedora 40 VM.

Initially the profile is shown as none and then clicking on it opens the tuned dialog which shows:

image

And then the tuned profile changes to virtual-guest

image

And clicking that:

image

@jelly
Copy link
Member

jelly commented Oct 1, 2024

The problem seems to be that tuned just takes too long these days to start, times out and reports an error. So I guess we have two options:

  • increase our dbus timeout?
  • retry one or two times?

@martinpitt alternative ideas welcome :)

@mvollmer
Copy link
Member

The problem seems to be that tuned just takes too long these days to start

I don't believe that.

@mvollmer
Copy link
Member

So, yeah, what a mess. :-)

I think what @SludgeGirl says is right: the DBusClient in the TunedDialog component is stale when the service is started as part of opening the dialog.

Also, the state handling in tuned-dialog.jsx as a whole is needlessly obfuscated. I would recommend making one object with all the state ("TunedState") that exposes the necessary stuff to fill the UI, and emit a "changed" signal when the UI needs updating. It is almost always a mistake to try to get sensible state management out of a couple of spaghetti useEffect calls. It's almost like programming in Prolog! :-) This PR here is also too complex when it comes to showing two dialogs in sequence, IMO.

@mvollmer
Copy link
Member

What to do about starting tuned.service is another thing... I think I agree that we should be more explicit about it. The proper state of affairs seems to be that if you have tuned installed, it should be running. If you don't want any tuning, switch it off with its own configuration.

@mvollmer
Copy link
Member

Alright, I dug into this and my first try was to get the opening/closing of the DBus connection sorted out: #21114. Then I remembered that we don't need to reopen DBusClients when their service restarts. So I tried to keep the DBusClient open at all times so that it never gets stale: #21115. Both tries seem to be successful. What do you guys think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants