D-Bus API for iSCSI

[joseivanlopez]

Problem

D-Installer does not provide a way of connecting to iSCSI targets yet.

https://trello.com/c/iQ7BCexU/3261-5-d-installer-d-bus-interface-configure-the-iscsi-initiator-and-the-iscsi-targets

Solution

Extend current D-Bus API of the storage service in order to support iSCSI.

/DInstaller/Storage1
  .ObjectManager
  .DInstaller.ServiceStatus1
  .DInstaller.Progress1
  .DInstaller.Validation1
  .DInstaller.Storage1
  .DInstaller.Storage1.Proposal.Calculator
  .DInstaller.Storage1.ISCSI.Initiator
/DInstaller/Storage1/Proposal
  .DInstaller.Storage1.Proposal
/DInstaller/Storage1/iscsi_nodes/[0-9]+
  .DInstaller.Storage1.ISCSI.Node

Now the main object /DInstaller/Storage1 implements a new interface .DInstaller.Storage1.ISCSI.Initiator . That interface offers methods for configuring the iSCSI initiator, performing discovey, and deleting node.

Discovered nodes are dynamically exported in the path /DInstaller/Storage1/iscsi_nodes/[0-9]+, and such nodes implements the interface .DInstaller.Storage1.ISCSI.Node. A node allows to login and logout.

Follow-up:

• Configure offload card [create issue].
• Validate proposal after changing iSCSI nodes [create issue].

Examples

Configure iSCSI initiator name:

$ busctl --address unix:path=/run/d-installer/bus set-property org.opensuse.DInstaller.Storage /org/opensuse/DInstaller/Storage1 org.opensuse.DInstaller.Storage1.ISCSI.Initiator InitiatorName s "iqn.1996-04.de.suse:01:351e6d6249"

Perform discovery

$ busctl --address unix:path=/run/d-installer/bus call org.opensuse.DInstaller.Storage /org/opensuse/DInstaller/Storage1 org.opensuse.DInstaller.Storage1.ISCSI.Initiator Discover sua{sv} 192.168.100.215 3260 4 Username s testi Password s testi ReverseUsername s testt ReversePassword s testt

Login

$ busctl --address unix:path=/run/d-installer/bus call org.opensuse.DInstaller.Storage /org/opensuse/DInstaller/Storage1/iscsi_nodes/2 org.opensuse.DInstaller.Storage1.ISCSI.Node Login a{sv} 5 Username s testi Password s testi ReverseUsername s testt ReversePassword s testt Startup s onboot

Logut

$ busctl --address unix:path=/run/d-installer/bus call org.opensuse.DInstaller.Storage /org/opensuse/DInstaller/Storage1/iscsi_nodes/1 org.opensuse.DInstaller.Storage1.ISCSI.Node Logout

Delete

$ busctl --address unix:path=/run/d-installer/bus call org.opensuse.DInstaller.Storage /org/opensuse/DInstaller/Storage1 org.opensuse.DInstaller.Storage1.ISCSI.Initiator Delete o /org/opensuse/DInstaller/Storage1/iscsi_nodes/1

Testing

• Added unit tests
• Tested manually

Screenshots

[coveralls]

Coverage: 78.429% (+0.7%) from 77.731% when pulling b802a6d on joseivanlopez:iscsi-dbus into a457bfc on yast:master.

[mvidner]

Not directly applicable right now, but eventually I hope:
"Document D-Bus API inside introspection XML" #415

[mvidner]

In the review somebody mentioned that service/lib/dinstaller/dbus/storage/manager.rb was getting quite big, with all the interfaces it needs to have.

One way to address it would be to put each of the interfaces in its own Module... and we're already doing it with DInstaller::DBus::Interfaces::Progress, ServiceStatus an Validation.

Perhaps doing the same for all the remaining interfaces would solve the problem? Or split the manager object up according to the responsibilities?

If ruby-dbus can get an improvement to help with it, I'd happily add it, but so far I don't see one.

[joseivanlopez]

In the review somebody mentioned that service/lib/dinstaller/dbus/storage/manager.rb was getting quite big, with all the interfaces it needs to have.

One way to address it would be to put each of the interfaces in its own Module... and we're already doing it with DInstaller::DBus::Interfaces::Progress, ServiceStatus an Validation.

Perhaps doing the same for all the remaining interfaces would solve the problem? Or split the manager object up according to the responsibilities?

If ruby-dbus can get an improvement to help with it, I'd happily add it, but so far I don't see one.

The problem I see is that the interface properties and methods are defined by the dbus object. If a dbus object implements quite some ifaces, then it has to define all the methods for each intereface, which implies to have a dbus object with a lot of possibly unrelated logic. Moreover, we need to pay special attention to property/method name collition, specially when importing interfaces from a module (e.g., Progress iface).

I would like to have some kind of composition or similar approach for aisolating the interface logic and definition. An idea could be to do something similar to what a service does with exporting D-Bus objects. For example, interfces are defined in its own class, and then the D-Bus object add them:

class MyObject < DBus::Object
  def initialize
    foo_iface = DBus::Interface.new
    bar_iface = DBus::Interface.new

    add_interfaces foo_iface, bar_iface
  end
end

We could workaround it by implementing some kind of Interface and Object wrappers around DBus::Object in D-Installer, but maybe it would be better to do it directly in ruby-dbus.

And maybe, it would also open the door to dynamically removing interfaces in the future: remove_interface foo_iface.

[mvidner]

If a dbus object implements quite some ifaces, then it has to define all the methods for each intereface, which implies to have a dbus object with a lot of possibly unrelated logic.

But that problem is also true if we leave out D-Bus out of the picture, and then it looks to me like an ordinary OOP design problem: mixing too many responsibilities in a single class.

I would solve that by using different classes/objects. Here, the ISCSI.Initiator interface is IMHO a natural case for one.

Even if the objects lifetimes are tightly coupled (so you could take that a reason to combine them in a single class) I'd split them up for clarity and ease of maintenance.

[ancorgs]

Just releasing some comments (I did hit the wrong button when adding them).

[ancorgs]

Generally it looks pretty nice. Great work. But I added a couple of comments.

[ancorgs]

LGTM. Great work!