-
Notifications
You must be signed in to change notification settings - Fork 357
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
fixes for advanced storage: WWID for multipath, path-id for zfcp, generate zfcp kickstart #5249
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some suggestions. Otherwise, the code looks reasonable, but I didn't test it.
Hi @steffen-maier thanks for your contribution! This PR has the same as #5250 . For Fedora-39 you need to have exception because final freeze is in progress now. I would instead recommend you to change branch to |
d8100fc
to
68b054b
Compare
I took storaged-project/blivet#1161 (review) too literal and did not think it through myself. Of course, this PR here does not (yet) see the blivet changes from storaged-project/blivet#1161. So we cannot yet adapt the unit test with 3b159b96249c. So I reverted that commit again with 899b6b5. |
Actually it looks like a chicken and egg problem to me. After having looked at the related/pre-req storaged-project/blivet#1161 and its unit test failure https://github.com/storaged-project/blivet/actions/runs/6548000845/job/17864147818?pr=1161, I realized that both blivet and anaconda run the anaconda unit tests, but these use blivet objects.
or
Either way, it seems to need an override to merge despite unit test fail. I'm out of ideas. |
Don't worry, this happens quite often actually. We usually just merge the blivet change first (ignoring the failed Anaconda unit tests in blivet's CI) and then re-run the Anaconda CI checks here to check the change works as expected (Anaconda uses nightly builds of Blivet in the CI here). |
…(#2046654) Multipath devices themselves do not have a path-id but only their path members. In that case, a good volume identifier is the WWID/WWN/UUID. Fixes: 8e75893 ("Remove the local storage object from the advanced storage spoke") Signed-off-by: Steffen Maier <[email protected]>
Currently, the UI prints the kernel device names for multipath path members. Kernel device names are unpredictable and non-persistent and thus hardly suitable as path identifier for users. This is a preparation providing backend data to be used in a future commit to show in the UI. FCoE, iSCSI, and NVDIMM already have the attribute since commit e74030e ("Add the path-id attribute to the DBus structure for device data"). Provide the same for zfcp-attached SCSI disks, which should always be used with multipathing. This change tolerates the absence of the id_path attribute if the corresponding change adding the attribute in blivet does not exist. Signed-off-by: Steffen Maier <[email protected]>
As user I expect that I can re-run the installation unattendedly when using the generated kickstart file of an interactive installation run (using the same installer boot options plus inst.ks=). Meanwhile, only those zfcp statements from a kickstart file provided as input for an installation run were also written into the generated kickstart file. Fix the missing zfcp statements in the generated kickstart file that originate in user interactions on the storage screen UI. Fixes: d819bb9 ("Remove unused writeKS methods.") Fixes: 0d76185 ("Remove all the writeKS methods except in network and storage.") Signed-off-by: Steffen Maier <[email protected]>
899b6b5
to
3363266
Compare
FYI: I just squashed and force-pushed the final code. It's the same code as before, minus the trailing revert of the anaconda unit tests using blivet zfcp objects. This is in anticipation of the related blivet change and once that is in, the unit tests here should be good again. |
Looks like the tests are not good? |
Regarding the one failed unit test in https://github.com/rhinstaller/anaconda/actions/runs/6696080043/job/18659161207?pr=5249: |
Friendly ping. |
/kickstart-test --testtype smoke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, sorry for the delay.
@vojtechtrefny I think we can now merge the blivet part and re-run the tests here? |
The blivet PR is now merged, you can restart the tests here (looks like I don't have permissions do that). |
/kickstart-test --testtype smoke |
I think we'll have to wait until the updated blivet is in rawhide. Anyway for some reason it seems impossible to re-run the unit tests in this PR so I am doing it here: #5369 |
So if I bump the blivet version (d9927c7) the container rebuild is triggered and the blivet copr build pulled in, so the tests in #5369 are passing. |
Next build (already done) is python-blivet-3.8.2-2 |
Hello @steffen-maier. So this PR seems to be somehow broken in a way I am not able to re-run the tests. |
Hi Radek, I cherry-picked your e53e979 from https://github.com/rvykydal/anaconda/tree/rv-steffen-maier-adv-stor-fixes to my branch of this PR here (steffen-maier:adv-stor-fixes) and pushed it, which seems to have triggered the tests. |
/kickstart-test --testtype smoke |
I've merged the #5369 (added the required blivet version bump to one of the commits). |
path-id for zfcp gets its (optional) data from storaged-project/blivet#1161
but that's not a hard dependency
I tested this locally with updates.img from code (anaconda+blivet) branched closely enough for the updates to work with RHEL9.1 products.img (current when I started development).
This is a forward port, which applied pretty much automatically to the new base.
@jstodola @poncovka @sharkcz