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

Port NVMe over fabrics GUI to Fedora #4514

Merged
merged 5 commits into from
Feb 28, 2024
Merged

Port NVMe over fabrics GUI to Fedora #4514

merged 5 commits into from
Feb 28, 2024

Conversation

VladimirSlavik
Copy link
Contributor

@VladimirSlavik VladimirSlavik commented Jan 18, 2023 •
edited
Loading

This ports #4423 to Fedora. See pictures there.

TODOs:

  • readiness lower on the stack - blivet etc.
  • manual testing
  • release notes

Most of the work by @poncovka. Looks like cherry-picking amended commits hides the other author :(

@VladimirSlavik VladimirSlavik added f38 Fedora 38 manual testing required This issue can't be merged without manual testing blocked Don't merge this pull request! notable change Important changes like API change, behavior change... labels Jan 18, 2023
@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype smoke

@poncovka poncovka added the release note required Write a release note for this change. label Jan 18, 2023
@VladimirSlavik
Copy link
Contributor Author

@vojtechtrefny do you have any idea about when the nvme fabrics code in blivet wold be upstream too?

@vojtechtrefny
Copy link
Contributor

@vojtechtrefny do you have any idea about when the nvme fabrics code in blivet wold be upstream too?

We plan to release new version of blivet in Fedora 38 -- blivet 3.7.0 and it will contain all the nvme changes that are available in RHEL, but we don't plan to release the libblockdev NVMe plugin until Fedora 39 (or possibly 40). The blivet code will work, but the additional information about the NVMe drives supplied by libblockdev won't.

@VladimirSlavik
Copy link
Contributor Author

In other words, this will have to wait, correct?

@vojtechtrefny
Copy link
Contributor

In other words, this will have to wait, correct?

Yes and no. Without the libblockdev plugin Blivet will report NVMe oF devices as "normal" NVMe devices. So the Anaconda support for NVMe oF won't work, but it also won't crash (if the blivet part is written correctly :-)). But it definitely has to wait for Blivet 3.7.0 release where the NVMeFabricsNamespaceDevice and other classes will be available.

@VladimirSlavik VladimirSlavik added f39 and removed f38 Fedora 38 labels Feb 23, 2023
@VladimirSlavik
Copy link
Contributor Author

Looks like blivet is in place now. Rebased.

poncovka
poncovka approved these changes Mar 23, 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.

Thank you!

@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype smoke

@VladimirSlavik VladimirSlavik removed the blocked Don't merge this pull request! label Mar 30, 2023
@VladimirSlavik
Copy link
Contributor Author

Blocked by the manual testing requirement.

@VladimirSlavik
Copy link
Contributor Author

Rebased.

@VladimirSlavik
Copy link
Contributor Author

Updates image: master_updates.img.gz

@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype smoke

@AdamWill
Copy link
Contributor

https://fedorapeople.org/groups/qa/anaconda-nvme-fabric-01.iso is an ISO with the PR included, for testing.

@skycastlelily
Copy link

Hi Vladimir,
I tested with the latest updates image:https://github.com/rhinstaller/anaconda/files/11325287/master_updates.img.gz,
the installation halted at "[ OK ] Finished dracut-pre-pivot.…dracut pre-pivot and cleanup hook."( I added loglevel=7,but failed to get more information)
Actually the installation halts there even without that updates image added.
On non-nvme-of servers ,with that updates , you will see nvme fabric devices choose is added.
I didn't get time to test this pr before your last rebase,but the above symptoms sound just like you described in the email before the last pr.
I checked the updates image,it dose contain the latest files.
So,you sure the latest pr works?Or,did I miss anything here?
I'm gonna to email you the following beaker jobs to see if you could find some might useful information.
1.successful fedora installation on non-nvme-of servers with the updates added
2.failed fedora installation on nvme-of server with the updates added
3.successful rhel installation on that nvme-of server

@AdamWill
Copy link
Contributor

@skycastlelily can you test with my ISO instead? it's possible the changes here are the kind that can't really be tested with an updates image (this does happen sometimes; usually if the change needs to happen before the update overlay process kicks in).

@skycastlelily
Copy link

skycastlelily commented Apr 26, 2023 •
edited
Loading

can you test with my ISO instead?

I'm afraid I'm not able to use that iso on beaker servers.

@VladimirSlavik Did you use Adam's iso to perform the test?If so,would you please tell me how ?Thanks.

@VladimirSlavik
Copy link
Contributor Author

No, I used update images.

@skycastlelily
Copy link

No, I used update images.

Then,would you please points out the different place of my job from yours or would you please tell me how you perform the test?Thanks

@AdamWill
Copy link
Contributor

So, do you want @skycastlelily to try this again with an updates image?

@VladimirSlavik
Copy link
Contributor Author

I'm told @tbzatek will manage that, somehow.

@tbzatek
Copy link
Contributor

tbzatek commented Sep 21, 2023

I'm told @tbzatek will manage that, somehow.

Right, I'm going to test this but first we have a couple of dracut changes to get in.

@github-actions GitHub Actions
Copy link

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Nov 21, 2023
@tbzatek
Copy link
Contributor

tbzatek commented Nov 28, 2023

Needs #5328

@github-actions github-actions bot removed the stale label Nov 29, 2023
@github-actions GitHub Actions
Copy link

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Jan 29, 2024
@tbzatek
Copy link
Contributor

tbzatek commented Jan 29, 2024

Hmm so there's a number of components like dracut that currently don't have all the changes we need in Fedora, so this is currently difficult to test.

Suggesting merging this and possibly fixing later. However since the same code has been included in RHEL for some time, I don't expect any issues.

@github-actions github-actions bot removed the stale label Jan 30, 2024
VladimirSlavik and others added 5 commits February 21, 2024 13:41
@VladimirSlavik @vojtechtrefny
Install nvme-cli on boot.iso
a9e2928 
Install external tooling for managing NVMe-FC devices in the installation
environment. The utilities are needed to manipulate NVM-Express devices,
in this case mainly because of NVMe over Fabrics.

Resolves: rhbz#2107346

(cherry picked from commit 369d83a)
@VladimirSlavik @vojtechtrefny
Filter out NVMe over Fibre Channel from local disks
4e78f3e 
Resolves: rhbz#2107346

(cherry picked from commit 4b16bc5)
@poncovka @vojtechtrefny
Update the .glade file for the Advanced Storage screen
4da1322 
Apply changes generated by Glade 3.40.0.

Related: rhbz#2107346
(cherry picked from commit f63b0ed)
@VladimirSlavik @vojtechtrefny
Add a NVMe-FC tab to the Advanced Storage screen
d40cb44 
Resolves: rhbz#2107346
(cherry picked from commit bd0c061)
@VladimirSlavik @vojtechtrefny
Add release note for NVMe-oF
c9686d6
@vojtechtrefny vojtechtrefny added f41 and removed f40 labels Feb 22, 2024
@vojtechtrefny
Copy link
Contributor

I've rebased this to the latest master and did some manual testing with @tbzatek so I think this is ready to be merged now.

@vojtechtrefny
Copy link
Contributor

/kickstart-test --testtype smoke

@vojtechtrefny vojtechtrefny self-assigned this Feb 22, 2024
M4rtinK
M4rtinK approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@M4rtinK M4rtinK 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, thanks! :)

@vojtechtrefny vojtechtrefny merged commit 1cd55f4 into rhinstaller:master Feb 28, 2024
21 of 22 checks passed
@VladimirSlavik VladimirSlavik deleted the master-port-nvme branch March 15, 2024 14:12
@VladimirSlavik
Copy link
Contributor Author

Thank you for testing and finishing this!

@jkonecny12 jkonecny12 mentioned this pull request Apr 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poncovka poncovka poncovka approved these changes

@M4rtinK M4rtinK M4rtinK approved these changes

Assignees

@vojtechtrefny vojtechtrefny

Labels
f41 manual testing required This issue can't be merged without manual testing notable change Important changes like API change, behavior change... port to RHEL10 release note required Write a release note for this change.
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@VladimirSlavik @vojtechtrefny @AdamWill @skycastlelily @tbzatek @M4rtinK @poncovka