DimensionDataResearch / terraform-provider-ddcloud

Terraform provider for Dimension Data cloud compute.
MIT License
16 stars 13 forks source link

Add the ability to change power state of a server #102

Closed pdenning closed 6 years ago

pdenning commented 6 years ago

Once the server is provisioned, have the ability to change the power state of the server.

This feature would also obsolete the auto_start argument as the new power state would auto start the server if set to start.

This would be achieved by creating a new argument for the server resource with 3 options. start, off, shutdown.

Please see pull request #101 for working example

tintoy commented 6 years ago

Hi - I like the sound of this and thanks for providing a PR to go with it :)

It's around 10pm here but I'll have a look at this properly first thing tomorrow.

tintoy commented 6 years ago

Ok, I like this idea; can we walk through a scenario or 2 to validate the design?

For example, let's say you have a configuration that specifies one server but doesn't specify an initial power state. If the default is off, then when you first run terraform apply it will create server and leave it powered off.

Now: Let's boot up our new server and at some point in the future run terraform apply again.

What's the desired behaviour here? Should the server be powered off again? If the server is started when the configuration specifies a power_state of off vs shutdown, should terraform refresh show any changes?

I guess what I'm getting at is that the machine's power state may be more ephemeral than the usual stuff we manage with Terraform and so we might need to be careful how we implement this feature (for example, it's always safe to do in resourceServerCreate, but doing it in resourceServerUpdate may do unexpected things from the perspective of people used to the existing behaviour).

As it is, this feature would be super-useful for modifying behaviour during resource creation, but we may need to refine the behaviour or make it a little more flexible during subsequent runs of terraform apply.

For example, off could mean "don't auto-start when deploying, but otherwise leave it alone" whereas shutdown could mean "shut down the server if it's ever powered on when Terraform runs".

What do you reckon?

CC: @wninobla @kumarappanc @calloes @nwright-nz - if you guys have opinions on this feel free to weigh in as well :)

pdenning commented 6 years ago

Ok I understand where it can be problematic if you are managing the state from both terraform and the web console.

Terraform refresh should refresh the state file to the current state of the server. Which i have made a change for. Will push after discussion about server power states have concluded as i'll input that into it.

To answer your question about differences in state and configuration files. The way i see it is that it should turn off the server again. As this keeps true to what terraform does. Sets desired state.

Now the question is how do you handle mistakes that can easily occur if you are using both terraform/mcp to manage the state of your server(s).

Outside of processes and procedures it is tough to answer. To help stop issues with backwards compatibility.
Have the following states

Removing the off state to remove any ambiguity between turning of a server and the feature not being disabled.

tintoy commented 6 years ago

That sounds pretty good - what about people who want the server to automatically start on first deploy but then ignored subsequently? Maybe add autostartas an option?

pdenning commented 6 years ago

autostart is a good idea. to allow for starting and not needing to worry about future state.

As this is now getting away from my original intend which was to a) control the power state of the server and b) know what state it current is in.

I would like to propose adding a started bool attribute that will either be true/false if the server is currently running or not.

tintoy commented 6 years ago

Yep I hear you, I just want to make sure existing users can upgrade without too much trouble :) started attribute sounds fine; do you mean like a computed property right?

pdenning commented 6 years ago

Yah, understand that, the last thing i would want is everyone's servers to start shutting down on them.

Computed property is what i am think of.

tintoy commented 6 years ago

I'm totally fine with adding it :)

PS. Thanks for sticking with this PR, I appreciate that it's turned out to be more work than you were initially expecting,

pdenning commented 6 years ago

Cheers I'll added that in to it on the note of the backwards compatibility. I have set auto-start to removed. Would you prefer that it be set to Deprecated?

and no worries. If the work i have done can be modified and be put to use by others. I'm happy to help.

tintoy commented 6 years ago

I think your first approach for that is better, let's leave it as obsolete so people get a useful message when they try to use a config that still specifies it.

BTW, don't forget to add yourself to CONTRIBUTORS.md :)

pdenning commented 6 years ago

made the changes and ran initial tests.

Would you like a migration from schema v4 to v5 ?

tintoy commented 6 years ago

Good catch! But in this case it would I be correct in guessing that it may not be necessary since any existing value would be ignored / equivalent to the default?

pdenning commented 6 years ago

ignored is default so if auto_start is false or not set it is fine.

However setting auto_start to removed. It needs to be removed from the config file or it'll error. So it won't convert auto_start = true to power_state = "autostart".

I have the basics for a migrate completed. just need to test therefore it might be worth just handling anyway.

tintoy commented 6 years ago

Yep fair point :)

pdenning commented 6 years ago

@tintoy I've updated the schema version and created a migration plan.

However I found a bug with the previous migration, from what i can tell the Migration from v3 to v4 would have never ran. Due to the currentSchemaVersion in resource_server_migration being set to 3.

tintoy commented 6 years ago

Oops! They're meant to execute in sequence, so if you set the correct value for currentSchemaVersion then it should still run yours correctly. I'll have a quick look at the v3-v4 upgrade logic to see what's in there.

tintoy commented 6 years ago

Thanks, good catch!

I've created an issue to track that and will have a go at writing a couple of acceptance tests for it later this week.

tintoy commented 6 years ago

Implemented in DimensionDataResearch/dd-cloud-compute-terraform#101 :-)