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

Creating and running the VM via quickemu #26

Merged
merged 9 commits into from
Oct 9, 2023

Conversation

oskardotglobal
Copy link
Member

I began working on this (it isn't complete yet), but wanted to already open a PR to get some feedback and to discuss some stuff (and to check if pre-commit.ci will check this).

At first, I was worried that we would have to daemonize the VM but this isn't needed because quickemu runs the VM in the background. Sadly, there's no easy way to send shell commands, so we're best off creating our own scripts to replace quickget and then install some kind of ssh server.

After running quickget, you get a folder structure which contains an "unattended" folder used for unattended install, which contains a file "unattended.xml". We can use this to run commands on startup.

@oskardotglobal oskardotglobal added enhancement New feature or request rewrite Using the winapps rewrite priority: medium labels Jul 27, 2023
@oskardotglobal
Copy link
Member Author

pre-commit.ci run

@LDprg
Copy link
Member

LDprg commented Jul 27, 2023

You have written the quick emu installer script in python. As we will need propably some more installer scripts, I would prefer to select a uniform programming language for all such scripts. So we should discuss which language we should select.

I am personal fine with python (easy and fast script writing). The only downside is that integrating the installer into the winapps executable would be easier if it is also written in rust.

@oskardotglobal
Copy link
Member Author

pre-commit.ci run

@oskardotglobal
Copy link
Member Author

You have written the quick emu installer script in python.

Yeah, this is just a temporary solution to test on several machines with. There's also a nix package available for quickemu, but I don't know how reliable that is dependency-wise. However, if that works, we could just distribute winapps itself over nix, which would be the easiest way to start with (also mentioned that in #23)

@oskardotglobal
Copy link
Member Author

Also, I wanted the quickemu install script to be independant for now since we can always link to it for people who are having trouble with the legacy way of setting up the VM

Copy link
Member

@LDprg LDprg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pub(crate) fn get_data_dir() in the winapps/src/quickemu.rs file seems a bit strange. I think we can just change it to pub fn get_data_dir().

winapps/src/quickemu.rs Outdated Show resolved Hide resolved
@oskardotglobal oskardotglobal marked this pull request as draft July 30, 2023 17:22
@LDprg
Copy link
Member

LDprg commented Aug 8, 2023

For controlling the vm with quickemu are we aiming for a demon like process?

@oskardotglobal
Copy link
Member Author

Quickemu doesn't need a daemon
Execute the command, the VM starts.

@AkechiShiro
Copy link

@oskardotglobal Does quickemu shutdown the VM once you quit the application or does it snapshot the state and writes it to disk to avoid rebooting it ?
Should I peek into the code of quickemu or do you happen to know ?

Copy link

@AkechiShiro AkechiShiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my first review ever, so if you believe any of my suggestion aren't necessary feel free to let me know, I can also work on them on this draft PR, if you're too busy to apply these changes.

Thanks for your draft work on this, it looks pretty good as a base.

scripts/install_quickemu Outdated Show resolved Hide resolved
winapps/src/quickemu.rs Outdated Show resolved Hide resolved
scripts/install_quickemu Show resolved Hide resolved
@oskardotglobal
Copy link
Member Author

@oskardotglobal Does quickemu shutdown the VM once you quit the application or does it snapshot the state and writes it to disk to avoid rebooting it ? Should I peek into the code of quickemu or do you happen to know ?

I don't know, it fully boots when you start up the VM but it boots up very quickly so there might be some pausing going on

@AkechiShiro
Copy link

I just saw an issue affect quickemu breaking W10 VMs, we might want to watch it out, there is a workaround but no upstream fix yet, I believe : quickemu-project/quickemu#654

@LDprg
Copy link
Member

LDprg commented Sep 4, 2023

@AkechiShiro It seems like this issue is only when upgrading quickemu. However it seems like it was one time event and does not affect new installs.

@LDprg
Copy link
Member

LDprg commented Sep 4, 2023

Quickemu doesn't need a daemon
Execute the command, the VM starts.

@oskardotglobal But does the vm start in background without any window?

The idea was to run the vm in headless mode, after the vm setup. However to make the vm run in background we may need a demon that does stuff like shutdowns, reboots or startups (on deman or at boot). If we have no demon we would (if I am not wrong) have a floating spice or terminal window.

@AkechiShiro
Copy link

AkechiShiro commented Sep 4, 2023

I do agree with @LDprg for having tested quickly quickemu a terminal windows or the spice floating windows will be shown, I do not know if quickemu can be launched headless using a specific flag or some special configuration files

But that daemon is already libvirtd systemd service no ?
I believe we can start a VM Headless that way but I'm not sure how this would integrate with quickemu

@LDprg
Copy link
Member

LDprg commented Sep 4, 2023

@AkechiShiro quickemu can be launched in a headless with --display=none. However we would need to ensure that the shell environment where it was launched keeps running, so the vm would not just quit when the winapps command finishes.

Therfore I suggested a custom demon, because quickemu not directly uses the libvirt demon (you cant see quickemu in virt manager). It seems like it only uses its interface/libary. Quickemu simply runs as long as the display app or the terminal command (in headless mode). At least that is what I think it is doing based on some testing.

@AkechiShiro
Copy link

@LDprg do you have any idea of a daemon library/crate in Rust that we could use, or should I try and search for that ?

@LDprg
Copy link
Member

LDprg commented Sep 5, 2023

Daemonize seems like a good option.

@AkechiShiro
Copy link

AkechiShiro commented Sep 5, 2023

It looks a bit abandoned in terms of maintainship @LDprg (last commit to main/master 5 years ago, this doesn't look good to rely on such, unless we plan to maintain a fork ourselves)

And I thought that the daemon would be in Rust, not in Python, but I believe we will have a blend of both in the final software, perhaps ?

Maybe, we could have some input from @oskardotglobal as well, if he agrees to go with this ? Or if he feels this isn't necessary, why ?

@AkechiShiro
Copy link

I would recommend we'd use something like this @LDprg https://pagure.io/python-daemon/commits/main, last commit (6 months ago)

But there might be better solution

@LDprg
Copy link
Member

LDprg commented Sep 5, 2023

@AkechiShiro The libary is for Rust, there is a python libary out there which has the same name and a similar interface. For the daemon I would use some compiled language because of the performance overhead.

@AkechiShiro
Copy link

@LDprg you meant thus that we'd use this : https://docs.rs/daemonize/latest/daemonize/

@LDprg
Copy link
Member

LDprg commented Sep 5, 2023

yes

@oskardotglobal
Copy link
Member Author

Daemonize looks perfect; from what I understand this daemonizes the current process though, right? I would just suggest daemonizing the winapps-created vm directly via the OS init system (systemd, rc.d, openrc)

@oskardotglobal
Copy link
Member Author

Looking into this again realizing we don't need a daemon; if you run quickemu with --display none the process runs in the background
we just need to ideally store the PID so we can shut down the process if required later on

@oskardotglobal
Copy link
Member Author

So turns out quickemu just provides a pidfile, so I've now implemented killing; the question remains if that is the proper way to shut down the vm

@oskardotglobal oskardotglobal marked this pull request as ready for review September 28, 2023 15:48
Copy link

@AkechiShiro AkechiShiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's looking pretty good so far, do we need to test this branch on our system before merging ?

Thanks for the amazing work @oskardotglobal

Anything you'd like to mention @LDprg ?

@oskardotglobal
Copy link
Member Author

Yeah, It'd be a good idea if at least one of you builds and tests it locally, because on my fedora 37 system XDG_CONFIG_HOME and XDG_DATA_HOME are unset, so I don't know if they work.

@AkechiShiro
Copy link

AkechiShiro commented Oct 3, 2023

@oskardotglobal is vm creation working on your side ?

I tested the branch, it worked alright but there were a few issues :

  • I've added one more distro, ArchLinux for the Python quick install script,
  • Some commands of the CLI needs more work
    • the winapps-cli check -> xfreerdp seems to error at the end ?
      image

    • winapps-cli vm create, should give the user a status of the download of the Windows ISO file because it is very long (5.7GB ISO), a user could think the program froze or something, we need some kind of progress bar there (I'll try and work on it).

      • At the end of creating the VM, it seems to crash :
Couldn't read XDG_CONFIG_HOME, falling back to ~/.config
Creating windows 10 vm..
Couldn't read XDG_DATA_HOME, falling back to ~/.local/share
Couldn't read XDG_CONFIG_HOME, falling back to ~/.config
thread 'main' panicked at 'Failed to save config, VM will not start properly: Os { code: 9, kind: Uncategorized, message: "Bad file descriptor" }', winapps/src/quickemu.rs:26:32
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_fmt
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/panicking.rs:67:14
   2: core::result::unwrap_failed
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/result.rs:1651:5
   3: winapps::quickemu::create_vm
   4: winapps_cli::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Not sure why, but I believe, some debugging is needed to get to the root of the issue here, there is something wrong with the save_config/load_config function ?

  • The config file (.toml) do exist ~/.config/winapps/config.toml and has default values and the directories winapps were properly created.

Future upgrades too :

  • Add a parameter for vm creation that allows to customize where the ISO is stored, however this path would need to be stored inside the config.toml file I believe

@oskardotglobal
Copy link
Member Author

@AkechiShiro Check is not implemented yet, @LDprg is working on that. The panic you're getting means that the config file couldn't be written or serialized which might have to do with file permissions or the ~/.config/winapps folder not being created properly

@LDprg
Copy link
Member

LDprg commented Oct 3, 2023

@AkechiShiro maybe just remove the config, so it gets recreated. The check command is only partially implemented, so it may not work due to some testing code.

@AkechiShiro
Copy link

AkechiShiro commented Oct 3, 2023

I removed the config file and starting the VM works, but we need to do some work to automatically do the installation, perhaps ?
I'll need to experiment with this : https://medium.com/@santimar/windows-unattended-installation-from-an-iso-file-to-a-configured-machine-73d386593136
Using something (unattended.xml ?) that could allow us to automatically installed Windows in default manner.

I've seen also quick warnings on two things :

  1. Maybe we can, ignore msrs by default, I don't think it's an issue, but it spams dmesg kernel logs.
 - MSR:      WARNING! Ignoring unhandled Model-Specific Registers is disabled.

             echo 1 | sudo tee /sys/module/kvm/parameters/ignore_msrs

             If you are unable to run macOS or Windows VMs then run the above 👆
             This will enable ignoring of unhandled MSRs until you reboot the host.
             You can make this change permenant by running: 'quickemu --ignore-msrs-always'
  1. I think this issue may come from quickemu and we might need to see if there is something upstream about this, I didn't find any information upstream about this.
qemu-system-x86_64: -no-hpet: warning: -no-hpet is deprecated, use '-machine hpet=off' instead

Tested so far :

  • vm
    • start
    • kill
    • create (crashed due to already existing config file)
    • help
  • check
  • connect
  • help

@oskardotglobal
Copy link
Member Author

@LDprg can you re-review this so we can get it merged?

@oskardotglobal oskardotglobal mentioned this pull request Oct 8, 2023
Copy link
Member

@LDprg LDprg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Just some minor changes.

scripts/install_quickemu Outdated Show resolved Hide resolved
winapps-cli/src/main.rs Show resolved Hide resolved
winapps/src/quickemu.rs Show resolved Hide resolved
@oskardotglobal oskardotglobal merged commit d2bc015 into winapps-org:rewrite Oct 9, 2023
3 checks passed
LDprg added a commit that referenced this pull request Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: medium rewrite Using the winapps rewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants