fix(initrd): merge file mounts into the existing CPIO archive - #991
fix(initrd): merge file mounts into the existing CPIO archive#991kastakhov wants to merge 2 commits into
Conversation
Signed-off-by: kastakhov <16296930+kastakhov@users.noreply.github.com>
✅ Deploy Preview for urunc canceled.
|
Signed-off-by: kastakhov <16296930+kastakhov@users.noreply.github.com>
e9308d3 to
09fe4f2
Compare
cmainas
left a comment
cmainas
left a comment
There was a problem hiding this comment.
Hello @kastakhov ,
thank you for this PR. I was wrong about my initial thought of simplification. This PR resolves multiple issues correctly. However, I have added some comments and some generic notes:
- Some parts of the code would benefit from some comments to explain the rationale and the logic (e.g.
./preifx inarchivePathso we can think twice if we need to change that in the future). - We do not really need the temporary file. It is fine to parse and then append the original file. If something goes wrong we will fail the execution. Then a new container will start and the snapshotter will create a fresh rootfs with the original initrd.
|
|
||
| func (i initrdRootfs) postSetup() error { | ||
| err := initrd.CopyFileMountsToInitrd(i.initrdHostFullPath, i.mounts) | ||
| updateInitrd := initrd.CopyFileMountsToInitrd |
There was a problem hiding this comment.
Lets not use function assignment here and simply call the respective function inside the respective if branch. We avoid function assignments.
| "github.com/urunc-dev/urunc/pkg/unikontainers/unikernels" | ||
| ) | ||
|
|
||
| func TestNewRootfsBuilderPassesGuestTypeToInitrd(t *testing.T) { |
There was a problem hiding this comment.
Let's add a TODO comment to move this test somewhere else, since it tests a function that does not belong to initrd_rootfs.go
| w := cpio.NewWriter(archive) | ||
| require.NoError(t, w.WriteHeader(&cpio.Header{ | ||
| Name: "./original", Mode: cpio.TypeReg | 0o644, Size: 3, | ||
| })) | ||
| _, err = w.Write([]byte("old")) | ||
| require.NoError(t, err) | ||
| require.NoError(t, w.Close()) |
There was a problem hiding this comment.
nit: We can squash these lines to just one require.NoError(t, cpio.NewWriter(archive).Close()). An initrd with just just the trailer is valid.
| var trailerOffset int64 | ||
| for { | ||
| hdr, err := r.Next() | ||
| if errors.Is(err, io.EOF) { |
There was a problem hiding this comment.
We still need to ensure that there is a valid trailer here. The "github.com/cavaliergopher/cpio" package will return EOF in the case trailer was read or if there are no bytes (actual eof).
| var missing []string | ||
| for parent := path.Dir(strings.TrimPrefix(name, "./")); parent != "."; parent = path.Dir(parent) { | ||
| archiveParent := "./" + parent | ||
| if _, exists := existingNames[archiveParent]; !exists { |
There was a problem hiding this comment.
This check can lead to duplicates. The exisitngNames map contains all entries found from findTrailer. But in findTrailer the value is written as found in the initrd record. However, there is no guarantee that the name will have the prefix "./". So, we should ensure that the values stored in existingNames have the same format as the ones we compare here.
|
|
||
| func addMissingParents(w *cpio.Writer, name string, existingNames map[string]struct{}, inodes *inodeAllocator) error { | ||
| var missing []string | ||
| for parent := path.Dir(strings.TrimPrefix(name, "./")); parent != "."; parent = path.Dir(parent) { |
There was a problem hiding this comment.
This loop is bug prone. If in the future we are careless and remove the "./" prefix in archivePath, then we risk having a full path which will never end up in ".".
There was a problem hiding this comment.
I think (not really tested it out) if we reverse the order (from top directory to children, then we wll also reduce the complexity of parsing the missing list in reverse.
| if err != nil { | ||
| return fmt.Errorf("could not allocate inode for directory %s: %w", parent, err) | ||
| } | ||
| hdr := &cpio.Header{Name: parent, Inode: inode, Mode: cpio.TypeDir | 0o755, Links: 2} |
There was a problem hiding this comment.
A comment here with the rational of Links: 2 will be helpful.
| return fileMounts, nil | ||
| } | ||
|
|
||
| func findTrailer(f *os.File) (int64, map[string]struct{}, *inodeAllocator, error) { |
There was a problem hiding this comment.
This function should be renamed to something that captures its role better (e.g. parseInitrd).
2 participants
Description
CopyFileMountsToInitrdappends file mounts as a second CPIO archive. Unikraft stops extracting at the originalTRAILER!!!, making the appended files unavailable inside the guest.This change:
Related issues
How was this tested?
test_docker,test_ctrLLM usage
OpenAI Codex (GPT-5.6) assisted with the implementation, test review, and PR description based on my specifications. I reviewed and approved the final changes.
Checklist
make lint).make test_ctr,make test_nerdctl,make test_docker,make test_crictl).