Fix runner termination in AWS with non cloud regions #344

dacbd · 2022-01-03T22:32:34Z

closes Runner does not destroy if we set a synthetic region in AWS #343

… json

dacbd · 2022-01-03T23:30:12Z

@DavidGOrtega this should take care of it, I have to set up an AWS acc to really give it whirl if you can try it out against how you were able to reproduce the original error as well?

DavidGOrtega · 2022-01-03T23:30:49Z

I will try it

iterative/resource_runner.go

DavidGOrtega · 2022-01-03T23:43:44Z

iterative/utils/helpers.go

 )

+var (
+	SynthRegions = map[string]map[string]string{


Not sure about this... I prefer to have the GetRegions implementation per provider. Or should go in the machine... if we were using the machine...

I moved it here because I think there was actually a bug in the subnet-id code after you guys fixed the last one.

See previous:

region := GetRegion(d.Get("region").(string)) availabilityZone := GetAvailabilityZone(region)

The get avail func would always get a stripped region and never detect an availability-zone, now it needs to first reference the synth-regions map to eliminate those as a false positive.

Since it need to be referenced in multiple places I went ahead and broke it out / consolidated that helper func across all providers.

… tfstate json" This reverts commit 9e4f51c.

dacbd · 2022-01-04T01:04:01Z

after beating my personal AWS account's 2FA (had to dig up an old phone 🔒 ) I tested with the original command from discord and the instance self terminated successfully:

./bin/cml.js runner \
    --labels=cml-runner \
    --idle-timeout=60 \
    --reuse \
    --cloud aws \
    --cloud-type=t2.micro \
    --cloud-spot true \
    --cloud-region us-west

DavidGOrtega · 2022-01-04T10:59:10Z

Thanks @dacbd. How did you manage to setup this version of the provider in the cloud instance?
The ways I have found are:

Create your own provider in terraform's registry
use a local provider -> setup a large idle_timeout (10mins) -> connect SSH -> make my terraform provider

Obviously the first one is easier but the registry is buggy and sometimes the provider breaks and can not be updated and cant be deleted. I have a bunch of them already

dacbd · 2022-01-04T16:05:53Z

@DavidGOrtega you know I didn't actually put much thought into it and now that you mention is I'm not 100% sure it worked properly(using the right one), (but it kinda, since it self-terminated). I modified the cml terraform.js to use a local one and added a git clone/checkout then make && make install into a startup script...

Were you able to test it with the latest commit successfully? -- I've put more thought in testing custom cml versions #255 😉

DavidGOrtega · 2022-01-04T16:15:25Z

@dacbd still testing it... Right now I have an issue but it should not be because of this PR think...
With region us-west-1 goes fine, with us-west it does not... it propagates the tf file with region us-west-1 and then I have an error about roles... a very nice riddle since as I say using us-west-1 is working always

No EC2 imds role found

dacbd · 2022-01-04T16:18:47Z

I'll try a few more as well

DavidGOrtega · 2022-01-04T16:28:40Z

iterative/utils/helpers.go

+			"eu-north": "europe-north1-a",
+			"eu-west":  "europe-west1-d",
+		},
+		"az": {


@dacbd cloud in machine and runner is azure not az

@0x2b3bfa0 we should have maintained the consistency

I do not feel confident to release this without having tested every cloud.
Please @dacbd try to fix it just only for AWS vendor without reaching a wider scope

I mean roll back utils.Get region

@0x2b3bfa0 we should have maintained the consistency

We do, to some extent.

terraform-provider-iterative/iterative/resource_machine.go

Line 200 in 01af144

} else if cloud == "azure" || cloud == "az" {

@0x2b3bfa0 we should have maintained the consistency

We do, to some extent.

terraform-provider-iterative/iterative/resource_machine.go

Line 200 in 01af144

} else if cloud == "azure" || cloud == "az" {

We should support then az in the machine and runner

We do support az in both the machine and runner resources. At least, before merging this pull request.

One should be an alias of the other to avoid issues like this one

Do you mean replacing the value intead of accepting both?

Yes, an alias

This reverts commit 3f1913f.

dacbd · 2022-01-04T17:09:26Z

@DavidGOrtega Something like this okay or do you want it more fully reverted back?

dacbd · 2022-01-04T17:21:50Z

I've had good terminations with:

us-west
us-west-1
us-west-1c
us-west-1b

0x2b3bfa0 · 2022-01-04T17:26:00Z

iterative/utils/helpers.go

 )

+var (
+	AWSSynthRegions = map[string]map[string]string{


Given that it's only for AWS, wouldn't it be enough to use a simple map[string]string? Else, should it be named SynthRegions instead?

Indeed probably I would keep it in the AWS provider instead of utils

#344 (comment)

I can roll it back more, I left it in to inflict my feelings that these region mappings should all be defined in one place.

Probably makes sense to have it in the provider following the same pattern as the others. Also as @0x2b3bfa0 pointed out there is not need to use a matrix

DavidGOrtega

👍

DavidGOrtega · 2022-01-04T21:06:42Z

🙏 thanks @dacbd !!! ❤️

dacbd added 4 commits January 3, 2022 14:30

initial setup

ac27e61

consolidate Synth region table

c56d81e

update local getregion funcs

3f1913f

go test -update

64c09dc

DavidGOrtega changed the title ~~Closes #343~~ Fix runner termination in AWS with non cloud regions Jan 3, 2022

revert region in tfResource value should be safe already from tfstate…

9e4f51c

… json

dacbd marked this pull request as ready for review January 3, 2022 23:30

DavidGOrtega reviewed Jan 3, 2022

View reviewed changes

iterative/resource_runner.go Show resolved Hide resolved

DavidGOrtega reviewed Jan 3, 2022

View reviewed changes

Revert "revert region in tfResource value should be safe already from…

709d1ed

… tfstate json" This reverts commit 9e4f51c.

DavidGOrtega suggested changes Jan 4, 2022

View reviewed changes

casperdcl added cloud-aws Amazon Web Services p0-critical Max priority (ASAP) resource-runner iterative_runner TF resource labels Jan 4, 2022

dacbd added 2 commits January 4, 2022 08:49

Revert "update local getregion funcs"

dfcbc5f

This reverts commit 3f1913f.

only aws

3c42cce

dacbd marked this pull request as draft January 4, 2022 17:13

dacbd marked this pull request as ready for review January 4, 2022 17:22

dacbd requested a review from DavidGOrtega January 4, 2022 17:22

go test -update

a7b6c72

0x2b3bfa0 reviewed Jan 4, 2022

View reviewed changes

pull out of utils / only used in aws provider

5129f29

DavidGOrtega approved these changes Jan 4, 2022

View reviewed changes

DavidGOrtega merged commit ddf38b7 into iterative:master Jan 4, 2022

This was referenced Nov 15, 2022

Helper machine type parity with task #718

Merged

Common/Static interface for resolving our helper machine_type(s) and region(s) #720

Open

Fix runner termination in AWS with non cloud regions #344

Fix runner termination in AWS with non cloud regions #344

Uh oh!

Conversation

dacbd commented Jan 3, 2022 • edited by DavidGOrtega Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dacbd commented Jan 3, 2022

Uh oh!

DavidGOrtega commented Jan 3, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dacbd commented Jan 4, 2022

Uh oh!

DavidGOrtega commented Jan 4, 2022

Uh oh!

dacbd commented Jan 4, 2022

Uh oh!

DavidGOrtega commented Jan 4, 2022

Uh oh!

dacbd commented Jan 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dacbd commented Jan 4, 2022

Uh oh!

dacbd commented Jan 4, 2022

Uh oh!

0x2b3bfa0 Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidGOrtega left a comment

Choose a reason for hiding this comment

Uh oh!

DavidGOrtega commented Jan 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dacbd commented Jan 3, 2022 •

edited by DavidGOrtega

Loading

0x2b3bfa0 Jan 4, 2022 •

edited

Loading