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

Create VMInstance class #827

Closed
wants to merge 1 commit into from
Closed

Create VMInstance class #827

wants to merge 1 commit into from

Conversation

gerboland
Copy link
Contributor

@gerboland gerboland commented Jun 7, 2019

I want to share my thinking into a refactor.

We currently keep a lot of state and operations code related to VMs in the Daemon object. For instance state, i.e. preparing a VM, VM being deleted, VM shutdown delay, we track that state by moving the instance name between different sets/maps. Operations code are mostly platform-independent VM-control methods (start/shutdown/reboot/install_sshfs/mounts).

I'm in a situation where I want to add another per-VM operation (a maintenance operation which will fire on a timer while VM running). It is a platform independent op, so doesn't belong in the VirtualMachine implementations themselves. Right now, platform independent VM ops are private methods in Daemon itself. So adding this new op means I'll have to add another list to Daemon, and carefully add/remove from the list in sync with the VM state - clunky.

My thinking is to introduce a "VMInstance" class, which will hold per-VM state and platform independent operations (i.e. start/shutdown/reboot/install_sshfs/mounts).

I hope to replace the state sets/maps in Daemon with VMInstance::state() which for example would return Prepared/Exists/Deleted/ - I've to decide if the actual VM state should be included too.

And I also want to move the ops like start/shutdown/reboot/install_sshfs/mounts our of Daemon and create as members of this.

This PR is the first step on this journey, and make it easier to add my per-VM operation.

What do you folks think? Right direction or no?

@townsend2010
Copy link
Contributor

I think this is a really good idea. The growing list of unordered_maps is getting a bit large along with all of the things the daemon itself is responsible for regarding instances. The daemon code itself is simply getting too large and starting to refactor it in a way like this where another object is charge of the instance is a good thing.

I will look over the code some more, but on the first pass, it looks quite reasonable.

@gerboland
Copy link
Contributor Author

To give you idea of where this brings us, this is how VMInstances looks when I move more related bits out of Daemon:
https://pastebin.ubuntu.com/p/JBvBK5Nkxq/

@townsend2010
Copy link
Contributor

That should also make this much easier to test too as we'd only need to worry about this class and no need to mock so many other parts of the daemon.

@ricab
Copy link
Collaborator

ricab commented Jun 11, 2019

Yes, this sounds like a good idea to me too. Thanks!

Base automatically changed from master to main March 3, 2021 13:41
@gerboland gerboland closed this May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants