[MM-61113] Allow multiple customizable subnets #833

fmartingr · 2024-10-16T16:24:50Z

Summary

deployer config changes:
- ClusterSubnetID -> ClusterSubnetIDs
- ElasticsearchSettings.VPCID removed in favor of ClusterVpcId
All resources are now matched with the specified VPC in the configuration
Elasticsearch now has specific configuration for AZ Awareness if more than one subnet is set
Fixed a bug where we wouldn't wait for terraform stdout resulting in incomplete output from commands. 61f26c2

Ticket Link

https://mattermost.atlassian.net/browse/MM-61113

streamer45

Cool! A couple of high-level comments.

deployer config changes:

ClusterVpcId -> ClusterVPCIDs

I don't see this change in here. I guess you meant ClusterSubnetID to ClusterSubnetIDs.

Also, is the implicit indexing driven by certain constraints or just simpler/quicker to implement? I am wondering whether a more structured setting could be easier/digestible (and potentially less error-prone) on the user side.

fmartingr · 2024-10-17T19:30:57Z

Cool! A couple of high-level comments.

deployer config changes:

ClusterVpcId -> ClusterVPCIDs

I don't see this change in here. I guess you meant ClusterSubnetID to ClusterSubnetIDs.

Yeah, sorry. Edited.

Also, is the implicit indexing driven by certain constraints or just simpler/quicker to implement? I am wondering whether a more structured setting could be easier/digestible (and potentially less error-prone) on the user side.

What do you mean by this?

streamer45 · 2024-10-18T00:32:15Z

Also, is the implicit indexing driven by certain constraints or just simpler/quicker to implement? I am wondering whether a more structured setting could be easier/digestible (and potentially less error-prone) on the user side.

What do you mean by this?

Having the specific index of ClusterSubnetIDs refer to a resource type feels perhaps a bit overengineered. Why not define an object with N named fields (one per resource type)?

fmartingr · 2024-10-18T07:53:18Z

Having the specific index of ClusterSubnetIDs refer to a resource type feels perhaps a bit overengineered. Why not define an object with N named fields (one per resource type)?

I had something like that before (I iterated quite a bit during this week) but if I don't specify the subnet from a data "aws_subnet" block Terraform considers it part of the environment and tries to delete it on the destroy operation.

fmartingr · 2024-10-18T17:29:22Z

Refactored the code after a talk with @agarciamontoro.

I wasn't able to reproduce the issue I had that the destroy operation was trying to destroy the subnet if directly specified by a subnet_id parameter.

agnivade · 2024-10-21T07:23:41Z

deployment/terraform/engine.go

-	for scanner.Scan() {
-		mlog.Info(scanner.Text())
-	}
+	go func() {


What is the benefit of doing this? Scanning stdout is not really blocking anything. And anyways you need to wait until stderr scanning is finished.

Without this I was not getting the entire output from terraform, some details about errors where missing.

Ok, that's weird. I don't understand how this changes anything though. Probably we can look into this in an isolated scenario. FYI @agarciamontoro @streamer45

Hmm .. ok I understand the bug. https://pkg.go.dev/os/exec#Cmd.StdoutPipe

Cmd.Wait will close the pipe after seeing the command exit, so most callers need not close the pipe themselves. It is thus incorrect to call Wait before all reads from the pipe have completed.

So using a scanner to read from the pipe is error-prone and probably why it fails a lot of times. The better way is to directly set the StdErr/Stdout fields with a custom io.Writer.

type cmdLogger struct { } func (*cmdLogger) Write(in []byte) (int, error) { mlog.Info(string(in)) return len(in), nil }

and then

cmd.Stdout = &cmdLogger cmd.Stderr = cmd.Stdout if err := cmd.Start(); err != nil { return err } if err := cmd.Wait(); err != nil { return err }

Having the same writer for stdout and stderr is handled properly in the stdlib:

// If Stdout and Stderr are the same writer, and have a type that can
// be compared with ==, at most one goroutine at a time will call Write.

@fmartingr - can we try with something like this?

agnivade · 2024-10-21T07:26:02Z

config/deployer.sample.json

+    "Keycloak": "",
+    "Metrics": "",
+    "Proxy": "",
+    "Redis": []


Why do we need a list for Redis when it's just a single server? Contrary, why we don't need a list for other resources like app/agent/proxy?

I could use lists and then index by count, but I favoured simplicity in this first PR. if you think that's necessary it's a easy change.

Went ahead and did the change: 46be0e8

Ah no no, my point was if Redis is a single server, then we don't need a list there. Unless the list is mandatory somehow.

I've made the list change and if we only need one subnet (because there's only one server) we can just set only one subnet in the setting (or leave it empty to use the default from aws_subnet.selected). Feels simpler if every setting works the same way.

Ok, conceptually it just felt odd to have an array when it's just one resource. But not a big deal.

maybe if we use aws_elasticache_replication_group in the future?

It's all premature optimization :)

deployment/terraform/assets/cluster.tf

deployment/terraform/utils.go

agnivade · 2024-10-21T09:44:46Z

@fmartingr - Here: https://github.com/mattermost/mattermost-cloud-monitoring/pull/765/files#diff-af79a285e58611c0f303f2d367ca5d6de36a521a3e8375f3c092f2fda2e0c742R5, I can see aws_elasticache_subnet_group. Is there something different that forces us to use only aws_db_subnet_group?

fmartingr · 2024-10-21T09:50:13Z

@fmartingr - Here: https://github.com/mattermost/mattermost-cloud-monitoring/pull/765/files#diff-af79a285e58611c0f303f2d367ca5d6de36a521a3e8375f3c092f2fda2e0c742R5, I can see aws_elasticache_subnet_group. Is there something different that forces us to use only aws_db_subnet_group?

I just followed some documentation. I will change the resource type, but it's really weird that it let me use it.

7199169

agnivade

Thanks.

I would like to see #833 (comment) being tracked separately if you don't want to do this in this PR itself.

fmartingr · 2024-10-21T15:14:22Z

Thanks.

I would like to see #833 (comment) being tracked separately if you don't want to do this in this PR itself.

Opened #835 to track. I can use that changes locally myself while I test the RHEL scripts since that will be very prone to errors.

streamer45

Looks good!

streamer45 · 2024-10-21T17:39:02Z

deployment/config.go

+func (c *ClusterSubnetIDs) IsAnySet() bool {
+	return len(c.App) > 0 || len(c.Job) > 0 || len(c.Proxy) > 0 || len(c.Agent) > 0 || len(c.ElasticSearch) > 0 || len(c.Metrics) > 0 || len(c.Keycloak) > 0 || len(c.Database) > 0 || len(c.Redis) > 0
+}


I think that using something like reflect.DeepEqual to avoid every single field and automatically support future ones may be a worthwhile simplification given performance is not really a concern. Not a huge deal though.

agarciamontoro

Amazing job, and incredibly thorough! I left a couple of comments below. Thank you so much for all your work here, @fmartingr <3

deployment/config.go

agarciamontoro · 2024-10-22T08:51:28Z

deployment/terraform/utils.go

+// convertToTerraformVar converts a list parameter to a json encoded string
+func convertToTerraformVar[T any](param T) string {
+	result, err := json.Marshal(param)
+	if err != nil {
+		mlog.Error("failed to convert parameter to terraform var", mlog.Any("param", param), mlog.Err(err))
+		return ""
+	}
+
+	return string(result)
+}


Should we move this as a method of ClusterSubnetIDs, just like IsAnySet? It seems to be specific to that type, although it has a pretty generic name right now (although the comment says it only applies to lists)

As per the code, this converts any parameter since it just marshals into json, but happy to make it just a String() function on the type. I could also update the comment 👓

but happy to make it just a String() function on the type

That's what I did for TerraformMap, the type of CustomTags. That way you can simply pass it to Sprintf. I don't have a super strong opinion on this, but I find it slightly better, since it isolates its function.

docs/config/deployer.md

Co-authored-by: Alejandro García Montoro <[email protected]>

agarciamontoro

Thank you! Again, amazing work :)

* multiple subnet support * wait for terraform stdout * revert elasticsearch zone awareness * fixed line removed on merge conflict * AWSAvailabilityZone compatibility * support a single subnet in db groups * refactor to use explicit subnets * elasticsearch multiple subnet support * docs * review comments * use list of subnets for all resources * tags for db-subnet-group * updated sample files * aws_db_subnet_group -> aws_elasticcache_subnet_group * use reflect.DeepEqual to simplify code * Update docs/config/deployer.md Co-authored-by: Alejandro García Montoro <[email protected]> * Update deployment/config.go Co-authored-by: Alejandro García Montoro <[email protected]> * docs link to aws docs * function to struct method --------- Co-authored-by: Alejandro García Montoro <[email protected]>

fmartingr added 7 commits October 16, 2024 16:46

multiple subnet support

bd6cf5c

wait for terraform stdout

61f26c2

revert elasticsearch zone awareness

e46f4ff

Merge remote-tracking branch 'origin/master' into feat/vpc-subnets

e0a2169

fixed line removed on merge conflict

3a0365f

AWSAvailabilityZone compatibility

ed7411d

support a single subnet in db groups

ceaec13

fmartingr requested review from agarciamontoro, streamer45 and agnivade and removed request for agarciamontoro, streamer45 and agnivade October 17, 2024 14:56

fmartingr marked this pull request as ready for review October 17, 2024 14:56

streamer45 reviewed Oct 17, 2024

View reviewed changes

fmartingr added 2 commits October 18, 2024 09:57

Merge remote-tracking branch 'origin/master' into feat/vpc-subnets

597b264

refactor to use explicit subnets

08f7976

fmartingr requested review from streamer45, agnivade and agarciamontoro and removed request for agarciamontoro and agnivade October 18, 2024 17:27

agnivade reviewed Oct 21, 2024

View reviewed changes

fmartingr added 6 commits October 21, 2024 09:54

elasticsearch multiple subnet support

97b6784

docs

79f1662

review comments

4403ddb

use list of subnets for all resources

46be0e8

tags for db-subnet-group

b452366

updated sample files

24e710d

aws_db_subnet_group -> aws_elasticcache_subnet_group

7199169

fmartingr requested a review from agnivade October 21, 2024 14:23

agnivade approved these changes Oct 21, 2024

View reviewed changes

fmartingr mentioned this pull request Oct 21, 2024

Sometimes complete terraform output not being shown when there are errors #835

Open

streamer45 approved these changes Oct 21, 2024

View reviewed changes

use reflect.DeepEqual to simplify code

aa455ca

agarciamontoro requested changes Oct 22, 2024

View reviewed changes

fmartingr and others added 3 commits October 22, 2024 12:14

Update docs/config/deployer.md

9976510

Co-authored-by: Alejandro García Montoro <[email protected]>

Update deployment/config.go

a135454

Co-authored-by: Alejandro García Montoro <[email protected]>

docs link to aws docs

abe06e9

fmartingr requested a review from agarciamontoro October 22, 2024 11:13

function to struct method

4f0a362

agarciamontoro approved these changes Oct 22, 2024

View reviewed changes

agarciamontoro added the 4: Reviews Complete All reviewers have approved the pull request label Oct 22, 2024

fmartingr merged commit 29284a6 into master Oct 22, 2024
1 check passed

fmartingr deleted the feat/vpc-subnets branch October 22, 2024 13:46

agarciamontoro mentioned this pull request Oct 23, 2024

MM-61232: Deployment failures when ClusterSubnetIDs is not specified #837

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-61113] Allow multiple customizable subnets #833

[MM-61113] Allow multiple customizable subnets #833

fmartingr commented Oct 16, 2024 •

edited

Loading

streamer45 left a comment

fmartingr commented Oct 17, 2024

streamer45 commented Oct 18, 2024

fmartingr commented Oct 18, 2024

fmartingr commented Oct 18, 2024

agnivade Oct 21, 2024

fmartingr Oct 21, 2024

agnivade Oct 21, 2024

agnivade Oct 21, 2024

agnivade Oct 21, 2024

fmartingr Oct 21, 2024

fmartingr Oct 21, 2024

agnivade Oct 21, 2024

fmartingr Oct 21, 2024

agnivade Oct 21, 2024

fmartingr Oct 21, 2024

agnivade Oct 21, 2024

agnivade commented Oct 21, 2024

fmartingr commented Oct 21, 2024 •

edited

Loading

agnivade left a comment

fmartingr commented Oct 21, 2024

streamer45 left a comment

streamer45 Oct 21, 2024

agarciamontoro left a comment

agarciamontoro Oct 22, 2024

fmartingr Oct 22, 2024 •

edited

Loading

agarciamontoro Oct 22, 2024

fmartingr Oct 22, 2024

agarciamontoro left a comment

[MM-61113] Allow multiple customizable subnets #833

[MM-61113] Allow multiple customizable subnets #833

Conversation

fmartingr commented Oct 16, 2024 • edited Loading

Summary

Ticket Link

streamer45 left a comment

Choose a reason for hiding this comment

fmartingr commented Oct 17, 2024

streamer45 commented Oct 18, 2024

fmartingr commented Oct 18, 2024

fmartingr commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnivade commented Oct 21, 2024

fmartingr commented Oct 21, 2024 • edited Loading

agnivade left a comment

Choose a reason for hiding this comment

fmartingr commented Oct 21, 2024

streamer45 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmartingr Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro left a comment

Choose a reason for hiding this comment

fmartingr commented Oct 16, 2024 •

edited

Loading

fmartingr commented Oct 21, 2024 •

edited

Loading

fmartingr Oct 22, 2024 •

edited

Loading