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

Drop virtme-ng-init submodule and import commits instead #216

Merged
merged 112 commits into from
Jan 8, 2025

Conversation

matttbe
Copy link
Collaborator

@matttbe matttbe commented Jan 6, 2025

As discussed on #213, the virtme-ng-init submodule has been dropped, and the commits have been imported in the virtme_ng_init directory instead.

3 commits have been "manually" created here:

  • 89e8fa1 ("virtme-ng-init: remove the submodule"), using:
    git rm virtme_ng_init && git commit -s
  • d2b8a2b ("Add 'virtme_ng_init/' from commit '9b1e02a0deb094a36741b6172fb7ea8dc7dd8ddb'"), using:
    git subtree -P virtme_ng_init add [email protected]:arighi/virtme-ng-init.git main
  • 2c4fd14 ("gh: import and adapt rust workflow")

The git subtree add command automatically created as many commits as there are in the virtme-ng-init repo, making this PR a bit hard to review. I guess only the commands used above, and the last commit needs to be reviewed.

Note: Because I re-used the same directory as the one previously used by the submodule, I suspect people syncing with the new version might have to do an additional step (to be verified), e.g. this one?

git submodule deinit virtme_ng_init

arighi and others added 30 commits May 30, 2023 11:03
Signed-off-by: Andrea Righi <[email protected]>
Signed-off-by: Andrea Righi <[email protected]>
Setup basic building and testing automation.

Signed-off-by: Andrea Righi <[email protected]>
Allow to run graphical applications from virtme-ng.

Signed-off-by: Andrea Righi <[email protected]>
Update README.md with a note that this project has been merged into
virtme-ng.

Signed-off-by: Andrea Righi <[email protected]>
Always use `su` instead of `su -` when switching to the target user in
the guest, to preserve the same path of the host (unless specified
otherwise by command line arguments).

Signed-off-by: Andrea Righi <[email protected]>
There is a potential race between udev and network startup, the latter
could run too early, failing to detect a proper network interface and
failing to setup networking.

If that happens we need to wait a little bit to make sure udev has
registered corrected the virtio network device, and try again to setup
the network.

Moreover, refactor the networking code a little bit to make it more
readable.

Signed-off-by: Andrea Righi <[email protected]>
Enable all the required (kernel and user-space) features to start snapd
and execute snaps directly inside a virtme-ng instance.

The snap support is activated only if snapd is installed in the host
system. This won't introduce any additional overhead in systems that
don't have snap available.

In systems that have snapd available, virmte-ng will automatically start
it at boot (tricking it a bit into believing that it's running in a
standard systemd-enabled environment) and at this point we can start any
snap as regular system binaries inside virtme-ng.

This fixes issue arighi#21.

Open issues
===========

The main issue at the moment is that the guest requires read access to
/var/lib/snapd/state.json that has 0600 permissions by default. Since we
are running the guest as unprivileged user, by default it won't have
access to the snapd state, therefore snapd cannot be started in the
guest.

For this reason in order to properly support snapd inside virtme-ng
we need to change the permissions mask of this file and make it readable
by everyone (unless we figure out a better solution to this).

virtme-ng will print a warning in this case, to inform the user to
run chmod manually (sad...) to change the permission of state.json.

However, with this change applied we can run any kind of snap inside
virtme-ng, even graphic applications (using `--graphics`), such as
videogames, web browsers, etc.

Signed-off-by: Andrea Righi <[email protected]>
Considering that snap support is still experimental, provide an option
to enable it, instead of enabling it implicitly by default.

Signed-off-by: Andrea Righi <[email protected]>
XDG_RUNTIME_DIR is an environment variable that specifies the path to a
directory where user-specific runtime files should be stored. It is
defined by the X Desktop Group (XDG) Base Directory Specification, which
is a set of guidelines for storing user-specific files in a standardized
manner.

Initialize this working directory and the corresponding environment
variable when the graphic environment is enabled.

This allows better compatibility with certain graphic applications (such
as web browsers, videogames, etc.).

Signed-off-by: Andrea Righi <[email protected]>
Introduce helper script `virtme-sound-script` to guest tools to
automatically detect and start a valid sound subsystem when sound is
enabled (`--sound`).

NOTE: only pipewire is supported for now, support for other sound
subsystems can be added later modifying this script.

Signed-off-by: Andrea Righi <[email protected]>
Instead of showing a warning suggesting to run
`chmod +r /var/lib/snapd/state.json` when snaps are enabled we may try
to automatically give read access to state.json to the current user
using POSIX ACL (via getfacl / setfacl).

It's still potentially an unsafe operation in the system, so make sure
to print a big warning before changing any permission.

Even if it's still an ugly workaround to support snaps, it seems better
than asking users to run a `chmod +r` on a system file.

Signed-off-by: Andrea Righi <[email protected]>
Move all the commands required to initialize the system into
virtme-ng-init (or virtme-init).

Moreover, when running scripts inside the guest don't use /run as a
temporary place to store the script that needs to be executed, use /mnt
instead, because /run needs to be re-mounted with tmpfs by
virtme-ng-init or virtme-init.

This is the first step to provide an initramfs-less way of executing
scripts inside the guest, that should help to improve boot time even
more (for the script execution case).

Signed-off-by: Andrea Righi <[email protected]>
Instead creating an initramfs on-the-fly to store the --exec command(s)
that need to be executed in the guest, pass them to the guest via
/proc/cmdline, using the kernel boot parameter virtme.exec=`<command>`.

This allows to save some extra boot time, because we don't have to
create an initramfs on-the-fly on the host and use it to boot the guest.

Signed-off-by: Andrea Righi <[email protected]>
The flags in MountInfo should be a 32-bit unsigned on 32-bit
architectures and 64-bit unsigned on 64-bit architectures.

Redefine it accordingly.

Signed-off-by: Andrea Righi <[email protected]>
This allows to build virtme-ng-init also on 32-bit architectures.

Signed-off-by: Andrea Righi <[email protected]>
We don't require any specific version of nix and users crate, so let's
just relax their version constraints.

Signed-off-by: Andrea Righi <[email protected]>
Signed-off-by: Andrea Righi <[email protected]>
Signed-off-by: Andrea Righi <[email protected]>
Also run the workflows on any push and pull request.

Signed-off-by: Andrea Righi <[email protected]>
Decode base64 shell commands passed via virtme.exec=...

Encoding the shell command in base64 allows to pass commands that
contain special characters, such as double quotes, single quotes, etc.

Signed-off-by: Andrea Righi <[email protected]>
Use fixed version of dependent crates to make sure virtme-ng-init always
builds.

Signed-off-by: Andrea Righi <[email protected]>
Prevent crashing init when an external command cannot be executed,
instead fail gracefully logging a warning.

Signed-off-by: Andrea Righi <[email protected]>
Support long commands using the "--" syntax also when running virtme-ng
in graphic mode.

Signed-off-by: Andrea Righi <[email protected]>
When virtme_user is specified in the boot options we should also honor
it when running scripts, instead of just using it in interactive mode.

Signed-off-by: Andrea Righi <[email protected]>
I looked for functions that returned Option like get_user_id() before
this commit. This function calls some other function, check if it
returns a value (Option::Some), and then does something with this value.
If a None was seen, a None is explicitly returned.

This can be simplified with the question mark operator. By using ? on
the Option, the function automatically returns None if a None is seen. I
think this is a bit more readable (although perhaps less beginner
friendly since this might read like magic).

Some functions did similar things but with a Result. Here, I used .ok()?
to get the desired behaviour. I still feel like this is simpler.

Signed-off-by: Uli Schlachter <[email protected]>
arighi and others added 7 commits November 27, 2024 14:36
When virtme.vsockexec=`<cmd>` is set, socat is started in the
background, listening to a VSock connection on the port 1024: once
connected, a pty console is started with the given <cmd>, e.g. 'bash
-i'.

This allows a simple remote control.

Link: arighi#151
Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
vsock: socat service for remote console
This will be used to get access to a script that will setup the remote
shell: HOME dir, log with the right user, set tty, etc.

This script will need to be created when a new connection is needed, to
get access to the terminal dimension, and maybe more. Because of that,
it is needed to have a way to share such info from the host to the VM.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
vsock: mount virtme_vsockmount if needed
Provide an option to start sshd in the guest. The sshd init script will
be provided as a guest script by virtme-ng (if not present `virtme.ssh`
will be simply ignored).

Signed-off-by: Andrea Righi <[email protected]>
@matttbe matttbe requested a review from arighi January 6, 2025 10:35
matttbe and others added 2 commits January 8, 2025 15:13
It looks like the submodule is causing more issues than helping:

- Modifying this subproject requires two separated commits:
  - It might delay things
  - When only the init part is modified, it might be unclear how the
    modifications will be used, abd discussions about changing something
    on the main project can be easily forgotten
  - Small modifications might not be done because it needs more work,

- People using the git repo often forget to sync, and have "strange"
  issues

- It might be harder to package

- Etc.

In this commit, the submodule is removed. The code will be re-imported
after this commit.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
Merge all the history of virtme-ng-init directly into the virtme-ng
repository.

Commands used to perform the merge:

 $ git clone [email protected]:arighi/virtme-ng-init.git temp
 $ cd temp
 $ git filter-repo --to-subdirectory-filter virtme_ng_init
 $ cd ..
 $ git remote add temp temp
 $ git merge --allow-unrelated-histories temp/main
 $ rm -rf temp

Signed-off-by: Andrea Righi <[email protected]>
@arighi
Copy link
Owner

arighi commented Jan 8, 2025

Hi @matttbe thanks for looking at this! I would be even more aggressive, import/rewrite all the commits and forget about the external virtme-ng-init repo.

I've tried to do some experiments and I came up with this:
https://github.com/arighi/virtme-ng/tree/arighi/vng-init-merge

This is what I've done, I cherry picked your first commit, then:

$ git clone [email protected]:arighi/virtme-ng-init.git temp
$ cd temp
$ git filter-repo --to-subdirectory-filter virtme_ng_init
$ cd ..
$ git remote add temp temp
$ git merge --allow-unrelated-histories temp/main
$ rm -rf temp

The thing that I like a lot about this is that we can git log virtme_ng_init and see the history of virtme-ng-init, like the real history, in particular we can see the real contributor of each commit (instead of the author that updated the submodule), which I think is eally nice. Also, in this way you last commit gh: import and adapt rust workflow is not needed (IIUC), we can archive/close virtme-ng-init and do all the development here.

What do you think?

@matttbe
Copy link
Collaborator Author

matttbe commented Jan 8, 2025

Hi @matttbe thanks for looking at this! I would be even more aggressive, import/rewrite all the commits and forget about the external virtme-ng-init repo.

I've tried to do some experiments and I came up with this: https://github.com/arighi/virtme-ng/tree/arighi/vng-init-merge

This is what I've done, I cherry picked your first commit, then:

$ git clone [email protected]:arighi/virtme-ng-init.git temp
$ cd temp
$ git filter-repo --to-subdirectory-filter virtme_ng_init
$ cd ..
$ git remote add temp temp
$ git merge --allow-unrelated-histories temp/main
$ rm -rf temp

The thing that I like a lot about this is that we can git log virtme_ng_init and see the history of virtme-ng-init, like the real history, in particular we can see the real contributor of each commit (instead of the author that updated the submodule), which I think is eally nice.

Oh, I'm surprised the result is not the same for the git log virtme_ng_init command.

With git subtree, the good thing is that we can keep the same SHA to follow the differences that were done on the submodule. But I understand we might not need that, and we might prefer to look at history with just git log virtme_ng_init. Also, we will not need to do any sync later on, so we don't need the exact same SHA. It is of course fine for me to use your technique!

Also, in this way you last commit gh: import and adapt rust workflow is not needed (IIUC), we can archive/close virtme-ng-init and do all the development here.

I think it is still needed to move the workflow file from .virtme_ng_init/.github to .github, no? Or can you keep it in a subdirectory (virtme_ng_init) as it is in your version?

Do you plan to cherry-pick the last commit and open a PR replacing this one? (you can also override my branch if you prefer)

Two modifications have been done:

- Restrict the build on push to the main branch only, similar to what
  was done in commit 4da53c3 ("github: restrict builds on push to the
  main branch")

- Change the working directory to ./virtme_ng_init, because the root
  directory is now different.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
@arighi
Copy link
Owner

arighi commented Jan 8, 2025

Also, in this way you last commit gh: import and adapt rust workflow is not needed (IIUC), we can archive/close virtme-ng-init and do all the development here.

I think it is still needed to move the workflow file from .virtme_ng_init/.github to .github, no? Or can you keep it in a subdirectory (virtme_ng_init) as it is in your version?

Do you plan to cherry-pick the last commit and open a PR replacing this one? (you can also override my branch if you prefer)

Ah! You are absolutely right! I cherry-picked your last commit and updated my branch.

@arighi
Copy link
Owner

arighi commented Jan 8, 2025

I really like that we can preserve the git history with the original authors and it'd be nice to keep this PR instead of opening a new one, maybe it's easier if I just force push to your branch?

@matttbe
Copy link
Collaborator Author

matttbe commented Jan 8, 2025

maybe it's easier if I just force push to your branch?

Yes, that sounds better. You should have the right to modify my branch on my repo, no?

@arighi
Copy link
Owner

arighi commented Jan 8, 2025

maybe it's easier if I just force push to your branch?

Yes, that sounds better. You should have the right to modify my branch on my repo, no?

Ah no, I don't think so, because the branch is in your workspace.

BTW maybe at some point we should also move the repo to https://github.com/virtme/ so that we can all create branches and stuff in the same workspace.

@arighi
Copy link
Owner

arighi commented Jan 8, 2025

@matttbe maybe you can force push my branch to your branch and then we can merge :)

@arighi arighi force-pushed the merge-vng-init-submodule branch from 2c4fd14 to 360894c Compare January 8, 2025 20:42
@arighi
Copy link
Owner

arighi commented Jan 8, 2025

oh wait, nevermind, I just tried and it worked, I just forced pushed to your branch... what? :-o

@matttbe
Copy link
Collaborator Author

matttbe commented Jan 8, 2025

It looks like it worked 🙂

BTW maybe at some point we should also move the repo to https://github.com/virtme/ so that we can all create branches and stuff in the same workspace.

Yes, good idea! (Or virtme-ng)
With the migration option in the repo, people will be redirected to the new repo, so it sounds safe.

@matttbe
Copy link
Collaborator Author

matttbe commented Jan 8, 2025

oh wait, nevermind, I just tried and it worked, I just forced pushed to your branch... what? :-o

Yes, that's linked to the "Allow edits and access to secrets by maintainers" option 🙂

@arighi
Copy link
Owner

arighi commented Jan 8, 2025

oh wait, nevermind, I just tried and it worked, I just forced pushed to your branch... what? :-o

Yes, that's linked to the "Allow edits and access to secrets by maintainers" option 🙂

I was a bit surprised to see that I could just destroy your work lol.

About moving the project to a virtme-ng organization, I didn't know there was a migrate option, that's very interesting! I think it'd make more sense to migrate the project outside of my personal workspace, considering that we have multiple contributors now. I'll take a look!

@matttbe
Copy link
Collaborator Author

matttbe commented Jan 8, 2025

I was a bit surprised to see that I could just destroy your work lol.

I still have a local copy 🙂

@arighi: before merging, did you have issues to switch to my branch when you had the submodule still configured? Just to know if we have to tell people what to do? 🙂

@arighi
Copy link
Owner

arighi commented Jan 8, 2025

@arighi: before merging, did you have issues to switch to my branch when you had the submodule still configured? Just to know if we have to tell people what to do? 🙂

Ah yes, good point, I had to manually remove the virtme_ng_init dir and reset to your branch. Probably people will need to do the same with this PR. I'm not sure if there's a clean way to prevent this... I guess people will just have to fix this manually (or clone the repo again).

@matttbe
Copy link
Collaborator Author

matttbe commented Jan 8, 2025

@arighi: before merging, did you have issues to switch to my branch when you had the submodule still configured? Just to know if we have to tell people what to do? 🙂

Ah yes, good point, I had to manually remove the virtme_ng_init dir and reset to your branch. Probably people will need to do the same with this PR. I'm not sure if there's a clean way to prevent this... I guess people will just have to fix this manually (or clone the repo again).

Should I then just merge and see? 🙂

If someone has the issue, maybe the proper solution is to execute:

git submodule deinit virtme_ng_init

@arighi
Copy link
Owner

arighi commented Jan 8, 2025

Let's go ahead and merge, people will figure out what to do. If not they can ask. :)

@matttbe matttbe merged commit 07b109d into arighi:main Jan 8, 2025
7 checks passed
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.

7 participants