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

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

Merged
merged 1 commit into from Nov 7, 2016

Conversation

Projects
None yet
4 participants
@AdamWill
Copy link
Contributor

commented Oct 27, 2016

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.

Note: this assumes that the self.iscsi.log_into_node() call in pyanaconda/ui/gui/spokes/advstorage/iscsi.py will happen before this code gets hit; I think it should, but if that's not the case, it won't work right.

@vpodzime
Copy link
Contributor

left a comment

Looks good to me, thanks! And I believe this could stay the same even with extra changes suggested/requested in storaged-project/blivet#518

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2016

This should still be compatible with the revised version of storaged-project/blivet#518 . Obviously this shouldn't be merged until that is (although it wouldn't actually make anything any worse, I don't think.)

@jkonecny12

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

Jenkins, it's ok to test.

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2016

The blivet change is still not merged yet.

@jkonecny12

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

I know, these tests are failing because of other issue. So from tests point of view, this PR looks good.

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2016

The tests probably don't cover this code at all, then, because neither the current code nor the new code can possibly work at this point unless the tests are running with an ancient version of blivet that still uses libiscsi :)

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

The blivet change is merged now, this should be merged too.

@jkonecny12

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

I would like to merge it right away but it's the final freeze now so I don't want to merge it without a freeze exception or blocker.
I could of course merge it to master without a problem but first I want to know if we want to create the exceptions for f25.

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

we can count it as part of https://bugzilla.redhat.com/show_bug.cgi?id=1378156 , I'd say, since I just found all these bugs while fixing that.

@M4rtinK

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

BTW, I suggest noting bug numbers in the commit message (if known) - that way we can easily check if the bugs are blockers or have freeze exceptions.

We have some documentation for that here:
https://github.com/rhinstaller/anaconda/blob/master/CONTRIBUTING#L72

If the commit fixes/is related to more than one bug the RHEL-style notation can be used:
https://github.com/rhinstaller/anaconda/blob/master/CONTRIBUTING#L86

Thanks in advance! :)

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

eh, it doesn't technically fix that specific bug, I'm just suggesting we wedge all the iSCSI fixes in together because I don't feel like filing five new bugs, proposing them all as blockers, and getting them all voted on =)

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

@AdamWill AdamWill force-pushed the AdamWill:iscsi-node-auth branch from 770c5bf to f4f96e2 Nov 4, 2016

@jkonecny12

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

I just tested it and it looks like I'm getting unhandled exception from blivet on iscsi timeout which is causing crash of Anaconda.
Fix for this is here #862 .
I'm going to merge this now. Thank you for these patches.

@jkonecny12 jkonecny12 merged commit 66a2b4d into rhinstaller:f25-devel Nov 7, 2016

0 of 2 checks passed

Anaconda F25 tests
Details
default Build finished.
Details
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.