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

Add terminal-only appvms #20

Merged
merged 1 commit into from
Apr 5, 2020
Merged

Conversation

msm-code
Copy link
Contributor

@msm-code msm-code commented Apr 5, 2020

  • I tested it locally.
  • I tried to run at least one application VM and it works.

Add terminal-only appvms

I hope this change is not too controversial.

Sometimes I want to run a VM without a window running in the foreground.
For example, a router/firewall VM, temporary VM for compiling some untrusted
code or installing a random package from the internet. Full GUI is usually
unnecessary, since it's much nicer to be able to use host's terminal directly.

In this change I add --cli switch that disables GUI window.

Initially I also added serial device to domain (this allowed automatic connect
with virsh -c qemu:///system console appvm_name), but I dropped it before
making a PR - adding a SSH server to appvm definition works better for me.

I also change networking model from qemu to virtmanager's device (virbr0).
The reason for this is that it's much easier for me to configure firewall/
iptables with virt manager network, than with qemu device. Also AppVMs
don't have IPs with qemu device, I think. If this change is not OK I'm
happy to keep this change only in my fork (or maybe as a switch
--networking-model?)

Thoughts?

@jollheef
Copy link
Owner

jollheef commented Apr 5, 2020

Hello,

Thanks for the pull request.

All those changes are fit the initial idea, so does not looks controversial for me.

Regarding the networking model, I think introducing switch --networking-model is the best way to make it right.

In general, looks good to me. However, aside from introducing switch, please make each feature in separate commits and rewrite the commit message (too much unnecessary info).

@jollheef jollheef mentioned this pull request Apr 5, 2020
@msm-code
Copy link
Contributor Author

msm-code commented Apr 5, 2020

Sure, making two separate changes makes a lot of sense.

Regarding the networking model, I think introducing switch --networking-model is the best way to make it right.

It doesn't make sense to have both --offline and --networking-model so maybe they should be merged? What do you think about it taking a role of a current --offline switch? So you'd have:

--networking=qemu
--networking=libvirt
--networking=offline  # or =none?

Otherwise I can just throw an error where both are specified.

@jollheef
Copy link
Owner

jollheef commented Apr 5, 2020

I don't think it's a problem to have some duplicity. Also for some people (like me also) type --offline just more convenient.

So it's better to throw an error if someone has specified --networking=!offline and at the same time used --offline.

@msm-code
Copy link
Contributor Author

msm-code commented Apr 5, 2020

I agree that --offline is more convenient than long --networking=offline. I'll add this networking changes in a separate PR then.

@jollheef jollheef merged commit e588479 into jollheef:master Apr 5, 2020
@msm-code msm-code deleted the feature/cli-devices branch April 6, 2020 00:15
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.

None yet

2 participants