Skip to content

Conversation

@joseivanlopez
Copy link
Contributor

@joseivanlopez joseivanlopez commented Mar 31, 2025

Add auto-installation support for configuring iSCSI.

Main changes:

  • Extend profile schema to include an iscsi section.
  • Extend D-Bus API:
    • Export object /org.opensuse/Agama/Storage1/ISCSI.
    • Provide #SetConfig method.
  • Extend HTTP API:
    • Add endpoint /iscsi/config (post)
    • Add endpoint /iscsi/issues (get)
    • Set iSCSI config before setting storage config (e.g., config load).

Out of scope:

  • Export iSCSI config (#GetConfig).
  • AutoYaST conversion (will be addressed in a separate card).
  • UI migration to the new iSCSI API (has to be evaluated).

@mvidner
Copy link
Contributor

mvidner commented Apr 3, 2025

FYI, the problem with returning the wrong type for the CurrentStep property and ruby-dbus correctly failing but not telling you which property is the problem: being addressed in mvidner/ruby-dbus#147

@joseivanlopez joseivanlopez force-pushed the auto-iscsi branch 4 times, most recently from ec8d68e to 919bc77 Compare April 7, 2025 10:52
@joseivanlopez joseivanlopez changed the title feat: add autoinstall support for iSCSI feat(storage): add autoinstall support for iSCSI Apr 7, 2025
@joseivanlopez joseivanlopez force-pushed the auto-iscsi branch 6 times, most recently from cd53c1b to 1b78ce6 Compare April 9, 2025 05:38
@joseivanlopez joseivanlopez changed the title feat(storage): add autoinstall support for iSCSI feat(storage): Add auto-installation support for configuring iSCSI Apr 9, 2025
@joseivanlopez joseivanlopez changed the title feat(storage): Add auto-installation support for configuring iSCSI feat(storage): Add auto-installation support for iSCSI Apr 9, 2025
@joseivanlopez joseivanlopez marked this pull request as ready for review April 9, 2025 06:13
Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

Added some inline comments for minor things.

Other than that, I have mixed feelings about the commit titled "Add iSCSI Adapter". It somehow looks like making the code less object oriented and encapsulated.

Just to clarify, "mixed feelings" doesn't mean I think it is wrong. I would go on and merge as it is (after processing the inline comments), because I think it has advantages and the drawbacks may only be my personal impression.

Having said that, let me elaborate.

Before that commit there is a mixin WithIscsiAuth that is in charge of building the authentication hash from the whole struct (ie. to know what are the fields that are relevant for authentication). After the commit, the mixin is gone and that filtering of relevant fields is repeated in a couple of places.

On the other hand, before the commit the classes Initiator and Manager act as some kind of model for the business logic, encapsulating the data and its manipulation. After that, all manipulation is extracted to Adapter, which is a good step regarding single responsibility but also feels like a class that simply holds a bunch of methods in a similar way than YaST modules used to do things.

@joseivanlopez
Copy link
Contributor Author

joseivanlopez commented Apr 9, 2025

Before that commit there is a mixin WithIscsiAuth that is in charge of building the authentication hash from the whole struct (ie. to know what are the fields that are relevant for authentication). After the commit, the mixin is gone and that filtering of relevant fields is repeated in a couple of places.

Yes, the credentials hash is built in 3 different places: Storage1.ISCSI.Node#Login, Storage1.ISCSI.Initiator#Dicover and Storage1.ISCSI#SetConfig. Only the first two are built exactly in the same way (using the very same D-Bus options). The last one is built by the config importer. I removed the mixin because the code for building the credentials is quite trivial and it is repeated only once (hopefully temporary if we finally refac ISCSI APIs). Should I bring a mixin back for building the hash?

On the other hand, before the commit the classes Initiator and Manager act as some kind of model for the business logic, encapsulating the data and its manipulation. After that, all manipulation is extracted to Adapter, which is a good step regarding single responsibility but also feels like a class that simply holds a bunch of methods in a similar way than YaST modules used to do things.

Yes, currently both Initiator and Node classes are there only for representing "probed devices". The adapter concept was added to follow the network approach.

Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

LGTM

@joseivanlopez joseivanlopez merged commit 2d25742 into master Apr 9, 2025
19 checks passed
@joseivanlopez joseivanlopez deleted the auto-iscsi branch April 9, 2025 11:43
@imobachgs imobachgs mentioned this pull request Apr 22, 2025
imobachgs added a commit that referenced this pull request Apr 22, 2025
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Apr 23, 2025
https://build.opensuse.org/request/show/1272126
by user IGonzalezSosa + anag_factory
- Version 14

- Fixed detection of iBFT (bsc#1239046).
- Removed offload_card from iSCSI (bsc#1231385).

- bsc#1238038
  - copy NVMe configuration files from inst-sys to target
    (gh#agama-project/agama#2257).

- Allow to specify extra kernel parameters for bootloader
  (jsc#PED-10810)

- Add auto-installation support for iSCSI
 (gh#agama-project/agama#2231).

- Write the registration URL to the installed system (bsc#1239316).

- Make the extension version attribute optional, search the version
  automatically if it is missing (related to jsc#AGM-100)

- Allow to specify bootloader timeout in profile (jsc#PED-10810)

- Copy the hostname to the installed system if it exists
  (gh#agama-project/agama#2226).

- Properly map AutoYaST scripts "source" to "content"
- Always display the patterns
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.

5 participants