Merge ~raharper/curtin:fix/eui-is-not-wwn into curtin:master
Status: | Merged |
---|---|
Approved by: | Ryan Harper on 2019-10-24 |
Approved revision: | 3841a0dca1314b8311456316f7eb30da403d083f |
Merge reported by: | Server Team CI bot |
Merged at revision: | not available |
Proposed branch: | ~raharper/curtin:fix/eui-is-not-wwn |
Merge into: | curtin:master |
Diff against target: |
210 lines (+127/-14) 4 files modified
curtin/commands/block_meta.py (+8/-12) tests/unittests/test_block.py (+26/-0) tests/unittests/test_commands_block_meta.py (+90/-1) tests/unittests/test_curthooks.py (+3/-1) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Chad Smith | Approve on 2019-10-24 | ||
Server Team CI bot | continuous-integration | Approve on 2019-10-24 | |
Dan Watkins | 2019-10-22 | Approve on 2019-10-23 | |
Review via email:
|
Commit message
block_meta: use lookup for wwn, fix fallback from wwn, serial, path
NVMe devices may provide a ID_WWN value from udev, but the NVMe rules
do not create a by-id/wwn-$ID_WWN symlink which breaks behavior with
other ID_WWN devices resulting in curtin not finding certain NVMe
devices. Resolve this issue buy using block.lookup_disk on WWN values
as well as serial.
Additional fixes to block_meta.
- get_path_
path values in order to find the specified disk
- Add os.path.exists check to proposed constructed values, if the
path is not valid, we continue to use different keys if possible.
- Updated the Exception message to indicate the volume wasn't found
and emit the config it used.
LP: #1849322
- fa3dd45... by Ryan Harper on 2019-10-22
FAILED: Continuous integration, rev:fa3dd457103
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:fa3dd457103
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
Dan Watkins (oddbloke) wrote : | # |
A couple of nits (so leaving to you to merge) and some questions for my own edification, but looks good to me. Thanks!
- 493fa9f... by Ryan Harper on 2019-10-23
- 92188d8... by Ryan Harper on 2019-10-23
Dan Watkins (oddbloke) wrote : | # |
On Wed, Oct 23, 2019 at 08:22:56PM -0000, Ryan Harper wrote:
> Diff comments:
>
> > diff --git a/curtin/
> > index 37b93dd..74faa63 100644
> > --- a/curtin/
> > +++ b/curtin/
> > @@ -416,21 +419,23 @@ def get_path_
> > # sys/class/block access is valid. ie, there are no
> > # udev generated values in sysfs
> > volume_path = os.path.
> > - break
> > - elif disk_key == 'wwn':
> > - by_wwn = '/dev/disk/
> > - volume_path = os.path.
> > + if not os.path.
>
> Yes, I can move to bottom of loop.
Thanks!
> > + block_meta.
> > + self.m_
>
> OK, I think I can convert to an assertEqual on the call_args_list.
I think assert_called_with more accurately maps to `assertIn` (though
`assertEqual` may be more correct in the context of a given test, which
would be more like `assert_
`assert_has_calls` can most easily either be `assertEqual(
call(...)], m.call_args_list)` for strict ordering, or the same but with
the list wrapped in `set()` for non-strict ordering. Strictly speaking,
though, I think assert_has_calls is really doing something more like
`assertTrue(
but that probably doesn't matter in most contexts.
PASSED: Continuous integration, rev:92188d82e38
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
Chad Smith (chad.smith) wrote : | # |
I think we might need to fix the two points I mentioned inline. Will look again when you get a chance to review.
Chad Smith (chad.smith) wrote : | # |
Approve once we drop the inner try/except and manipulate the outer try/except ValueError to be a 'continue' instead of 'pass'
- 3841a0d... by Ryan Harper on 2019-10-23
FAILED: Continuous integration, rev:3841a0dca13
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
Ryan Harper (raharper) wrote : | # |
rebuilding, git.lp was unhappy.
PASSED: Continuous integration, rev:3841a0dca13
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:82f20fee48a 89862e006c15521 bb5078df09610e /jenkins. ubuntu. com/server/ job/curtin- ci/3750/ /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= metal-arm64/ 3750/ /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= metal-ppc64el/ 3750/ /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= metal-s390x/ 3750/ /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= torkoal/ 3750/
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild: /jenkins. ubuntu. com/server/ job/curtin- ci/3750/ /rebuild
https:/