Draft initramfs method #11
Conversation
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @kvaps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the tool's boot capabilities by introducing an 'initramfs' method. This new approach allows the tool to leverage the host system's existing kernel, dynamically modifying a generic Talos initramfs to include the necessary host kernel modules. This ensures greater compatibility and adaptability when booting Talos, particularly in environments where a specific Talos kernel or UKI might not be readily available or compatible. The changes also include advanced kernel module management and comprehensive kexec error handling. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new initramfs boot method, which is a significant addition. The implementation involves complex interactions with the host system, including kernel module loading, file system manipulation, and kexec calls. The code is well-structured, but there are several areas for improvement regarding portability, robustness, and maintainability. My review includes feedback on critical error handling, hardcoded system-specific values, code duplication, and other potential issues.
| } | ||
| fmt.Println() | ||
|
|
||
| tmpDir, _ := os.MkdirTemp("", "initramfs-*") |
There was a problem hiding this comment.
The error returned by os.MkdirTemp is ignored. If creating the temporary directory fails (e.g., due to permissions or lack of space), the program will continue and then likely crash with a panic when it tries to use tmpDir. This error must be handled.
| tmpDir, _ := os.MkdirTemp("", "initramfs-*") | |
| tmpDir, err := os.MkdirTemp("", "initramfs-*") | |
| must("create temporary directory", err) |
| // Use finit_module syscall | ||
| // SYS_FINIT_MODULE = 313 on x86_64 | ||
| // long finit_module(int fd, const char *param_values, int flags); | ||
| const SYS_FINIT_MODULE = 313 |
There was a problem hiding this comment.
The syscall number for finit_module is hardcoded as 313. This value is specific to the x86_64 architecture, which makes the code non-portable. To improve portability and maintainability, consider using build constraints to define architecture-specific constants, or update golang.org/x/sys to a version that includes unix.FinitModule and use that instead.
| log.Printf("cmdline: %s", cmdline) | ||
|
|
||
| // Call kexec_file_load via syscall | ||
| const SYS_KEXEC_FILE_LOAD = 320 |
There was a problem hiding this comment.
The syscall number for kexec_file_load is hardcoded as 320. This is specific to x86_64 and harms portability. Consider using build constraints to define architecture-specific constants or updating dependencies to use a library function if available (e.g., unix.KexecFileLoad in newer golang.org/x/sys versions).
| const LINUX_REBOOT_CMD_KEXEC = 0x45584543 | ||
| const LINUX_REBOOT_MAGIC1 = 0xfee1dead | ||
| const LINUX_REBOOT_MAGIC2 = 672274793 | ||
| const SYS_REBOOT = 169 | ||
| _, _, errno2 := unix.Syscall6( | ||
| SYS_REBOOT, | ||
| LINUX_REBOOT_MAGIC1, // magic1 | ||
| LINUX_REBOOT_MAGIC2, // magic2 | ||
| LINUX_REBOOT_CMD_KEXEC, // cmd | ||
| 0, // arg (unused) | ||
| 0, // unused | ||
| 0, // unused | ||
| ) | ||
| if errno2 != 0 { | ||
| return fmt.Errorf("reboot with kexec failed: %v", errno2) | ||
| } |
There was a problem hiding this comment.
The syscall number for reboot is hardcoded, and unix.Syscall6 is used directly. The golang.org/x/sys/unix package provides unix.Reboot(cmd int), which is a portable and safer wrapper for this syscall that handles the magic numbers internally. Using it will make the code cleaner and more portable.
| const LINUX_REBOOT_CMD_KEXEC = 0x45584543 | |
| const LINUX_REBOOT_MAGIC1 = 0xfee1dead | |
| const LINUX_REBOOT_MAGIC2 = 672274793 | |
| const SYS_REBOOT = 169 | |
| _, _, errno2 := unix.Syscall6( | |
| SYS_REBOOT, | |
| LINUX_REBOOT_MAGIC1, // magic1 | |
| LINUX_REBOOT_MAGIC2, // magic2 | |
| LINUX_REBOOT_CMD_KEXEC, // cmd | |
| 0, // arg (unused) | |
| 0, // unused | |
| 0, // unused | |
| ) | |
| if errno2 != 0 { | |
| return fmt.Errorf("reboot with kexec failed: %v", errno2) | |
| } | |
| const LINUX_REBOOT_CMD_KEXEC = 0x45584543 | |
| if err := unix.Reboot(LINUX_REBOOT_CMD_KEXEC); err != nil { | |
| return fmt.Errorf("reboot with kexec failed: %v", err) | |
| } |
| initramfsModulesPath := filepath.Join(initramfsModulesDir, kernelVersion) | ||
| os.MkdirAll(initramfsModulesPath, 0o755) | ||
|
|
||
| // Copy and decompress modules |
There was a problem hiding this comment.
This block of code for walking the module directory, copying files, and decompressing .zst files is nearly identical to the block at lines 705-745. This significant code duplication makes the code harder to maintain and more prone to bugs. This logic should be extracted into a dedicated helper function that takes source and destination paths as arguments.
| var moduleFile *os.File | ||
| if needsDecompress { | ||
| // Decompress module to a temporary file | ||
| tmpModulePath := modulePath + ".decompressed" |
There was a problem hiding this comment.
A temporary file is created by appending .decompressed to the module path. This is not guaranteed to create a unique filename and could lead to race conditions or errors if the same module is processed concurrently. It's safer to use os.CreateTemp to generate a unique temporary file.
| initrdFD := int(initrdFile.Fd()) | ||
|
|
||
| // Read current cmdline from /proc/cmdline | ||
| currentCmdlineBytes, _ := os.ReadFile("/proc/cmdline") |
There was a problem hiding this comment.
The error returned by os.ReadFile("/proc/cmdline") is ignored. While this file should generally exist on a running Linux system, failing to read it could indicate a problem. It's better practice to handle this error, at least by logging a warning, to make debugging easier in unexpected environments.
| currentCmdlineBytes, _ := os.ReadFile("/proc/cmdline") | |
| currentCmdlineBytes, err := os.ReadFile("/proc/cmdline") | |
| if err != nil { | |
| log.Printf("warning: could not read /proc/cmdline: %v", err) | |
| } |
| } | ||
|
|
||
| // Download initramfs | ||
| initramfsURL := "https://github.com/siderolabs/talos/releases/download/v1.12.0/initramfs-amd64.xz" |
There was a problem hiding this comment.
The initramfs download URL is hardcoded with a specific Talos version (v1.12.0) and architecture (amd64). This makes the tool inflexible and requires code changes to use a different version or architecture. Consider making these configurable, for example, via command-line flags.
|
|
||
| // Replace kernel modules | ||
| log.Print("replacing kernel modules") | ||
| oldModulesPath := filepath.Join(squashfsDir, "lib", "modules", "6.18.1-talos") |
There was a problem hiding this comment.
The path to the old kernel modules directory in the Talos initramfs (6.18.1-talos) is hardcoded. This is very brittle and will break if the Talos installer image is updated with a different kernel version. The code should attempt to discover this path dynamically if possible, or at least make it a configurable parameter.
Signed-off-by: Andrei Kvapil kvapss@gmail.com