Conversation
There was a problem hiding this comment.
Pull request overview
Adds an initial unikraft volume import command to import local data into an existing Unikraft Cloud volume by packaging the source into a CPIO archive and streaming it to a temporary “volimport” instance over TLS.
Changes:
- Introduces
volume importCLI command that builds/uses a CPIO archive and streams it to a per-volume import instance. - Adds
internal/builder.BuildImportRootfshelper to normalize an import source (directory vs existing CPIO). - Implements a TLS streaming protocol in
internal/builder/cpiofor sending CPIO entries and handling ack/stop responses.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| internal/cmd/volumes.go | Adds volume import command, instance spawning, TLS connect, and auth token generation. |
| internal/builder/import.go | Adds helper to build/validate an importable CPIO archive from a directory or existing CPIO file. |
| internal/builder/cpio/cpio.go | Adds CPIO magic detection and the TLS streaming/ack protocol implementation. |
| cmd/unikraft/volumes_test.go | Extends CLI help smoke test to include volume import --help. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
75717cb to
dd16f28
Compare
dd16f28 to
ac5d87c
Compare
|
I split all the suggested changes into separate commits. I will need to squash some of them up before it's ready to review again. Currently a bit broken as the Also rootfs building does not work without dockerfiles. (aka with directories or files) With these out of the way it should work again. |
Why do you need the "computer layer"? Do you need to open the file? If so, you can call
Indeed 🎉 I was hoping this was one of the things you might take a look at at some point, or even as part of this (though maybe in a separate PR). |
Hmm, then something is weird, because you told me to revert the file/directory/dockerfile detection that I previously had. (which is why I did them as commits on top instead of overwriting hehe.
Lol, not sure how I wrote that. I wanted to say the |
|
The detection should exist, yes, but it should be extracted - it shouldn't be done just for the volume imports. We should have one reusable function that does "pass this thing in, and build a rootfs from it". We may need to rework those functions and everything so that the signatures all work out. What we need to avoid is rewriting the same detection everywhere. |
|
Looking at how it's used actually... we could remove that method entirely. |
ac5d87c to
3679e46
Compare
|
All comments make sense, will address, next time you see this PR it will have the changed commit history |
dd5b0a5 to
2f7ab57
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
^ Will do monday. |
7dd8b9d to
93c55ad
Compare
|
Integration is really weird, at least 2-3 things related to the server itself PR should be ready |
jedevc
left a comment
There was a problem hiding this comment.
Only nits, happy to merge when they're resolved.
| RootfsTypeErofs RootfsType = "erofs" | ||
| RootfsTypeDir RootfsType = "dir" | ||
| // Unused for now, but may be useful in ROMs | ||
| RootfsTypeFile RootfsType = "file" |
There was a problem hiding this comment.
We should remove this for now if it's not being used.
| } else { | ||
| return BuildOpts{}, fmt.Errorf("unable to determine rootfs type for source %q", kf.Rootfs.Source) | ||
| sourcePath := filepath.Join(dir, kf.Rootfs.Source) | ||
| typ, err := DetectRootfsType(sourcePath) |
There was a problem hiding this comment.
Can you follow-up with a PR to x/kraftfile to add a field for this explicitly?
There was a problem hiding this comment.
shouldn't I do this before, so I can rebase this?
There was a problem hiding this comment.
I would keep the detection logic here for when that field is empty. Up to you on ordering, I have no preference.
5a0540f to
3bc965e
Compare
Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>
Implements 'volimport' instance lifecycle helpers. Also adds the communication protocol with the instance. Finally add a rootfs build helper for formatting properly. Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>
Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>
76a04f1 to
883907c
Compare
Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>
883907c to
eb29345
Compare
|
finally, it feels like 1 morbillion years have passed |
Signed-off-by: Justin Chadwell <justin@unikraft.com>
jedevc
left a comment
There was a problem hiding this comment.
Reviewed-by: Justin Chadwell justin@unikraft.com
Approved-by: Justin Chadwell justin@unikraft.com
This adds initial support for volume importing. As before, only CPIO files are supported for importing due to how the "backend" of this is built.
To add to this, only files and directories are supported right now. I could not figure out how to correctly split thedockerfileimplementation to make it stop before creating the OCI archive usingimagespec, as I needed it to give me a.cpiofile.Dockerfiles also work now.
To make this fully work,protoneeds to be updated and thedelete_on_stopflag needs to be renamed todelete-on-stop.Switched to new way of providing features that works.Sdk fixed.
Finally, I don't really like how things sit but I couldn't find a better way to break things apart whilst also keeping the general repo structure (aka not refactoring everything 😈)I did the refactoring hehe.
tldr: if you give it no--sourceit will use the workdir and assume it's a Kraftfile in there (or fail if not)Removed.
If you give it a
--sourceit uses that, be it a CPIO (directly uses it), or Directory (Packages it), or Dockerfile (Builds it).TODO: Regenerate tests before merging
Blocked on: https://github.com/unikraft-cloud/platform/pull/705
Closes: TOOL-694