On Wed, Mar 7, 2012 at 4:09 PM, Dan Kenigsberg <[email protected]> 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 <[email protected]> > 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 *
vdsm-getsessioninfo-1331130748.patch
Description: Binary data
_______________________________________________ vdsm-patches mailing list [email protected] https://fedorahosted.org/mailman/listinfo/vdsm-patches
