systemd: Don't open and close tuned DBus connections asynchronously #21114

mvollmer · 2024-10-14T11:17:59Z

The TunedPerformanceProfile component was keeping a DBus connection open to tuned, and was closing and reopening it whenever something might have changed that requires it.

This happened asynchronously with the rest of the code that was using this DBus connection, and would frequently cause that code to fail with "disconnected" errors.

Also, the TunedDialog was itself causing changes that require a new DBus connection (namely starting the tuned.service), but was always working with the one that was originally passed into it via its properties.

Let's straighten this all out:

Polling for the button state will open its own dedicated DBUs connection and close it when polling is done. There is no need to keep a DBus connection open for the whole Cockpit session.
The tuned.service is started when the dialog is opened and a DBus
connection is opened explicitly for the dialog after that and
closed when the dialog is closed.

pkg/systemd/overview-cards/tuned-dialog.jsx

mvollmer · 2024-10-14T11:20:45Z

Starting the service might be slow and it would be best to do it inside the dialog, while the spinner is showing. That is probably straightforward.

Also, I think our DBusClient is supposed to survive the dbus service coming and going, maybe with some flags and owner watching. That would be another, even cleaner, way of handling this, by only having a single DBusClient that is never closed and always works.

mvollmer · 2024-10-14T11:24:16Z

by only having a single DBusClient that is never closed and always works.

(except when superuser.allowed changes)

jelly

Tested locally, the dialog loads a lot faster!

jelly · 2024-10-14T11:25:55Z

pkg/systemd/overview-cards/tuned-dialog.jsx

@@ -303,14 +300,14 @@ const TunedDialog = ({
               className="ct-m-stretch-body"
               isOpen
               help={help}
-               onClose={Dialogs.close}
+               onClose={() => dialogResult.resolve()}


The equivalent would be dialogResult.resolve

Not necessarily. The this inside the call is different, and that might matter.

mvollmer · 2024-10-14T12:24:28Z

Starting the service might be slow and it would be best to do it inside the dialog, while the spinner is showing. That is probably straightforward.

Also, I think our DBusClient is supposed to survive the dbus service coming and going, maybe with some flags and owner watching. That would be another, even cleaner, way of handling this, by only having a single DBusClient that is never closed and always works.

Done in #21115.

mvollmer · 2024-10-14T12:50:48Z

Tested locally, the dialog loads a lot faster!

This is due to this: https://github.com/cockpit-project/cockpit/pull/21114/files#diff-edcd27aeefd91df68948709cd630aaefbc569ac0674cc54693139880404e17bdR101-R108

mvollmer · 2024-10-15T09:05:20Z

Starting the service might be slow and it would be best to do it inside the dialog, while the spinner is showing. That is probably straightforward.

@jelly, what do you think about this? Should we start tuned.service while the dialog is already open? Then the TunedDialog component needs to maintain its own DBusClient ins a useState hook, probably.

jelly · 2024-10-16T07:23:46Z

Starting the service might be slow and it would be best to do it inside the dialog, while the spinner is showing. That is probably straightforward.

@jelly, what do you think about this? Should we start tuned.service while the dialog is already open? Then the TunedDialog component needs to maintain its own DBusClient ins a useState hook, probably.

Hmmm right, what if it fails the error message get's "lost", so maybe we should do the extra work (and right thing) And start the service in the dialog?

mvollmer · 2024-10-22T08:34:54Z

Starting the service might be slow and it would be best to do it inside the dialog, while the spinner is showing. That is probably straightforward.

@jelly, what do you think about this? Should we start tuned.service while the dialog is already open? Then the TunedDialog component needs to maintain its own DBusClient ins a useState hook, probably.

Hmmm right, what if it fails the error message get's "lost", so maybe we should do the extra work (and right thing) And start the service in the dialog?

Yes, catching the error is convincing. I have done this now. The explicit close() function for closing the DBus channel is not great, that should rather be done in a "destroy" function of useObject or useEffect... do we care enough to refactor the code more?

The TunedPerformanceProfile component was keeping a DBus connection open to tuned, and was closing and reopening it whenever something might have changed that requires it. This happened asynchronously with the rest of the code that was using this DBus connection, and would frequently cause that code to fail with "disconnected" errors. Also, the TunedDialog was itself causing changes that require a new DBus connection (namely starting the tuned.service), but was always working with the one that was originally passed into it via its properties. Let's straighten this all out: - Polling for the button state will open its own dedicated DBUs connection and close it when polling is done. There is no need to keep a DBus connection open for the whole Cockpit session. - The tuned.service is started when the dialog is opened and a DBus connection is opened explicitly for the dialog after that and closed when the dialog is closed.

mvollmer · 2024-10-22T08:57:23Z

that should rather be done in a "destroy" function of useObject or useEffect... do we care enough to refactor the code more?

Yes, I did that as well.

cockpituous · 2024-10-22T09:46:54Z

pkg/lib/service.js

@@ -290,9 +290,9 @@ export function proxy(name, kind) {

            call_manager(dbus, method, args)
                    .then(([path]) => { pending_job_path = path })
-                    .catch(() => {
+                    .catch(ex => {


This added line is not executed by any test.

cockpituous · 2024-10-22T09:46:54Z

pkg/lib/service.js

                        dbus.close();
-                        reject();
+                        reject(ex);


This added line is not executed by any test.

cockpituous · 2024-10-22T09:46:54Z

pkg/systemd/overview-cards/tuned-dialog.jsx

+            })
+            .catch((ex) => {
+                if (!tunedService.exists)
+                    return ({ state: "not-installed" });


This added line is not executed by any test.

cockpituous · 2024-10-22T09:46:54Z

pkg/systemd/overview-cards/tuned-dialog.jsx

+                if (!tunedService.exists)
+                    return ({ state: "not-installed" });
+                else if (tunedService.state != "running")
+                    return ({ state: "not-running" });


This added line is not executed by any test.

cockpituous · 2024-10-22T09:46:54Z

pkg/systemd/overview-cards/tuned-dialog.jsx

+            let status;
+
+            if (state == "not-installed")
+                status = _("Tuned is not available");


This added line is not executed by any test.

cockpituous · 2024-10-22T09:46:54Z

pkg/systemd/overview-cards/tuned-dialog.jsx

@@ -303,14 +305,14 @@ const TunedDialog = ({
               className="ct-m-stretch-body"
               isOpen
               help={help}
-               onClose={Dialogs.close}
+               onClose={() => dialogResult.resolve()}


This added line is not executed by any test.

cockpituous · 2024-10-22T09:46:54Z

pkg/systemd/overview-cards/tuned-dialog.jsx

               title={_("Change performance profile")}
               footer={
                   <>
                       <Button variant='primary' isDisabled={!selected} onClick={setProfile}>
                           {_("Change profile")}
                       </Button>
-                       <Button variant='link' onClick={Dialogs.close}>
+                       <Button variant='link' onClick={() => dialogResult.resolve()}>


This added line is not executed by any test.

github-advanced-security bot found potential problems Oct 14, 2024

View reviewed changes

pkg/systemd/overview-cards/tuned-dialog.jsx Fixed Show fixed Hide fixed

pkg/systemd/overview-cards/tuned-dialog.jsx Fixed Show fixed Hide fixed

jelly reviewed Oct 14, 2024

View reviewed changes

mvollmer mentioned this pull request Oct 14, 2024

system: Keep DBus connection to tuned open over service state changes #21115

Closed

mvollmer mentioned this pull request Oct 14, 2024

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

Open

mvollmer force-pushed the tuned-straighten-out-dbus-connections branch from ec42cb7 to 50c64f6 Compare October 14, 2024 12:54

mvollmer marked this pull request as ready for review October 14, 2024 16:00

mvollmer requested a review from jelly October 15, 2024 09:04

mvollmer added the needswork label Oct 16, 2024

lib: Pass along errors when starting systemd units

5b049f5

mvollmer force-pushed the tuned-straighten-out-dbus-connections branch from 50c64f6 to c94b7e5 Compare October 22, 2024 08:32

mvollmer removed the needswork label Oct 22, 2024

mvollmer force-pushed the tuned-straighten-out-dbus-connections branch from c94b7e5 to dad8dd7 Compare October 22, 2024 08:57

cockpituous reviewed Oct 22, 2024

View reviewed changes

jelly approved these changes Oct 22, 2024

View reviewed changes

mvollmer merged commit e563926 into cockpit-project:main Oct 22, 2024
82 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

systemd: Don't open and close tuned DBus connections asynchronously #21114

systemd: Don't open and close tuned DBus connections asynchronously #21114

mvollmer commented Oct 14, 2024 •

edited

Loading

mvollmer commented Oct 14, 2024

mvollmer commented Oct 14, 2024

jelly left a comment

jelly Oct 14, 2024

mvollmer Oct 14, 2024

mvollmer commented Oct 14, 2024

mvollmer commented Oct 14, 2024

mvollmer commented Oct 15, 2024

jelly commented Oct 16, 2024

mvollmer commented Oct 22, 2024

mvollmer commented Oct 22, 2024

cockpituous Oct 22, 2024

cockpituous Oct 22, 2024

cockpituous Oct 22, 2024

cockpituous Oct 22, 2024

cockpituous Oct 22, 2024

cockpituous Oct 22, 2024

cockpituous Oct 22, 2024

systemd: Don't open and close tuned DBus connections asynchronously #21114

systemd: Don't open and close tuned DBus connections asynchronously #21114

Conversation

mvollmer commented Oct 14, 2024 • edited Loading

mvollmer commented Oct 14, 2024

mvollmer commented Oct 14, 2024

jelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer commented Oct 14, 2024

mvollmer commented Oct 14, 2024

mvollmer commented Oct 15, 2024

jelly commented Oct 16, 2024

mvollmer commented Oct 22, 2024

mvollmer commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer commented Oct 14, 2024 •

edited

Loading