intel / ccloudvm

Configurable Cloud VM is a small command line tool for automatically creating development and demo environments for complex projects. The tool sets up these development environments inside a virtual machine which it automatically creates on the user’s host computer. This avoids polluting the user’s host machine with components from the chosen development environment and provides a clean, predictable and repeatable environment in which this development environment can run.
Apache License 2.0
32 stars 19 forks source link

Support multiple instances #51

Closed markdryan closed 6 years ago

markdryan commented 6 years ago

This PR introduces multiple instances into ccloudvm. It also unfortunately, breaks backward compatibility with the old CLI.

ccloudvm now contains a setup command which must be run before running any other ccloudvm command. This command sets up a systemd user service, called ccloudvm. The old ccloudvm executable still exists but now simply ensures the systemd service is running and delegates almost all tasks to these service. The service is necessary as now we have multiple instances we can have multiple things happening at the same time so we need a service to synchronise everything. There's also a new command called teardown which removes the ccloudvm service and deletes any existing instances.

Once you've run cclouvm setup you can use all the old ccloudvm commands in much the same way as you did in the past, providing you only have one instance. If you have multiple instances you now need to specify the instance name. All instances now have names. If you don't explicitly assign a name to an instance at creation time one will be created for you. For example,

I can do

ccloudvm setup ccloudvm create xenial ccloud status ccloudvm connect ccloudvm delete

as before, but if I create a second instance, e.g., by

ccloud create --name second xenial

I now need to pass the instance name to any ccloudvm commands I issue, e.g.,

ccloudvm stop second.

There's one exception to this rule. The run command always requires the instance name to be specified even if there is only one instance.

There's also a new command,

ccloudvm instances

which provides information about all the existing instances.

Fixes: https://github.com/intel/ccloudvm/issues/3

markdryan commented 6 years ago

@ganeshmaharaj , @mcastelino , @grahamwhaley , @devimc , @rbradford, @kaccardi could I ask you guys to try out the new look ccloudvm with support for multiple instances? At this stage I'm really looking for feedback on the usage of the tool, rather than a code review.

To get started,

  1. delete your existing ccloudvm instance, with a ccloudvm delete
  2. Then pull the branch
  3. go install ./...
  4. ccloudvm setup
  5. ccloudvm create xenial
  6. ccloudvm create xenial
  7. ccloudvm instances
  8. ccloudvm connect [instance of your choice]
devimc commented 6 years ago

I like this new design, I like the support for multiple instances

ganeshmaharaj commented 6 years ago

Per your request, skipping the code review at this point and providing feedback on the tool itself.

If i am able to think of anything else, i will go ahead and share them here.

markdryan commented 6 years ago

There is no inter-networking between the two VMs i spawned (I am sure you already know that and that is the plan for now).

Yes, absolutely. This is the main limitation of the current networking model. Well that and it's slow. The benefit of the current model is that no root access is required. I'd like to also provide macvtap based networking as well, but this will be a fair amount of work.

i feel a little queasy using 127.0.0..{1|2} for the machines. Not able to find any reason as to why that will be an issue, but if it was me, i would increment the port numbers.

The code is a bit easier if each instance gets its own IP address, and as these ip addresses are not used very often, there's less chance of conflict, although we can't eliminate the possibility of conflict whatever we do though. This design is also preparation for macvtap networking where each instance will have its own ip address, albeit assigned from some locally running dns instance. In the meantime, it's fine to use 127.0.0.0/8 on linux.

There is a big bug in the current code though. The ip address management code is user specific but ip addresses are system wide resources. This means there will be a conflict if two users on the same computer use ccloudvm at the same time. Note this is not a regression as the existing code in master also suffers from this problem. In fact things are better in this PR as you can at least manually assign the instance's IP address. I plan to fix this by having setup and teardown manage a file lock protected gob encoded map of 127.1.1.0/8 subnets stored in a global location, e.g., /var/lib/ccloudvm. Unless that is, anyone else has a better idea.

Would be nice to allow us to name the machine and default to creating names. Would be helpful with scripting if any is done on this.

You can already manually assign a name to a VM when you create it using --name, e.g.,

ccloudvm create --name devstack-test xenial ccloudvm connect devstack-test

The name needs to be a valid domain name, otherwise cloud-init will error out, so for example, devstack_test wouldn't work properly.

markdryan commented 6 years ago

@ganeshmaharaj Also, when https://github.com/intel/ccloudvm/issues/49 is fixed, all commands will be scriptable via templates. The create command will pass the template a structure containing information about the new instance you've just created. So you'll be able to do something like

name=$(ccloudvm create -f '{{.Name}}' xenial)

rbradford commented 6 years ago

In fact things are better in this PR as you can at least manually assign the instance's IP address. I plan to fix this by having setup and teardown manage a file lock protected gob encoded map of 127.1.1.0/8 subnets stored in a global location, e.g., /var/lib/ccloudvm. Unless that is, anyone else has a better idea.

Encode the uid modulo 2^16 in the 2nd and 3rd bytes of the IP address. This gives you a single /24 for each user. I think that's probably enough.

markdryan commented 6 years ago

@devimc , @ganeshmaharaj, @rbradford , @mcastelino

I've addressed the review comments, updated the documentation, added some unit tests ( and fixed the resulting bugs ). I think this one is good to go now.