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.

Sign up for GitHub

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

Jump to bottom

fixes for advanced storage: WWID for multipath, path-id for zfcp, generate zfcp kickstart #5249

Closed
wants to merge 4 commits into from
Closed

fixes for advanced storage: WWID for multipath, path-id for zfcp, generate zfcp kickstart #5249

wants to merge 4 commits into from

Conversation

steffen-maier
Copy link
Contributor

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

@github-actions github-actions bot added the f39 label Oct 13, 2023
poncovka
poncovka reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@poncovka poncovka left a 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.

pyanaconda/ui/gui/spokes/advanced_storage.py Outdated Show resolved Hide resolved
pyanaconda/modules/storage/devicetree/viewer.py Outdated Show resolved Hide resolved
@jkonecny12
Copy link
Member

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 master and focus on next Fedora.

@steffen-maier steffen-maier changed the title fixes for advanced storage: WWID for multipath, path-id for zfcp, generate zfcp kickstart fixes for advanced storage: WWID for multipath, path-id for multipath members, generate zfcp kickstart Oct 16, 2023
@steffen-maier steffen-maier changed the base branch from fedora-39 to master October 16, 2023 12:19
@steffen-maier steffen-maier changed the title fixes for advanced storage: WWID for multipath, path-id for multipath members, generate zfcp kickstart fixes for advanced storage: WWID for multipath, path-id for zfcp, generate zfcp kickstart Oct 16, 2023
@KKoukiou KKoukiou added f40 and removed f39 labels Oct 16, 2023
@steffen-maier
Copy link
Contributor Author

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.

@steffen-maier
Copy link
Contributor Author

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.
The "new" blivet over there uses an "old" anaconda so the unit tests fail because they expect old blivet.
And the "new" anaconda here uses an "old" blivet and so the unit tests failed because I had them make expect a "new" blivet. How to untangle the dependencies? Hopefully, it works by

  1. reverting the premature unit test expectation
  2. merging the anaconda part here (which is kind of odd because it loosely depends on the blivet PR over there)
  3. unrevert the test expectations and merge it despite unit test fail
  4. merge the blivet part over there, which now has new unit test expectations matching the blivet code change

or

  1. merge the blivet part over there despite unit test fail
  2. unrevert the test expectations here and the anaconda unit test should succeed, assuming it uses the latest blivet from step 1
  3. merge the anaconda part here

Either way, it seems to need an override to merge despite unit test fail. I'm out of ideas.

This was referenced Oct 19, 2023
Merged
Closed
@vojtechtrefny
Copy link
Contributor

Actually it looks like a chicken and egg problem to me.

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

@steffen-maier
fix missing WWID values for multipath devices in advanced storage UI …
1202272 
…(#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]>
@steffen-maier
DeviceTreeViewer: Add path-id attribute to zfcp-attached SCSI disks
1ce59b9 
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]>
@steffen-maier
fix missing emit of zfcp kickstart statements
3363266 
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]>
@steffen-maier
Copy link
Contributor Author

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.

@VladimirSlavik
Copy link
Contributor

Looks like the tests are not good?

@steffen-maier
Copy link
Contributor Author

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:
My understanding is that it turns green once the related blivet change from storaged-project/blivet#1161 was merged (not yet) and appears as part of the (nightly?) unit test container environment. This understanding is based on #5249 (comment). Am I missing something?

@steffen-maier
Copy link
Contributor Author

Friendly ping.
Have I missed something in order to merge this?

@rvykydal
Copy link
Contributor

rvykydal commented Dec 7, 2023

/kickstart-test --testtype smoke

@rvykydal rvykydal mentioned this pull request Dec 8, 2023
Merged
rvykydal
rvykydal approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@rvykydal rvykydal left a 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.

@rvykydal
Copy link
Contributor

rvykydal commented Dec 8, 2023

@vojtechtrefny I think we can now merge the blivet part and re-run the tests here?

@vojtechtrefny
Copy link
Contributor

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

@steffen-maier
Copy link
Contributor Author

/kickstart-test --testtype smoke

@rvykydal
Copy link
Contributor

rvykydal commented Dec 13, 2023 •
edited
Loading

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

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

@rvykydal
Copy link
Contributor

rvykydal commented Dec 13, 2023 •
edited
Loading

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

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.
(But the required version should be actually the next blivet build 3.8.2-2 I guess)

@vojtechtrefny
Copy link
Contributor

(But the required version should be actually the next blivet build 3.8.2-2 I guess)

Next build (already done) is python-blivet-3.8.2-2

@rvykydal
Copy link
Contributor

Hello @steffen-maier. So this PR seems to be somehow broken in a way I am not able to re-run the tests.
I would suggest to add blivet version requirement e53e979 to the PR which should re-trigger the tests.
(Or we can merge the #5369)

@steffen-maier
Copy link
Contributor Author

Hello @steffen-maier. So this PR seems to be somehow broken in a way I am not able to re-run the tests. I would suggest to add blivet version requirement e53e979 to the PR which should re-trigger the tests. (Or we can merge the #5369)

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.
You can merge this PR or #5369, whatever is best for you.

@rvykydal
Copy link
Contributor

/kickstart-test --testtype smoke

@rvykydal
Copy link
Contributor

rvykydal commented Dec 18, 2023 •
edited
Loading

I've merged the #5369 (added the required blivet version bump to one of the commits).
Thank you!

@rvykydal rvykydal closed this Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poncovka poncovka poncovka left review comments

@rvykydal rvykydal rvykydal approved these changes

Assignees

@rvykydal rvykydal

Labels
f40
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@steffen-maier @jkonecny12 @vojtechtrefny @VladimirSlavik @rvykydal @poncovka @KKoukiou