Hi

I answered inline.

Thanks,
Roi

On Wed, Mar 7, 2012 at 1:33 PM, Dan Kenigsberg <danken@redhat.com> wrote:
On Wed, Mar 07, 2012 at 09:43:27AM +0200, Roi Dayan wrote:
> ok.
>
> attached patch for getSessionInfo() only.

Roi,

It's nice that you've reached agreement with Saggi, but would you please give
the laymen a short description of this patch? It seems unrelated to iser.


The patch for getSessionInfo() is to read session info from /sys/class/iscsi_session and /sys/class/iscsi_connection directories
if could not be found under /sys/devices/platform/host*/ which is how the function with right now.
Actually from a mail from Mike Christie I saw this morning I understood that relaying on the /sys/devices/platform/host*/ directory is not right.
Also he gave an example that all offload drivers have pci devs instead of platform so they wont appear in /sys/devices/platform/.

 

>
> thanks,
> Roi
>
> On Wed, Mar 7, 2012 at 12:23 AM, Saggi Mizrahi <smizrahi@redhat.com> wrote:
>
> > Hi, first of all I'm truly sorry about sending you all back and forth and
> > you have been most patient.
> >
> > Secondly I would like you to split your patch to 2 parts.
> > The getSessionInfo() changes are good and I would have ACKed them if they
> > where posted on their own.
> >
> > Again, discovery will use the interface passed in iface. There is no need
> > to do all the looping and there is no need for the config values. Accepting
> > your patch means that the transport in the interface given in the parameter
> > is ignored.
> >
> > I do know that there is no way to currently select iser in the UI and
> > backend.
> > This, although unfortunate, does not mean we need to start hacking things
> > in to VDSM.
> > My problem with accepting this change is that it is now supported by VDSMs
> > interface.
> > This will make things hard for us to support in the future.
> >
> > I completely sympathize with your strong will to not start poking in the
> > Engine and UI.
> >
> > In any case, there is a way I can meet you half way. I would rather it'd
> > be fixed properly but I do get that time constraints might force us to fix
> > this in a less than optimal manner.
> >
> > The fix as I see it:
> > In HSM when there is no initiator name passed, hsm will not only try
> > iface('default') but also iface('iser').
> > Doing it this way will mean that the fallback is only supported by the old
> > API and is implemented at the API level and not the iscsi abstraction layer.
> > I would have done it myself and sent the patch if I had a set up to test
> > it in.
> >
> > Note that you will need the OK of someone in the ovirt management to
> > approve this API semantic change and it's supportability. I'm just an
> > annoying developer, I don't have veto powers.

If this is enabled via a config item, it would be easily acceptible.


Yes. this is from vdsm.conf under [irq] section:
iscsi_transports = iser,tcp

by default iscsi_transports is tcp only.

 

But please note my unanswered questions:

> > > A casual read of the man page makes we wonder: could a simple
> > >
> > > iscsiadm -m discoverydb -t st -p ip:port -I iser --discover
> > >
> > > do the trick for discovery?
> > > Could HeaderDigest mangling be avoided?
> > >
> > > BTW, Saggi, does it make sense to have our API accomodate
> > > discovery/login over multiple interfaces?

iscsi.discoverSendTargets() could accept a *list* of interfaces, so that both
-I iser and -I default are used.



The patch doesn't touch HeaderDigest anymore. Changing iface to iser is enough.

If we can tell from the start that we want iser target with do do discovery with iser iface
but then all targets discovered will be set to iser iface.
Discovery is done over tcp so it's not that the result will be iser targets only.

The patch modifies addIscsiNode() which is called after discovery.
If the user wanted iser, according to the config file, we change the iface to iser.
If login fails we change iface back to "iface.name" which currently is always default (tcp).

 

Dan




--

Roi