search

cloudfoundry-attic / bosh-init

bosh-init is a tool used to create and update the Director VM
Apache License 2.0
31 stars 33 forks source link

CLI does not handle deleted deployment.json #1

Closed uzzz closed 9 years ago

uzzz commented 9 years ago

Hi,

I found a bug while was playing with bosh micro cli. Here the steps: 1) Set deployment with bosh-micro deployment /path/to/manifest.yml (this step creates deployment.json file with generated uuid near the manifest.yml) 2) Remove deployment.json (I don't remember why I actually did it, but I did) 3) Run bosh-micro deploy /path/to/release.tgz /path/to/stemcell.tgz

As an outcome the deploy step doesn't check if UUID is present and extracts tarballs right into ~/.bosh_micro/ directory. Further it creates VM successfully, but fail to start bosh agent because of registry contains empty value for "agent_id" key (the value should be uuid from deployment.json). It was not so easy to find out what's exactly wrong, I had to dig into source.

I tried to fix it and send PR, but there are couple of points thats needs to be discussed. First of all what's if expected behaviour if deployment.json is not exist? Show an error? Generated new file silently? Second point is about design. NewDeployCmd and NewDeploymentCmd constructors are made to make fakes injection in tests easy, and this is great, but on the other hand cmd factory (https://github.com/cloudfoundry/bosh-micro-cli/blob/master/cmd/factory.go) handles too much responsibilities (including loading deployment.json). So for instance to check if the file exist I see two options: check it in factory's loadDeploymentConfig() method, but it's not great from my point of view: I think deploy command should care about validating, second is encapsulate loadConfig and code from createDeployCmd into deploy command, but it will break dependency injection.

mariash commented 9 years ago

Hi @uzzz, thanks for reporting this issue. We actually have in our roadmap to fix this. I think that you have the right the feeling that validation should be done in deploy not in the factory. But this would require changing lots of interfaces. The idea would be to switch deployment config to something like config provider, that will be passed to objects that need it.

You can follow the progress of this story here: https://www.pivotaltracker.com/story/show/82914700

karlkfi commented 9 years ago

This is really a couple of issues:

  1. Any time the deployment.json file is created, a "Director ID" needs to be lazily generated to uniquely identify the director (used by the CPI to set in the VM metadata). For an real director, this would be shared across deployments, but multiple bosh-micro deploys could concievably be run simultaneously (after --config has been added), as long as they have different ports configured for registry/ssh-tunnel. So it really acts like multiple directors, not a single one.
  2. Any time the deployment.json file is created, a "Deployment ID" needs to be lazily generated to uniquely identify the deployment (used to determine the workspace path, where the CPI is compiled & installed). This could debatably be the "Director ID", but we would need to first add management for multiple CPI installations (which we'll likely have to do eventually). CPIs currently just get compiled/installed on top of the previous compilation/installation for that deployment (under ~/.bosh_micro//). So if you want to deploy to a different cloud, you need a different deployment (ID & manifest).
  3. Each deployed vm should each have its own "Agent ID", lazily generated when the vm is created. (Right now, the deploymentUUID is mistakenly being passed to CPI.create_vm AS the agent_id.)
karlkfi commented 9 years ago

Pushed fix: d7df3b6ebbc8a436375c858349775bf804bb4902

There was a fair amount of refactoring involved. Thanks for the heads-up.