On Wed, Mar 7, 2012 at 4:09 PM, Dan Kenigsberg <danken@redhat.com> wrote:
On Wed, Mar 07, 2012 at 02:08:57PM +0200, Roi Dayan wrote:
> 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/.

So, you say that a more intrusive patch is needed? Or your current version is an
acceptable step forward?


yes. we can just skip checking the platform directory since even if a platform exists for a session
it will exists in /sys/class/iscsi_session/ anyway.

I also saw now that the function iterateIscsiSessions() also checking platform to know which sessions
exists.
I edited it to work with /sys/class/iscsi_session/ instead.

I attached an updated patch with the above.
its actually cleaner after removing the platform directory glob stuff.

 

> > > >
> > > > 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 this is from your old, obsoleted patch, right?
We are going to have a slightly different patch (this time to hsm.py)
I think a clearer name would be be

iscsi_default_ifaces = default,iser

ok. I'll change that.
 

> >
> > 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.

Which patch? Is there anything beyond the getSessionInfo patch?

>
> 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.

I still do not understand if it is possible/beneficial to use
 iscsiadm -m discoverydb -t st -p ip:port -I iser -I default --discover
in your use case.


Doing this discovery will just create a target with 2 iface configurations. one for tcp and one for iser.
I still need to do a login over iser and see if it fails to know if the target is not iser.
So if i want to do this during the discovery process I need per target to do a login
and if it fails to set it default iface and if not to do logout and set it iser iface.

 

>
> 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).

that's the old patch, right? when you can, please provide a patch with logic
moved to hsm.py.


I'm not sure where you want me to set it on hsm.py.
Should it be in connectStorageServer() ? it's marked deprecated.

If I want after discovery to check if target should be iser or default ( ..discoverSendTargets().. )
I'll have to do a login and logout per target as I described above.


 

Regards,
Dan






--

Roi