initial API proposal for manager #42

jreidinger · 2022-02-25T12:45:38Z

Goal of this PR is to write top level manager API that control probing and installing. It is also responsible for providing reasonable progress information back to user.

coveralls · 2022-02-25T12:46:39Z

Pull Request Test Coverage Report for Build 1922880105

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 80.783%

Totals
Change from base Build 1897930787:	0.0%
Covered Lines:	190
Relevant Lines:	224

💛 - Coveralls

imobachgs · 2022-02-25T12:55:29Z

service/lib/dinstaller/dbus/manager.rb

-          # TODO
-          0
+        dbus_method :probe, "" do
+          # TODO: do it assynchronous. How? ractors will have problem with sharing yast data.


np: asynchronous

We need to do some research here. By now, we have been using threads and, although it is far from ideal, it worked. But sure we need a better solution. Ideally, we could have an actors-like solution where every subsystem is represented by an actor and they share nothing. But that's not realistic at this point.

sadly with way how we using yast I worry it will be very challenging

mvidner · 2022-02-25T13:50:16Z

Asynchronous calls and D-Bus

(This is probably different from what you've started to discuss, but I want to summarize it anyway)

In the old times (before 2009) the bus itself (dbus-daemon) would have a method call timeout (about 30 seconds) so things taking a long time had to be designed as void methods + signals bearing the reply.

But that has changed (suprisingly to me), and now the D-Bus API Design Guildelines say that a normal call+reply should be used, possibly disabling a timeout implemented in the language binding:

Note that D-Bus client bindings may implement synthetic timeouts of several tens of seconds, unless explicitly disabled for a call. For very long-running operations, you should disable the client bindings’ timeout and make it clear in the client’s UI that the application has not frozen and is simply running a long operation.

An anti-pattern to avoid in this situation is to start a long-running operation when a method call is received, then to never reply to the method call and instead notify the client of completion of the operation via a signal. This approach is incorrect as signal emissions do not have a one-to-one relationship with method calls — the signal could theoretically be emitted multiple times, or never, which the client would not be able to handle.

No timeout in ruby-dbus (yet)

Anyway, ruby-dbus does not currently impose a timeout on method calls, but there is an issue (mvidner/ruby-dbus#61) asking for it.

Client-side Asynchronous Calls in ruby-dbus

Also not what you're doing here, but for completeness:

Instead of a synchronous call

value = object_interface.some_method(args)
foo(value)
bar

you can do it asynchronously with

object_interface.some_method(args) { |value| foo(value) }
bar

so bar does not need to wait until some_method replies

mvidner · 2022-02-28T14:24:30Z

@jreidinger please remember to fill the PR description. It provides CONTEXT for people who happen to not have a copy of your brain. A link to Bugzilla/Trello is a good start.

service/lib/dinstaller/dbus/manager.rb

mvidner · 2022-02-28T16:24:50Z

service/lib/dinstaller/dbus/manager.rb

+        # t current major step
+        # t total minor steps. Can be zero which means no minor steps
+        # t current minor step
+        dbus_reader :progress, "(stttt)"


So a D-Bus client can poll the progress by reading the property, or listen for PropertiesChanged signals, right?

InstallationProgress has @dbus_obj&.Progress(...args...). How is that related? Which class (interface) is @dbus_obj?

Exactly. I see that pattern often.
InstallationProgress will be dropped. Plan is to use Progress.new in dbus Manager object and assign callback to it that raise signal. And attach progress to data structure read from that progress.object.

joseivanlopez · 2022-03-02T10:55:36Z

service/lib/dinstaller/manager.rb

    def_delegators :@software, :products, :product

-    # @return [InstallerStatus]
    attr_reader :status


NP: Please, document its type.

joseivanlopez · 2022-03-02T10:55:52Z

service/lib/dinstaller/manager.rb

-      # proposing installer instead of reading current one
-      Yast::Stage.Set("initial")
-    end
+    attr_reader :progress


NP: Please, document its type.

joseivanlopez · 2022-03-02T10:58:51Z

service/lib/dinstaller/installer_status.rb

@@ -23,29 +23,16 @@
 module DInstaller
  # This class represents the installer status
  class InstallerStatus


Maybe having a class is too much. Is this expected to grow?

Idea: What about having something like DInstaller::StatusManager class, which defines possible statuses, implement logic for changing status, for calling callbacks and so on? And DInstaller::Manager would simply instanciate a StatusManager and would request a status change when needed.

Well, I am not sure. I try to not touch this part :) Just change it from string to unsigned.

joseivanlopez · 2022-03-02T11:04:58Z

service/lib/dinstaller/manager.rb

+        Yast::Installation.destdir = "/mnt"
+        # lets propose it here to be sure that software proposal reflects product selection
+        # FIXME: maybe repropose after product selection change?
+        # first make bootloader proposal to be sure that required packages is installed


Typo: are installed.

joseivanlopez · 2022-03-02T11:15:35Z

service/lib/dinstaller/manager.rb

-    ensure
-      change_status(InstallerStatus::IDLE)
+      Thread.new do
+        sleep(1) # do sleep to ensure that dbus service is already attached


I guess this would not be needed if probing is launched by a client calling to Manager#probe dbus method, right?

yeap, but my goal is to get to probed status as soon as possible. That is why I do it when starting service.

joseivanlopez · 2022-03-02T11:48:35Z

service/lib/dinstaller/progress.rb

+  #     progress.next_minor_step("Doing subtask2")
+  #     subtask2
+  #     progress.next_minor_step("Finished subtask2")
+  class Progress


Maybe it would too much, but I also imagine this as a kind of ProgressManager, generating steps when requested:

steps_manager = StepsManager.new(steps: 3) steps_manager.next_step("Doing step1") #=> DInstaller::Step do_step1(steps_manager) steps_manager.next_step("Doing step2") do_step2(steps_manager) steps_manager.next_step("Doing step3") do_step3(steps_manager) ... def do_step1(steps_manager) current_step = steps_manager.current_step sub_steps_manager = current_step.sub_steps(2) #=> DInstaller::StepsManager sub_steps_manager.next_step("Doing" subtask1") subtask1 sub_steps_manager.next_step("Doing" subtask2") subtask2

This would allow to generate a tree of steps: step1 -> step 1.1 -> step 1.1.1. With this, we could also have different callbacks for substeps, etc. But as said, maybe is too much.

Well, having tree of steps is something that is hard to imagine for me how to display it in UI. I try to keep it simple now. So top level manager knows about top level steps and then module knows about module specific substeps if needed. ( from my current testing some steps like language probe is super fast, that it is even hard to notice it ).

Co-authored-by: Martin Vidner <[email protected]>

initial API proposal

82ae12a

imobachgs reviewed Feb 25, 2022

View reviewed changes

jreidinger added 3 commits February 25, 2022 22:05

add progress class

30e2dce

first attempt for manager

664b236

api proposal for progress

18b984b

mvidner reviewed Feb 28, 2022

View reviewed changes

jreidinger added 3 commits February 28, 2022 22:14

adapt to new manager API

a7d6035

wip

1874876

add workaround with sleep

2128880

joseivanlopez reviewed Mar 2, 2022

View reviewed changes

jreidinger and others added 2 commits March 2, 2022 15:38

Update service/lib/dinstaller/dbus/manager.rb

fec2be2

Co-authored-by: Martin Vidner <[email protected]>

changes from review

23b9ac1

imobachgs approved these changes Mar 3, 2022

View reviewed changes

jreidinger marked this pull request as ready for review March 3, 2022 12:23

jreidinger merged commit 2981954 into new-dbus-api Mar 3, 2022

dgdavid deleted the manager_implementation branch March 9, 2022 09:24

imobachgs mentioned this pull request Mar 15, 2022

New D-Bus API #70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial API proposal for manager #42

initial API proposal for manager #42

jreidinger commented Feb 25, 2022 •

edited

Loading

coveralls commented Feb 25, 2022 •

edited

Loading

imobachgs Feb 25, 2022

jreidinger Feb 25, 2022

mvidner commented Feb 25, 2022 •

edited

Loading

mvidner commented Feb 28, 2022

mvidner Feb 28, 2022

jreidinger Feb 28, 2022

joseivanlopez Mar 2, 2022

joseivanlopez Mar 2, 2022

joseivanlopez Mar 2, 2022

joseivanlopez Mar 2, 2022

jreidinger Mar 2, 2022

joseivanlopez Mar 2, 2022

joseivanlopez Mar 2, 2022

jreidinger Mar 2, 2022

joseivanlopez Mar 2, 2022

jreidinger Mar 2, 2022

initial API proposal for manager #42

initial API proposal for manager #42

Conversation

jreidinger commented Feb 25, 2022 • edited Loading

coveralls commented Feb 25, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1922880105

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvidner commented Feb 25, 2022 • edited Loading

Asynchronous calls and D-Bus

No timeout in ruby-dbus (yet)

Client-side Asynchronous Calls in ruby-dbus

mvidner commented Feb 28, 2022

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

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

jreidinger commented Feb 25, 2022 •

edited

Loading

coveralls commented Feb 25, 2022 •

edited

Loading

mvidner commented Feb 25, 2022 •

edited

Loading