Fix iscsi problems: type of arg passed to storaged, store auth info when logging in with it #518

Merged
merged 2 commits into from Nov 1, 2016

Projects

None yet

2 participants

@AdamWill
Contributor

These commits fix a couple of problems in iSCSI support. The first is simple: the type of the port arg doesn't match the type string (q is an int). The second is more complicated; when we moved from libiscsi to storaged the node objects were changed from libiscsi PyIscsiNode instances to a new namedtuple class called NodeInfo...but as a consequence of that, they no longer provide any access to the usernames and passwords used to authenticate to the target, which both blivet and anaconda actually expect. This restores that information in a different form (it just gets stuffed into the NodeInfo tuple) and adapts blivet to the change; I'll send a corresponding PR for anaconda.

@AdamWill AdamWill added a commit to AdamWill/anaconda that referenced this pull request Oct 27, 2016
@AdamWill AdamWill iSCSI: adjust to change in how blivet passes auth info
When blivet switched from using libiscsi to storaged, the node
attribute of an `iScsiDiskDevice` changed from being a libiscsi
`PyIscsiNode` instance to a blivet `NodeInfo` namedtuple, but
both blivet and anaconda continued trying to call the `getAuth`
method of the node - which `NodeInfo` doesn't have - to find
the authentication info for the node. I've sent a blivet PR
to store this information in the `NodeInfo` namedtuple instead:
rhinstaller/blivet#518 . This patch
adjusts anaconda to use it.
770c5bf
@AdamWill AdamWill referenced this pull request in rhinstaller/anaconda Oct 27, 2016
Merged

iSCSI: adjust to change in how blivet passes auth info #850

blivet/iscsi.py
@@ -239,7 +242,7 @@ def _login(self, node_info, extra=None):
extra = dict()
extra["node.startup"] = GLib.Variant("s", "automatic")
- args = GLib.Variant("(sisisa{sv})", tuple(list(node_info) + [extra]))
+ args = GLib.Variant("(sisisa{sv})", node_info[:5] + (extra,))
@AdamWill
AdamWill Oct 27, 2016 edited Contributor

the change here other than slicing is just a simplification. Turning a tuple into a list, adding it to a list, then turning the resulting list back into a tuple seemed a bit silly - let's just add a tuple to a tuple.

@vpodzime
vpodzime Oct 27, 2016 Contributor

Nice, I didn't know that one can concatenate tuples like this.

@vpodzime

Looks good to me otherwise.

blivet/iscsi.py
@@ -239,7 +242,7 @@ def _login(self, node_info, extra=None):
extra = dict()
extra["node.startup"] = GLib.Variant("s", "automatic")
- args = GLib.Variant("(sisisa{sv})", tuple(list(node_info) + [extra]))
+ args = GLib.Variant("(sisisa{sv})", node_info[:5] + (extra,))
@vpodzime
vpodzime Oct 27, 2016 Contributor

Nice, I didn't know that one can concatenate tuples like this.

+ if r_username:
+ node.r_username = r_username
+ if r_password:
+ node.r_password = r_password
@vpodzime
vpodzime Oct 27, 2016 Contributor

A don't this can work. A NamedTuple is immutable, AFAICT. If we need to do this, we need to change the NodeInfo into a traditional class.

@AdamWill
AdamWill Oct 27, 2016 Contributor

ah, you're right. As I said in the bug report, I didn't test it with an actual authenticated iSCSI target yet. Hmm, guess I get to think about the design again. More terrible ideas pending!

@AdamWill
Contributor

Well, I guess this, then. If you think it's worth adding getAuth and setAuth I could do that, but I don't really think it is.

@vpodzime

Looks almost perfect to me now, thanks a lot!

blivet/iscsi.py
+ self.r_username = None
+ self.r_password = None
+
+ def infotuple(self):
@vpodzime
vpodzime Oct 31, 2016 Contributor

One last double-nitpick -- I think this should be a conn_info @property.

@AdamWill
AdamWill Oct 31, 2016 edited Contributor

someone (I think kparal) was very rude about my habit of using read-only properties once, so I tend to avoid introducing them to other people's code, though I still use them all over my own =) But if you want it that way, sure, can do.

@vpodzime
Contributor

Well, I guess this, then. If you think it's worth adding getAuth and setAuth I could do that, but I don't really think it is.

I don't think we need that.

@vpodzime
Contributor

Jenkins, this is ok to test.

@vpodzime
Contributor

Jenkins, test this, please.

AdamWill added some commits Oct 26, 2016
@AdamWill AdamWill Use correct type for port in GVariant tuple
The type is `(sqa{sv})`, where `q` (according to the docs) is
"an unsigned 16 bit integer", so this should be an int, not a
string.
5cee775
@AdamWill AdamWill iSCSI: Store auth info in NodeInfo tuples
This seems to have been overlooked in 9280eff . When we were
using libiscsi, the `node` objects were `PyIscsiNode` instances
(I think), with `getAuth` and `setAuth` methods that let you
read and set the authentication information for the node. We
used `getAuth` in `iScsiDiskDevice.dracut_setup_args()` to
include the auth information in the `netroot` arg. anaconda
also expects the `node` attribute of an `iScsiDiskDevice`
instance to be a `PyIscsiNode` and calls its `getAuth` method
to populate the kickstart data for the node.

When we ditched libiscsi and turned the `node` objects into
`NodeInfo` namedtuples, this was missed and not handled at all.
Both blivet and anaconda are still trying to call methods that
these node objects just don't have any more. The blivet call
was changed from `getAuth()` to `get_auth()` in 4e8f941 , but
apparently whoever did that didn't notice that neither method
exists at all for these objects any more...

Here's my attempt to fix this: basically, just stuff the auth
information into the `NodeInfo` instances when we log in. I
thought of several different ways to do this, but I think in
the end it always has to boil down to storing the auth details
on the node object when we log in, so let's just go with the
obvious way. We could mimic the `getAuth` and `setAuth` methods
pretty easily for 'compatibility', but it doesn't seem worth
it, we'd probably still be missing other bits of the interface.
6a305be
@vpodzime vpodzime merged commit 7054b4c into rhinstaller:2.1-devel Nov 1, 2016

1 check passed

default Build finished.
Details
@AdamWill AdamWill added a commit to AdamWill/anaconda that referenced this pull request Nov 4, 2016
@AdamWill AdamWill iSCSI: adjust to change in blivet auth info (#1378156)
When blivet switched from using libiscsi to storaged, the node
attribute of an `iScsiDiskDevice` changed from being a libiscsi
`PyIscsiNode` instance to a blivet `NodeInfo` namedtuple, but
both blivet and anaconda continued trying to call the `getAuth`
method of the node - which `NodeInfo` doesn't have - to find
the authentication info for the node. I've sent a blivet PR
to store this information in the `NodeInfo` class instead:
rhinstaller/blivet#518 . This patch
adjusts anaconda to use it.

Resolves: rhbz#1378156
f4f96e2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment