Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

Projects
None yet
2 participants
@AdamWill
Copy link
Contributor

commented Oct 27, 2016

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 added a commit to AdamWill/anaconda that referenced this pull request Oct 27, 2016

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:
storaged-project/blivet#518 . This patch
adjusts anaconda to use it.
@@ -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,))

This comment has been minimized.

Copy link
@AdamWill

AdamWill Oct 27, 2016

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

This comment has been minimized.

Copy link
@vpodzime

vpodzime Oct 27, 2016

Contributor

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

@vpodzime
Copy link
Contributor

left a comment

Looks good to me otherwise.

@@ -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,))

This comment has been minimized.

Copy link
@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

This comment has been minimized.

Copy link
@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.

This comment has been minimized.

Copy link
@AdamWill

AdamWill Oct 27, 2016

Author 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 AdamWill force-pushed the AdamWill:fix-iscsi branch from 9f1290a to 2d6318f Oct 27, 2016

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2016

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.

@AdamWill AdamWill force-pushed the AdamWill:fix-iscsi branch from 2d6318f to 2e200c0 Oct 27, 2016

@vpodzime
Copy link
Contributor

left a comment

Looks almost perfect to me now, thanks a lot!

self.r_username = None
self.r_password = None

def infotuple(self):

This comment has been minimized.

Copy link
@vpodzime

vpodzime Oct 31, 2016

Contributor

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

This comment has been minimized.

Copy link
@AdamWill

AdamWill Oct 31, 2016

Author 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

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

Jenkins, this is ok to test.

@vpodzime

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

Jenkins, test this, please.

AdamWill added some commits Oct 26, 2016

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

@AdamWill AdamWill force-pushed the AdamWill:fix-iscsi branch from 2e200c0 to 6a305be Oct 31, 2016

@vpodzime vpodzime merged commit 7054b4c into storaged-project:2.1-devel Nov 1, 2016

1 check passed

default Build finished.
Details

AdamWill added a commit to AdamWill/anaconda that referenced this pull request Nov 4, 2016

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:
storaged-project/blivet#518 . This patch
adjusts anaconda to use it.

Resolves: rhbz#1378156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.