Hi
I answered inline.
Thanks,
Roi
On Wed, Mar 7, 2012 at 1:33 PM, Dan Kenigsberg <danken(a)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(a)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 *