-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MM-61113] Allow multiple customizable subnets #833
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Yeah, sorry. Edited.
What do you mean by this? |
Having the specific index of |
I had something like that before (I iterated quite a bit during this week) but if I don't specify the subnet from a |
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 |
for scanner.Scan() { | ||
mlog.Info(scanner.Text()) | ||
} | ||
go func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this I was not getting the entire output from terraform, some details about errors where missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
"Keycloak": "", | ||
"Metrics": "", | ||
"Proxy": "", | ||
"Redis": [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went ahead and did the change: 46be0e8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, conceptually it just felt odd to have an array when it's just one resource. But not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe if we use aws_elasticache_replication_group
in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's all premature optimization :)
@fmartingr - Here: https://github.com/mattermost/mattermost-cloud-monitoring/pull/765/files#diff-af79a285e58611c0f303f2d367ca5d6de36a521a3e8375f3c092f2fda2e0c742R5, I can see |
I just followed some documentation. I will change the resource type, but it's really weird that it let me use it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
deployment/config.go
Outdated
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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing job, and incredibly thorough! I left a couple of comments below. Thank you so much for all your work here, @fmartingr <3
deployment/terraform/utils.go
Outdated
// 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) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Alejandro García Montoro <[email protected]>
Co-authored-by: Alejandro García Montoro <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>
* 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]>
Summary
ClusterSubnetID
->ClusterSubnetIDs
ElasticsearchSettings.VPCID
removed in favor ofClusterVpcId
Ticket Link
https://mattermost.atlassian.net/browse/MM-61113