server/terraform: create the executor next to the installer #442
Conversation
|
Can one of the admins verify this patch? |
| @@ -135,7 +147,7 @@ func TestExecutorMissingVar(t *testing.T) { | |||
| // Wait for its termination. | |||
| select { | |||
| case <-done: | |||
| case <-time.After(1 * time.Second): | |||
| case <-time.After(10 * time.Second): | |||
There was a problem hiding this comment.
Just curious, but why was this timeout increased?
There was a problem hiding this comment.
Jenkins executors are waaaaayyy to slooowwww sometimes and fails to execute TerraForm within a second, which usually takes about 100ms.
There was a problem hiding this comment.
It is actually on master already, we made the change at the same time with Dan. Just need to rebase for it to be gone.
|
I am a little afraid with this change because calling apply again with the same cluster name but different variables will overwrite these variables and try to reconcile from the existing state.. it might wipe an existing cluster. Should we still add a random number after the cluster name? |
|
Just tested. |
|
Tested on mac & it works. 👍 |
|
Ideally I'd like clustername + datetime as the directory name. eg: |
Rather than tmp, so users always have the assets somewhere, regardless of whether they download them or not, and gardless of potential system reboots.
|
Just FYI, user experience with a conflicting cluster is not ideal:
I created a cluster named ggtest, then started over and created another one with the same name but different network settings. The old behavior (without conflicting dir names) was to error-out early in the apply phase, and force the user to clean up their conflicting bits of infrastructure. |
|
The timeout does not seem so related but yeah, TerraForm is trying to reconcile, and thus is deleting the networks to re-create them. Having separate folders will just make TerraForm fail in this case because things like entries in the public Route53 zone will already exist. Users will have to recycle manually the previous one. Just changed. Hopefully this will not cause too much trouble to users when they retry on failed deployments.
|
|
Tests passed. Re-tested manually too. |
|
Yep. New directory name works for me too. |
Rather than tmp, so users always have the assets somewhere,
regardless of whether they download them or not, and
regardless of potential system reboots.
We should probably have a flag / env variable that controls that,
but we have little time right now. Must switch to doing self-hosted etcd
cleanup for the release now.
Fixes #446