Skip to content

Questions and storage#248

Merged
imobachgs merged 19 commits intomasterfrom
questions-and-storage
Aug 26, 2022
Merged

Questions and storage#248
imobachgs merged 19 commits intomasterfrom
questions-and-storage

Conversation

@imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Aug 26, 2022

Problem

While working on #247 we found out that the manager might get blocked for quite some time while waiting for the user's answer. So we decided to implement the overdue move storage/bootloader to a separate process (#207).

However, this change introduces a bi-directional dependency between the storage and the manager (as storage needs to access the questions API). So we decided to move that API to a separate process too (extending #169).

Solution

  • Move bootloader/storage layer to org.opensuse.DInstaller.Storage.
  • Move the questions API to org.opensuse.DInstaller.Questions.

Caveats

As DInstaller::Security is only used to set up the Linux Security Modules and it is closely related to bootloader, we decided to move its usage to this storage/bootloader process.

Testing

  • Added a new unit test
  • Tested manuall

@coveralls
Copy link

coveralls commented Aug 26, 2022

Pull Request Test Coverage Report for Build 2933662478

  • 110 of 228 (48.25%) changed or added relevant lines in 16 files are covered.
  • 37 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-8.2%) to 72.602%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/lib/dinstaller/dbus/storage.rb 0 1 0.0%
service/lib/dinstaller/manager.rb 9 10 90.0%
service/lib/dinstaller/helpers.rb 11 13 84.62%
service/lib/dinstaller/dbus/questions_service.rb 0 35 0.0%
service/lib/dinstaller/dbus/storage_service.rb 0 37 0.0%
service/lib/dinstaller/dbus/storage/manager.rb 0 42 0.0%
Files with Coverage Reduction New Missed Lines %
service/lib/dinstaller/dbus/storage.rb 1 0%
service/lib/dinstaller/storage/proposal.rb 1 85.25%
service/lib/dinstaller/can_ask_question.rb 5 37.5%
service/lib/dinstaller/storage.rb 7 0%
service/lib/dinstaller/storage/actions.rb 23 0%
Totals Coverage Status
Change from base Build 2919263604: -8.2%
Covered Lines: 1688
Relevant Lines: 2325

💛 - Coveralls

@imobachgs imobachgs marked this pull request as ready for review August 26, 2022 11:56
end

ORDERED_SERVICES = [:language, :software, :users, :manager].freeze
ORDERED_SERVICES = [:questions, :language, :software, :storage, :users, :manager].freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

when I see it question is what is future of that containerized dinstaller? if we want to keep container per process we need to update way how it is run. ( but as we do not have yet docker-compose or something similar, probably not big issue )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we need to think a little bit about it. Actually, the questions service does not even depend on YaST at all.

# Run a block in the target system
#
# @param block [Proc] Block to run on the target system
def on_target(&block)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is something that I can imagine to have in WFM itself, just with additional param target

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds like a good idea. I will make a note.

Yast::WFM.CallFunction("inst_prepdisk", [])
start_progress(4)
progress.step("Preparing bootloader proposal") do
# first make bootloader proposal to be sure that required packages are installed
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, how this will work with separate processes? it first says which packages bootloader needs and then writes bootloader, but where it wait till it is installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At that point, it adds the packages to the PackagesProposal (through D-Bus, thanks to our overriden PackagesProposal.

Later, when the bootloader is about to be installed, the packages should be there.

@imobachgs imobachgs merged commit afdace9 into master Aug 26, 2022
@imobachgs imobachgs deleted the questions-and-storage branch August 26, 2022 12:53
<allow send_destination="org.opensuse.DInstaller.Questions" />
<allow send_destination="org.opensuse.DInstaller.Questions"
send_interface="org.freedesktop.DBus.Introspectable"/>
<allow send_destination="org.opensuse.DInstaller.Questions" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this line is duplicate, overall this is equivalent to

<allow send_destination="org.opensuse.DInstaller.Questions" />
<allow   receive_sender="org.opensuse.DInstaller.Questions" />

@imobachgs imobachgs restored the questions-and-storage branch November 23, 2022 09:04
@imobachgs imobachgs deleted the questions-and-storage branch November 23, 2022 09:05
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.

4 participants