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

Report progress of DASD formatting to the D-Bus interface #476

Merged
merged 9 commits into from
Mar 21, 2023

Conversation

ancorgs
Copy link
Member

@ancorgs ancorgs commented Mar 20, 2023

Problem

#464 implemented a D-Bus API for managing DASDs, but a way to track the progress of the formatting progress was still missing.

Solution

This adds the concept of storage jobs, that are exported at /org/opensuse/DInstaller/Storage1/jobs/[0-9]+

Every time a formatting operation starts properly, the path of a new job is returned. That job can be used to track the formatting status of all the DASDs involved in the operation.

Testing

  • Added some minimal unit tests
  • Manually tested in a real s390 with real formatting (thanks @teclator)

Documentation

  • Expanded doc/dbus_api.md

@ancorgs ancorgs force-pushed the dasd_progress2 branch 2 times, most recently from 6fbc316 to ead54a4 Compare March 20, 2023 14:18
@coveralls
Copy link

coveralls commented Mar 20, 2023

Coverage Status

Coverage: 75.553% (+0.3%) from 75.22% when pulling ead58cf on dasd_progress2 into 7a6669b on master.

@ancorgs ancorgs marked this pull request as ready for review March 20, 2023 14:58
@ancorgs ancorgs force-pushed the dasd_progress2 branch 3 times, most recently from 4b370df to aa9e86b Compare March 20, 2023 17:49
@mvidner
Copy link
Member

mvidner commented Mar 21, 2023

as far as I know, that integration test is failing for all pull requests
and I would say it's being ignored (ie. we are merging PRs despite the test)
it was already broken for all these already merged PRs: 475, 478, 482 and 483

Copy link
Member

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

First review: the API

doc/dbus_api.md Outdated Show resolved Hide resolved
doc/dbus_api.md Outdated Show resolved Hide resolved
Copy link
Member

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Omitting the PropertiesChanged signal would indeed confuse caching clients like cockpit.dbus, but we'll fix that with EmitsChangedSignal

doc/dbus_api.md Outdated Show resolved Hide resolved
Copy link
Member

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Bad news, this design fails because ruby-dbus is not thread safe

job = nil
progress = proc { |statuses| job.update_format(statuses) }
finish = proc { |result| job.finish_format(result) }
initial_statuses = dasd_backend.format(dasds, on_progress: progress, on_finish: finish)
Copy link
Member

Choose a reason for hiding this comment

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

This method eventually spawns a thread.

race Yay ! conditions

Besides the job initialization race visible in this method,
there are more serious races in ruby-dbus usage: the above callbacks will use the bus connection at a random time point, racing the main thread for protocol data on the D-Bus socket.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, the methods in the thread only send signals, meaning that they only write to the D-Bus socket. Maybe that is less prone to races than reading.

Anyway, here is an old branch from 2010 when I got a multithreading patch and failed to make it work mvidner/ruby-dbus@master...multithreading

Copy link
Member Author

Choose a reason for hiding this comment

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

Besides the job initialization race visible in this method

As commented above, I considered it to be safe enough. Although we can rework the D-Bus API to make it even safer by creating the job object in advance. That would imply we also create Job D-Bus objects for processes that fail to start (the case now reported with the exit code 2). So there will be only two exit codes, 1 for wrong list of paths and 0 for process started, even if that process failed immediately.

there are more serious races in ruby-dbus usage

That's something I wouldn't know how to minimize or avoid.

Copy link
Member

Choose a reason for hiding this comment

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

mvidner/ruby-dbus#73 is a basic demonstration of the way it fails

Copy link
Member

Choose a reason for hiding this comment

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

mvidner/ruby-dbus#128 is good enough for signals

Copy link
Member

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Assuming that the rebase did not introduce any problem, I am checking the changes entry and approving the PR because it was already reviewed.

@ancorgs ancorgs merged commit f289c70 into master Mar 21, 2023
@ancorgs ancorgs deleted the dasd_progress2 branch March 21, 2023 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants