I’ve just tested to deploy my staging environment with this branch and it worked fine for me.

Note I haven’t tried out any of the new features added, only whether what I’m using right now still works (I also use a spot instance, but not yet the new flags you’ve added).

@nh2 Thanks for the review. i did get @Chakerbh branch and added o ntop of it so i may did something wrong there to cause the commit message to appear twice Ill check and fix that.

@nh2 So i fixed the latter commits but i was not able to do for the duplicates ones i got a lot of conflicts when rebasing :( .

@PsyanticY maybe try rebasing and squashing any extra commit + the 2 first commits

@AmineChikhaoui Done squashing the commits, also did re-base the pr on current master. (man that was a pain)

@PsyanticY thanks !
Looking at the spot instance requests docs, looks like we can actually test this manually. As the request will remain active in case of persistent unless we cancel it, an aws ec2 terminates-instances call should fake a spot instance termination and the instance should restart at some point (at least this is my understanding).
I'm trying with that, would like to double check the behavior before merging.

Ok interesting, so here is what happens:

I create a persistent spot instance request sir-foo, that request gets fullfilled with instance id i-first, then I terminate the instance. When following the output of describe-spot-instance-requests, the sir creates another instance i-second after a couple of minutes, now nixops doesn't have any idea about that so the deploy would fail as it doesn't check the output of the sir again to pull out the new vm id.
Even a destroy would fail and barf about the sir being fulfilled unexpectedly with the new instance id, the only way to destroy is to run nixops check followed with nixops destroy as the first will update the state with vmId = None which will allow the cancellation of the sir as we don't have a way to compare vm ids anymore.

So, I believe that this PR needs more work to teach nixops to recover cleanly after a termination behavior of stop. The good part is at least it's possible to fake a spot instance termination :)

(experimenting with some labels to make filtering PRs easy)

I think the way you did test the persistent spot is wrong since persistent spot relies on stopping the instance when we are out of capacity and restart it when we have. since the instance is stopped the instance id is not lost and we can recover cleanly given that. Terminating a persistent spot is not supported since if you even try to deploy a a persistent spot with spotInstanceInterruptionBehavior set to terminate ec2 will complain about InvalidParameterCombination:

master..> got (possibly transient) EC2 error '{'Message': 'The terminate InstanceInterruptionBehavior is not supported when requestType is set to persistent.', 'Code': 'InvalidParameterCombination'}', retrying...
master..> got (possibly transient) EC2 error '{'Message': 'The terminate InstanceInterruptionBehavior is not supported when requestType is set to persistent.', 'Code': 'InvalidParameterCombination'}', retrying...
master..> got (possibly transient) EC2 error '{'Message': 'The terminate InstanceInterruptionBehavior is not supported when requestType is set to persistent.', 'Code': 'InvalidParameterCombination'}', retrying...
^Cerror: interrupted

Ok I think you're right the annoucement mentioned:

Upon restart, the EBS root device is restored from its prior state, previously attached data volumes are reattached, and the instance retains its instance ID.