Refactor urunc to enhance code organization and maintainability #11
Conversation
ananos
left a comment
ananos
left a comment
There was a problem hiding this comment.
Looks complete and thorough. Left a few minor comments in the code.
General comments:
- we need to run the tests somehow -- I missed how we run them. Is it because we need support from our runner infrastructure?
- I understand this is the first step for refactoring and we need another iteration to add already existing functionality in a more clean way. It would be really helpful to provide something like a changelog? known issues addressed and pending ? not sure, but we definitely need some way to track the changes introduced with this PR.
| @@ -4,7 +4,6 @@ on: | |||
| push: | |||
There was a problem hiding this comment.
how about triggering a build on PR? I understand the rationale of building & pushing only on the main branch, but we need somehow to test the binary of the PR before merging. Is it handled in a different workflow?
| Log.WithField("args", os.Args).Info("urunc INVOKED") | ||
| // to previous line | ||
|
|
||
| root := "/run/urunc" |
There was a problem hiding this comment.
we need to verify that mount options match what we want (eg. if we need the exec flag enabled). If by default they are not, we should do what kata does (remount/bind mount etc.)
Additionally, we should add a comment about the fact that we fill up /run with unikernel binaries (unless this is resolved so please just ignore this)
There was a problem hiding this comment.
Usually, when executing a unikernel container, we have to deal with 2 different container directories.
The first one is this root directory (/run/containerd/runc/<NAMESPACE> or the default value /run/urunc) usually passed down to us from the containerd-shim. Inside this directory, resides a child directory for each container (/run/containerd/runc/<NAMESPACE>/<CONTAINER_ID>). We use that container-specific directory to store our state.json file.
The second one (which is usually the cwd of the urunc process) is the bundle path and (in most cases) is /run/containerd/io.containerd.v2.task/<NAMESPACE>/<CONTAINER_ID>. This is where our rootfs, init.pid etc are stored.
So, as far as I understand, the root directory is not mounted.
The comment about filling up /run is definitely needed, I will add it to the relevant function.
docs/Installation.md
Outdated
| git clone -b v0.6.9 https://github.com/Solo5/solo5.git | ||
| cd solo5 | ||
| ./configure.sh && make -j$(nproc) | ||
| sudo mv tenders/hvt/solo5-hvt /usr/local/bin |
docs/Installation.md
Outdated
| cd solo5 | ||
| ./configure.sh && make -j$(nproc) | ||
| sudo mv tenders/hvt/solo5-hvt /usr/local/bin | ||
| sudo chmod +x /usr/local/bin/solo5-hvt |
There was a problem hiding this comment.
if we build it, we don't need this
There was a problem hiding this comment.
Thanks for pointing out. Done.
| "github.com/vishvananda/netlink" | ||
| ) | ||
|
|
||
| const DefaultInterface = "eth0" |
There was a problem hiding this comment.
Add a FIXME comment about supporting more interfaces, maybe point it to the relevant issue (if we don't have, we should create one). The same stands for the name of the veth container endpoint (eth0) -- maybe we need to discover that (again, as the kata people do, by examining the flags of all interfaces in the netns (https://github.com/kata-containers/kata-containers/blob/3b2fb6a604a624f1ec0c21a85d6e4182344aac4c/src/runtime/virtcontainers/network_linux.go#L337)
| } | ||
|
|
||
| // GetUnikernelConfig tries to get the Unikernel config from the bundle annotation. | ||
| // GetUnikernelConfig tries to get the Unikernel config from the bundle annotations. |
There was a problem hiding this comment.
add a comment about the annotations and also point to the relevant issue (if we don't have one, we should create one)
| }, nil | ||
| } | ||
|
|
||
| mask, err := subnetMaskToCIDR(data.EthDeviceMask) |
There was a problem hiding this comment.
add the following comment:
FIXME: in the case of rumprun & k8s, we need to explicitly set the mask to an inclusive value (eg 1 or 0), as NetBSD complains and does not set the default gw if it is not reachable from the IP address directly.
|
@ananos, thank you for your time and attention!
Thanks for the comments. I tried to address all of them.
The only way to run the tests currently is manually. We should definitely work out a way to run them in an automated way. Perhaps spawn a clean VM, install all neccessary components and run them there?
For the most part, this PR doesn't alter the functionality of the previous |
|
Overall, it seems good. I have added some comments. The majority of my comments are not crucial and can be ignored if we want to. I would definitely agree with a changelog file. We can add information, such as code restructuring and some of the fixes that take place in every release. |
|
|
||
| // AwaitAckReexec opens a new connection to UruncSock and | ||
| // waits for a AckReexec message | ||
| func (u *Unikontainer) AwaitAckReexec() error { |
There was a problem hiding this comment.
Same comment as AwaitMessageStarted. I think awaitMessage is the right abstraction. If it is necessary we can simply move the check for the socket inside the awaitMessage function
|
|
||
| // AwaitStartExecve opens a new connection to UruncSock and | ||
| // waits for a StartExecve message | ||
| func (u *Unikontainer) AwaitStartExecve() error { |
There was a problem hiding this comment.
Same as previous two functions.
| } | ||
|
|
||
| // SendReexecStarted sends an ReexecStarted message to InitSock | ||
| func (u *Unikontainer) SendReexecStarted() error { |
There was a problem hiding this comment.
Same as the above await-functions.
| } | ||
|
|
||
| // SendAckReexec sends an AckReexec message to UruncSock | ||
| func (u *Unikontainer) SendAckReexec() error { |
There was a problem hiding this comment.
Same as the above await-functions. I am mostly referring to the abstraction.
| } | ||
|
|
||
| // SendStartExecve sends an StartExecve message to UruncSock | ||
| func (u *Unikontainer) SendStartExecve() error { |
gntouts
force-pushed
the
refactor_pr
branch
2 times, most recently
from
55ac074 to
633825c
Compare
- Move urunc cmd tool code under cmd directory. - Introduce the 'unikontainers' package to separate urunc cmd tool from the underlying logic responsible for handling unikernel containers. - Separate hypervisor and unikernel functionality into distinct packages. - Update solo5-hvt to v0.6.9 - Rewrite IPC mechanism to allow for retrying failed communication attempts. - Use a runc-compatible logging configuration - Bump urunc version to 0.2.0 - Minor fixes Signed-off-by: Georgios Ntoutsos <gntouts@nubificus.co.uk> Reviewed-by: Anastassios Nanos <ananos@nubificus.co.uk> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Anastassios Nanos <ananos@nubificus.co.uk> PR: #11
- Add end to end tests for hvt/rumprun unikernels using ctr, nerdct, crictl - Update which branches trigger workflow runs Signed-off-by: Georgios Ntoutsos <gntouts@nubificus.co.uk> Reviewed-by: Anastassios Nanos <ananos@nubificus.co.uk> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Anastassios Nanos <ananos@nubificus.co.uk> PR: #11
Signed-off-by: Georgios Ntoutsos <gntouts@nubificus.co.uk> Reviewed-by: Anastassios Nanos <ananos@nubificus.co.uk> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Anastassios Nanos <ananos@nubificus.co.uk> PR: #11
- Move urunc cmd tool code under cmd directory. - Introduce the 'unikontainers' package to separate urunc cmd tool from the underlying logic responsible for handling unikernel containers. - Separate hypervisor and unikernel functionality into distinct packages. - Update solo5-hvt to v0.6.9 - Rewrite IPC mechanism to allow for retrying failed communication attempts. - Use a runc-compatible logging configuration - Bump urunc version to 0.2.0 - Minor fixes Signed-off-by: Georgios Ntoutsos <gntouts@nubificus.co.uk> Reviewed-by: Anastassios Nanos <ananos@nubificus.co.uk> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Anastassios Nanos <ananos@nubificus.co.uk> PR: #11
- Add end to end tests for hvt/rumprun unikernels using ctr, nerdct, crictl - Update which branches trigger workflow runs Signed-off-by: Georgios Ntoutsos <gntouts@nubificus.co.uk> Reviewed-by: Anastassios Nanos <ananos@nubificus.co.uk> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Anastassios Nanos <ananos@nubificus.co.uk> PR: #11
This PR includes a complete code refactoring for
uruncand enhances the codebase in various ways:Code Refactoring and Organization:
Dependency Updates:
solo5-hvtto v0.6.9.Testing and Documentation:
ctr,nerdctl, andcrictl.Minor Fixes: