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

storage: Remove support for NVDIMM namespaces #5353

Merged
merged 2 commits into from
Nov 30, 2023
Merged

storage: Remove support for NVDIMM namespaces #5353

merged 2 commits into from
Nov 30, 2023

Conversation

poncovka
Copy link
Contributor

All additional support for NVDIMM is being deprecated and removed, especially the support for the namespace reconfiguration. However, namespaces configured in the block/storage mode can be still used for installation.

The nvdimm kickstart command is deprecated and will be removed in future releases.

Depends on: pykickstart/pykickstart#469
Blocks: storaged-project/blivet#1172

Resolves: #INSTALLER-3766

@poncovka poncovka added the f40 label Nov 28, 2023
@poncovka poncovka mentioned this pull request Nov 28, 2023
Merged
@poncovka poncovka marked this pull request as draft November 28, 2023 14:56
@poncovka poncovka added the blocked Don't merge this pull request! label Nov 28, 2023
@poncovka
Copy link
Contributor Author

@vojtechtrefny I am able to select a NVDIMM device in the fsdax mode for the installation. It just won't boot. I guess that is the expected behaviour, right? I have used the Blivet changes as well for testing.

nvdimm_without_support

nvdimm_without_support_out

@vojtechtrefny
Copy link
Contributor

@vojtechtrefny I am able to select a NVDIMM device in the fsdax mode for the installation. It just won't boot. I guess that is the expected behaviour, right?

Yes, device mapper doesn't work with the fsdax mode (only with the sector mode). I'd say this is expected and we should just ignore it.

@vojtechtrefny
Copy link
Contributor

@vojtechtrefny I am able to select a NVDIMM device in the fsdax mode for the installation. It just won't boot. I guess that is the expected behaviour, right?

Yes, device mapper doesn't work with the fsdax mode (only with the sector mode). I'd say this is expected and we should just ignore it.

We could add pmemN devices (but not pmemNs devices (sector mode namespaces)) to the ignored list to avoid this.

VladimirSlavik
VladimirSlavik reviewed Nov 29, 2023
View reviewed changes
Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, but I didn't stop to think what might be missing...

@poncovka
Copy link
Contributor Author

We could add pmemN devices (but not pmemNs devices (sector mode namespaces)) to the ignored list to avoid this.

Done.

@poncovka
storage: Remove support for NVDIMM namespaces
607aebe 
All additional support for NVDIMM is being deprecated and removed, especially
the support for the namespace reconfiguration. However, namespaces configured
in the block/storage mode can be still used for installation.

The `nvdimm` kickstart command is deprecated and will be removed in future
releases.

Resolves: #INSTALLER-3766
@poncovka
storage: Ignore NVDIMM namespaces in a non-sector mode
5da3b57 
Don't allow to select devices that cannot be used for the installation.
For example, NVDIMM in the fsdax mode prevents the system from booting.
@poncovka poncovka marked this pull request as ready for review November 29, 2023 14:11
@poncovka
Copy link
Contributor Author

/kickstart-test --testtype smoke

vojtechtrefny
vojtechtrefny approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@vojtechtrefny vojtechtrefny 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.

@poncovka poncovka removed the blocked Don't merge this pull request! label Nov 29, 2023
@poncovka poncovka merged commit 7ee760e into rhinstaller:master Nov 30, 2023
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VladimirSlavik VladimirSlavik VladimirSlavik approved these changes

@vojtechtrefny vojtechtrefny vojtechtrefny approved these changes

Assignees
No one assigned
Labels
documentation f40
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@poncovka @vojtechtrefny @VladimirSlavik