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

allow compressed kernels with gz suffix #120

Merged
merged 4 commits into from
Sep 23, 2021

Conversation

mslacken
Copy link
Member

@mslacken mslacken commented Sep 9, 2021

handy for aarch64

@mslacken mslacken mentioned this pull request Sep 9, 2021
Copy link
Member

@gmkurtzer gmkurtzer left a comment

Choose a reason for hiding this comment

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

Please fix linting errors.

@mslacken
Copy link
Member Author

mslacken commented Sep 10, 2021

Fixed linting error (make lint shows no errors) , although the linting must have existed before.
Still the handling of adding kernels is not optimal right now. I think it would good to have:

  1. individual names for kernel, not just using their version as name, as you can have the same kernel version for different arches
  2. resolving the module path from the kernel version is consequent and simple, but begs problems when the kernel have different naming schemes
    So wouldn't it be better to have additional someting like
    wwctl kernel import -k $(REL_PATH_TO_KERNEL) -m $(REL_PATH_TO_MODULES) -n $(KERNEL_NAME)

Copy link
Contributor

@WestleyK WestleyK 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, just a couple suggestions about error handling. Otherwise looks good.

internal/pkg/kernel/kernel.go Outdated Show resolved Hide resolved
internal/pkg/kernel/kernel.go Outdated Show resolved Hide resolved
// allow uncompressed kernels (can be handy for aarch64)
kernelImage = path.Join(root, "/boot/vmlinux-"+kernelVersion)
if !util.IsFile(kernelImage) {
return "", errors.New("Could not locate kernel image")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is okay for now, but for the future error message should not contain caps, or newlines.

WestleyK pushed a commit to WestleyK/warewulf that referenced this pull request Sep 10, 2021
@gmkurtzer
Copy link
Member

I just merged @WestleyK's lint timeout increase. Please merge with the main branch and pull that in which should get this past the lint CI.

Thanks!

@gmkurtzer
Copy link
Member

Are ARM kernels always suffixed with .gz or is that a SuSE thing?

@mslacken
Copy link
Member Author

Are ARM kernels always suffixed with .gz or is that a SuSE thing?
Sorry don't know this.,

@gmkurtzer
Copy link
Member

Right after the copy of the kernel, let's also do an uncompress of the kernel.

mslacken and others added 2 commits September 17, 2021 11:47
Co-authored-by: WestleyK <westleyk@nym.hush.com>
Co-authored-by: WestleyK <westleyk@nym.hush.com>
@mslacken
Copy link
Member Author

I am feeling a bit uneasy implementing uncompressing the kernel without a rename, as at this would mean I would add the kernel with

wwctl kernel import -r $FOO vmlinux-$VERSION.gz

but the in reality we would deliver the uncompressed kernel vmlinux-$VERSION but the kernel would be known to warewulf with '.gz' suffix, what is misleading.
So my idea right now is to change the wwctl kernel import in such a way, that it behaves similar to wwctl container import so that you can have an arbitrary name for the kernel. This would mean that the kernel can be imported with

wwctl kernel import -r $FOO vmlinux-$VERSION.gz $NAME

This solves the problem, as when the kernel is compressed and a $NAME is given and would have .gz suffix, it was the will of the user. If no $NAME is given and the kernel is compressed and has .gz suffix, the kernel would uncompressed and stored with NAME=$VERSION
This would also solve the problem, that at the moment it is not possible to have the same kernel version for two different arches.

@rhenwood-arm
Copy link

@mslacken - in my case, I like the idea of copying and rename to provide some sort of 'audatibility'. For example, I want to remind myself that the kernel I'm booting is the one I downloaded or fished out of an ISO - so I'll md5 the known good kernel and check what is in my directory that warewulf is using... so I manually gunzip the kernel to the name 'vmlinuz' and keep the known good long-named kernel beside it in the directory.

I would appreciate a quick example of how the command could be used to import two same kernels for different platforms?

Is it something like:

wwctl kernel import -r ~/pxe vmlinux-5.12.1-aarch64.gz suse_aarch64
wwctl kernel import -r ~/pxe vmlinux-5.12.1-x86_64.gz suse_x86_64

?

(and while I'm at it, I would also like something like: wwwctl kernel import -f path_to_vmlinuz_file :) )

@gmkurtzer
Copy link
Member

@mslacken:
If you type uname -r on your ARM SuSE system, what do you get? I ask because the command you reference here confuses me as I would have guessed it to be just $VERSION:

wwctl kernel import -r $FOO vmlinux-$VERSION.gz

@rhenwood-arm:

(and while I'm at it, I would also like something like: wwwctl kernel import -f path_to_vmlinuz_file :) )

It is not trivial to get the kernel module version path from a kernel file directly.

gmkurtzer added a commit to gmkurtzer/warewulf that referenced this pull request Sep 18, 2021
@gmkurtzer gmkurtzer mentioned this pull request Sep 18, 2021
@mslacken
Copy link
Member Author

mslacken commented Sep 20, 2021

As far I see the kernel and its modules are stored at the moment as /var/warewulf/provision/kernel/$VERSION/kmods.img and vmlinuz.
I would suggest to use '/var/warewulf/provision/kenrl/$NAME/as base directory and have an additionalversion` file in this directory which stores the version. The version is anyway only needed, when 'building/imporing' the kernel, so storing it is not necessary, but still nice to have.
See #139

@mslacken
Copy link
Member Author

@mslacken:
If you type uname -r on your ARM SuSE system, what do you get? I ask because the command you reference here confuses me as I would have guessed it to be just $VERSION:

On an ARM SUSE system 'uname -rgives the version without the .gz suffix. In/boot/the lkernels have avmlinuxprefix and a.gz` suffix.

@gmkurtzer
Copy link
Member

Will you check out #137, I think I got this all integated, including gzip decompression automatically for ARM kernels to support iPXE.

@gmkurtzer
Copy link
Member

I'm going to merge this because we have blockers on getting 4.2.0 out and this is holding other things up. If there are any bugs with this, let's fix those after this is merged.

@gmkurtzer gmkurtzer merged commit e82210c into warewulf:main Sep 23, 2021
@gmkurtzer
Copy link
Member

Apologies, I meant to merge the other version of the PR that includes the gunzip. Leaving this one closed for now.

@mslacken mslacken deleted the kernel-container branch November 9, 2021 17:04
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.

None yet

4 participants