expose machine_spec.from_image to allocate_machine

[mwrock]

Currently from_image is added to the machine_spec immediately after allocate_machine. I'm not sure if that was by design. I can think of scenarios where one would want to know if the machine is being created from an image from allocate_machine. This is possible using this hack:

case action_handler
when Chef::Provisioning::AddPrefixActionHandler # From machine_batch
  machines = action_handler.action_handler.provider.new_resource.machines
  this_machine = machines.select { |m| m.name == machine_spec.name}.first
  this_machine.from_image
else # from machine
  action_handler.provider.new_resource.from_image
end

This feels a bit evil.

This PR sets the machine_spec.from_image in the machine load_current_resource which also allows the value to be set just once instead of both the machine and machine_batch resources.

[jkeiser]

Wow! It looks like no driver supports creating a machine with from_image (as far as I can tell). machine_spec is supposed to be a record of the current state, not a desired state, so I don't think this is the right change ... but it needs to be communicated somewhere.

How about we set this on new_machine_options?

[mwrock]

I don't think new_machine_options will work in a batch. However it simply calls machine_options(driver) and that is called from by_new_driver in machine_batch so putting it in the machine_options method should work. I can amend this tonight if that sounds ok.

[mwrock]

The machine_options method should probably be scoped no wider than manipulating the machine_options. new_machine_options simply calls machine_options and doesn't seem like the right place either.

Just to clarify, the motivation behind this PR is another PR I have for chef-provisioning-docker - chef-boneyard/chef-provisioning-docker#56. That driver does make use of from_image. My PR needs to know if the container is being built from an image in allocate_machine and I use the hack I mentioned earlier in this thread.

I think ideally, it would be nice to expose a reference to the actual machine resource to the driver calls but that certainly seems out of scope here. You can get that reference now from the action_handler but that requires perhaps too much implementation knowledge from driver authors while having it hang off the machine_spec feels more natural.

I just moved the bulk of the machine_spec creation out of load_current_resource and into a new machine_spec_for method that accepts the driver json. Not convinced this is much, if any, of an improvement but there is no obviously better place to put it that I can see.

[tyler-ball]

@jkeiser It seems like you are saying: if the machine hasn't converged yet and we add the from_image attribute to the spec, then the spec no longer represents actual state - it would represent desired state.

@mwrock In driver.rb you don't have access to new_resource correct? Otherwise you could just check new_resource.from_image.

[tyler-ball]

@mwrock I see how the changes in this PR would solve your problem in chef-provisioning-docker, but I would like to get @jkeiser's opinion on if this is the correct long term fix.

[jkeiser]

Yeah, the spec shouldn't have desired state in it--it's not supposed to. machine_options is desired, machine_spec is current. The Driver methods can use the spec to find out the current state, or to update the current state after changes, but it feels like we shouldn't mix its purpose.

Can machine_options do what we need here? Maybe a standard machine_option that gets set based on from_image?

[mwrock]

ok. So maybe from_image should be present in machine_options or I (and possibly the docker driver are misaligned with the intent). The docker driver certainly treats this as desired state but currently deals with it in ready_machine. If machine_spec.from_image is present, it creates a container from the given image.

To @tyler-ball 's point, one does have access to machine_provider in driver.rb but very indirectly through the action_handler and the method varies whether or not one is in a batch:

case action_handler
when Chef::Provisioning::AddPrefixActionHandler # From machine_batch
  machines = action_handler.action_handler.provider.new_resource.machines
  this_machine = machines.select { |m| m.name == machine_spec.name}.first
  this_machine.from_image
else # from machine
  action_handler.provider.new_resource.from_image
end

I was hoping to make the retrieval of from_image in allocate_machine more natural. I'm gonna really try and make the provisioning office hours this week so maybe we can discuss further then if either of you are there.

[tyler-ball]

Talked with @mwrock today during office hours - we both agree that the from_image should be added to the machine_options before allocate_machine is called.

One thing to keep in mind here is we should make sure both machine and machine_batch reference the options correctly - #378

[mwrock]

My apologies for falling off that call so abruptly. As we were talking about this it occured to me that I likely misunderstood @jkeiser 's initial comment to this PR above when he juggested moving this logic to new_machine_options. I had thought he was suggesting assigning the from_image to machine_spec in that method. However I think his intent was to assign it to machine_options in that method. So I will update this to do just that. @jkeiser let me know if that was not what you had in mind.

[jkeiser]

Yep! I'm sorry I've been so scarce, I'm on an incredibly intensive vacation right now, not lots of opportunities to come up for air.

[mwrock]

No worries @jkeiser. I have updated my PR leaving the machine_spec alone and updating the machine_options from machine_provider.from_image. I had to make the change inside of the machine_options method instead of the new_machine_options method (as suggested above) since new_machine_options is not used inside of machine_batch.

[tyler-ball]

Hey @mwrock - I'm going to accept this PR now. It took 3 of us about 30 minutes to figure out what was going on here, but I think I got it. Let me summarize:

In your docker PR you need access to the from_image attribute inside of driver.rb. This attribute is not available in driver#allocate_machine because the driver does not have access to new_resource. So any required options must be passed into the driver via the machine_options parameter passed to all the driver methods. Am I understanding this correctly?

The only question in my mind is whether we include the from_image attribute inside of the convergence_options part of machine_options or leave it at the top level. Because from_image is not actually needed for convergence then I think it should stay where it is. I believe we also talked about this during office hours.

So I'm going to accept this PR as-is! Thanks for your patience on this one.

[mwrock]

You are correct @tyler-ball. Makes more sense to me to keep it at the top level. Thanks for merging!