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.

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

Set kpartx as default mapper tool for s390 #2403

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

schaefi
Copy link
Collaborator

@schaefi schaefi commented Dec 11, 2023

Some time ago we moved the default partition mapper from kpartx to partx to reduce a package dependencies. Some months and issues later this change will now be reverted for the following reason. partx exists in util-linux in different versions and with different feature sets for different distributions. This makes it very hard to provide a consistently working partition mapper utility across the distributions in the way we had it with kpartx before. Changing the default for a tool that has issues and less support as the one before is a bad move and causes trouble to our users.
This Fixes #2277

@schaefi schaefi self-assigned this Dec 11, 2023
Copy link
Member

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

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

I don't think this is the appropriate response to the bug report. Nothing we're doing is particularly complex and the behavior in partx in util-linux hasn't changed in a long time, and we explicitly changed to this to make it easier by default to run in a container.

The bug this references seems to be related to something that the OBS builder VM does, so I don't think the problem is with us.

@schaefi
Copy link
Collaborator Author

schaefi commented Dec 11, 2023

The problem here is that if we change to a tool to make one area easier and broke another completely we should revert the decision. So partx in some versions has no support for other disk types e.g DASD on s390. Thus this default broke the build for an entire architecture which is not acceptable imho. Also I got reports from people who uses a non standard start sector for the disk and also had issues when partx did the mapping. So the report I referenced here is just one out of a collection of others who had trouble.

I thought it's better to switch to a default that has proven to work and allow to switch to partx for simplicity e.g in the container world rather than vice versa

If this is not acceptable than we should at least move the default to kpartx for the s390 architecture

Thoughts ?

@schaefi
Copy link
Collaborator Author

schaefi commented Dec 11, 2023

I don't believe OBS is a problem here. The setup of the mapping device nodes was no problem. Issues appeared when using the created device nodes and that's usually a kernel, ioctl or mapper code issue which is not related to the obs worker setup

@Conan-Kudo
Copy link
Member

So partx in some versions has no support for other disk types e.g DASD on s390.

Why does partx care about disk types? That's all in the kernel, isn't it?

@schaefi
Copy link
Collaborator Author

schaefi commented Dec 12, 2023

Why does partx care about disk types? That's all in the kernel, isn't it?

To my understanding the partition mapper needs to understand the partition table, read the offsets and creates a device map starting at the correct offsets per partition. In case of DASD kpartx does it right, partx does something but wrong. On the subsequent processing all kinds of issues arises because the data behind the mapped device is just bogus

@Conan-Kudo
Copy link
Member

Is there a bug filed with util-linux about it?

@schaefi
Copy link
Collaborator Author

schaefi commented Dec 13, 2023

Is there a bug filed with util-linux about it?

In the response on the bugzilla conversation at SUSE, Hannes Reinecke mentioned that upstream util-linux seems to work. I could not find a bug report that specifically fixes this particular issue though. Even if upstream works we have a number of distros out there that will not be able to build for this architecture.

I'm going to change the PR here to change the default to kpartx for s390 only. I think this is a fair compromise given that updates to these platforms usually moves at the speed of tectonic plates ;)

@schaefi schaefi changed the title Set kpartx as default mapper tool Set kpartx as default mapper tool for s390 Dec 13, 2023
@schaefi
Copy link
Collaborator Author

schaefi commented Dec 13, 2023

@Conan-Kudo ok so the current code changes the default to kpartx for s390 which is an important move as I said on the enterprise end, the move to partx broke all projects. So this needs to change even though an upstream update of util-linux will fix it I don't see that we can wait for this update to happen, we need to move quicker.

On the other this now still leaves the reporter from issue #2277 in a trouble state. As he says that he builds the image on his host there is no obs or koji involved. We have integration tests that works on leap as well as on TW with partx here:

It's obvious that partx is the reason why it fails for him but I don't understand why.

Please let us clarify this first with help from the reporter and then decided how to go forward.

Thanks

@Conan-Kudo
Copy link
Member

Please let us clarify this first with help from the reporter and then decided how to go forward.

Yes, I want to understand what's happening here before we go further, that said this is good to go since we do need s390x to work properly.

@Conan-Kudo
Copy link
Member

Can you please squash the two commits and rewrite the commit message for indicating just that we're changing it for s390x?

Some time ago we moved the default partition mapper from
kpartx to partx to reduce a package dependencies.
However, on the s390 architecture partx does not work
proplerly on e.g DASD devices. Thus the default mapper
tool for this architecture will change to kpartx
with this PR
@schaefi
Copy link
Collaborator Author

schaefi commented Dec 14, 2023

Can you please squash the two commits and rewrite the commit message for indicating just that we're changing it for s390x?

@Conan-Kudo done

@Conan-Kudo Conan-Kudo merged commit f7a22ce into master Dec 14, 2023
10 checks passed
@Conan-Kudo Conan-Kudo deleted the kpartx_as_default_mapper branch December 14, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cant build image by new version kiwi
2 participants