MM-57214: Integrate Redis into Terraform deployment #708

agnivade · 2024-03-14T07:41:12Z

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

agnivade · 2024-03-14T07:42:25Z

Just for review. The code is actually pointing to a branch in the server repo, which needs to be updated once it's merged.

agarciamontoro

This looks great! A couple of comments:

Do we want to support multiple Redis nodes at some point? I see everything hardcoded to one node. 0/5 on this at this point, just curious.
I understand this is still a WIP, but writing it here so that we don't foget: we need to document the newly added settings.

agarciamontoro · 2024-03-14T11:18:44Z

deployment/terraform/metrics.go

+		redisExporterService := fmt.Sprintf(redisExporterServiceFile, redisEndpoint)
+		rdr := strings.NewReader(redisExporterService)
+		if out, err := sshc.Upload(rdr, "/lib/systemd/system/redis-exporter.service", true); err != nil {
+			return fmt.Errorf("error upload redis exporter service file: output: %s, error: %w", out, err)


Suggested change

return fmt.Errorf("error upload redis exporter service file: output: %s, error: %w", out, err)

return fmt.Errorf("error uploading redis exporter service file: output: %s, error: %w", out, err)

agarciamontoro · 2024-03-14T11:21:35Z

deployment/terraform/output.go

@@ -189,6 +209,11 @@ func (o *Output) HasElasticSearch() bool {
 	return o.ElasticSearchServer.Endpoint != ""
 }

+// HasRedis returns whether a deployment has ElasticSaearch installed in it or not.


Suggested change

// HasRedis returns whether a deployment has ElasticSaearch installed in it or not.

// HasRedis returns whether a deployment has Redis installed in it or not.

agarciamontoro · 2024-03-14T11:26:26Z

deployment/terraform/assets/cluster.tf

@@ -85,6 +85,7 @@ resource "aws_instance" "app_server" {
      "sudo apt-get install -y mysql-client-8.0",
      "sudo apt-get install -y postgresql-client-11",
      "sudo apt-get install -y prometheus-node-exporter",
+      "sudo NEEDRESTART_MODE=a apt-get install -y redis-tools",


What does NEEDRESTART_MODE=a mean?

Haha this was a fun one. So if you try to install it manually, a screen comes up and it asks you what services you want to restart post installation. This NEEDRESTART_MODE=a is the way to automatically say ok to that prompt. -y does not take care of this case.

agarciamontoro · 2024-03-14T11:33:01Z

deployment/terraform/metrics.go

@@ -157,6 +159,31 @@ func (t *Terraform) setupMetrics(extAgent *ssh.ExtAgent) error {
 		}
 	}

+	if t.output.HasRedis() {
+		redisEndpoint := fmt.Sprintf("redis://%s", net.JoinHostPort(t.output.RedisServer.Address, strconv.Itoa(t.output.RedisServer.Port)))
+		redisTargets = append(redisTargets, "metrics:9121")


Nit:
This is the port of the redis exporter, right? I know the rest of definitions here are similar to this one, but they seem a bit cryptic if one doesn't know what they're defining, so maybe we can start slightly improving things with something like:

Suggested change

redisTargets = append(redisTargets, "metrics:9121")

redisExporterPort := "9121"

redisTargets = append(redisTargets, "metrics:"+redisExporterPort)

Isn't the same in the elasticsearch exporter as well?

Most probably, yes, but I noticed it here because it wasn't me the one writing the code 😂 I'm 1/5 here, though,just a nit :)

agnivade · 2024-03-14T14:04:45Z

Do we want to support multiple Redis nodes at some point? I see everything hardcoded to one node. 0/5 on this at this point, just curious.

Nope we don't. Because we don't want to get into handling consistency issues for Redis writer-reader again. One Redis instance is good for now.

streamer45 · 2024-03-28T16:54:33Z

Please add me back when ready for final review.

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

agarciamontoro · 2024-08-05T15:41:12Z

@agnivade Now that the main PR is to be merged, should we finalize this?

agnivade · 2024-08-05T16:27:02Z

Yep, this is next in my queue once the PR gets merged.

agarciamontoro · 2024-08-05T16:39:08Z

Neat, thanks!

agnivade · 2024-08-06T04:53:22Z

@agarciamontoro, @streamer45 - Ready for review now. I tested and verified that everything works fine.

streamer45

Nice!

streamer45 · 2024-08-06T06:48:56Z

deployment/terraform/assets/cluster.tf

@@ -84,6 +84,7 @@ resource "aws_instance" "app_server" {
      "sudo apt-get install -y mysql-client-8.0",
      "sudo apt-get install -y postgresql-client-14",
      "sudo apt-get install -y prometheus-node-exporter",
+      # "sudo NEEDRESTART_MODE=a apt-get install -y redis-tools",


What's the deal with this one?

This is needed to install the redis-cli. But the thing is there is an UI intervention while installing, which is supposedly fixed with NEEDRESTART_MODE=a. But that still somehow didn't fix things. So just ignoring for now. If someone needs to connect to Redis, they can install manually.

FWIW, the command worked fine on my end (EC2 running Ubuntu 22.04).

Hmm .. weird. I will test it again later.

streamer45 · 2024-08-06T06:51:48Z

deployment/terraform/create.go

+		cfg.CacheSettings.CacheType = model.NewString(model.CacheTypeRedis)
+		redisEndpoint := net.JoinHostPort(t.output.RedisServer.Address, strconv.Itoa(t.output.RedisServer.Port))
+		cfg.CacheSettings.RedisAddress = model.NewString(redisEndpoint)
+		cfg.CacheSettings.RedisDB = model.NewInt(0)


How about model.NewPointer? :p We probably need a ticket to review the whole thing, eventually.

Coming soon. I already have it for the drafts PR.

agarciamontoro

Whoops, missed this one, sorry! All looks good, thank you!

The only thing is the "sudo NEEDRESTART_MODE=a apt-get install -y redis-tools" line. What are we doing with this one? I'm ok simply removing it and adding it as an improvement later.

agarciamontoro · 2024-08-12T15:42:07Z

deployment/terraform/metrics.go

@@ -157,6 +159,31 @@ func (t *Terraform) setupMetrics(extAgent *ssh.ExtAgent) error {
 		}
 	}

+	if t.output.HasRedis() {
+		redisEndpoint := fmt.Sprintf("redis://%s", net.JoinHostPort(t.output.RedisServer.Address, strconv.Itoa(t.output.RedisServer.Port)))
+		redisTargets = append(redisTargets, "metrics:9121")


Most probably, yes, but I noticed it here because it wasn't me the one writing the code 😂 I'm 1/5 here, though,just a nit :)

agnivade · 2024-08-13T03:24:49Z

The only thing is the "sudo NEEDRESTART_MODE=a apt-get install -y redis-tools" line. What are we doing with this one? I'm ok simply removing it and adding it as an improvement later.

Removed it.

agnivade added the 2: Dev Review Requires review by a core committer label Mar 14, 2024

agnivade requested review from streamer45 and agarciamontoro March 14, 2024 07:41

agarciamontoro reviewed Mar 14, 2024

View reviewed changes

streamer45 removed their request for review March 28, 2024 16:54

agnivade force-pushed the redis branch from 60b27e9 to 40d5476 Compare May 16, 2024 05:48

agnivade added 2 commits July 5, 2024 10:27

WIP: MM-57214: Integrate Redis into Terraform deployment

62a4320

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

remove getAllTeams

da679ca

agnivade force-pushed the redis branch from d32037a to da679ca Compare July 5, 2024 04:58

Update go.mod

2d2c73f

agnivade added 4 commits August 6, 2024 09:46

Merge remote-tracking branch 'origin/master' into redis

9bbcb1b

using master public module version

d893aa7

Address review comments

d6d564d

Added redis dashboard

e95e015

agnivade changed the title ~~WIP: MM-57214: Integrate Redis into Terraform deployment~~ MM-57214: Integrate Redis into Terraform deployment Aug 6, 2024

agnivade marked this pull request as ready for review August 6, 2024 04:52

agnivade requested review from agarciamontoro and streamer45 August 6, 2024 04:52

streamer45 approved these changes Aug 6, 2024

View reviewed changes

agarciamontoro approved these changes Aug 12, 2024

View reviewed changes

Remove unnecessary line

5dbb2bb

agnivade merged commit 8148e93 into master Aug 13, 2024
1 check passed

agnivade deleted the redis branch August 13, 2024 03:29

agnivade added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Aug 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-57214: Integrate Redis into Terraform deployment #708

MM-57214: Integrate Redis into Terraform deployment #708

agnivade commented Mar 14, 2024

agnivade commented Mar 14, 2024

agarciamontoro left a comment

agarciamontoro Mar 14, 2024

agarciamontoro Mar 14, 2024

agarciamontoro Mar 14, 2024

agnivade Mar 14, 2024

agarciamontoro Mar 14, 2024 •

edited

Loading

agnivade Aug 6, 2024

agarciamontoro Aug 12, 2024

agnivade commented Mar 14, 2024

streamer45 commented Mar 28, 2024

agarciamontoro commented Aug 5, 2024 •

edited

Loading

agnivade commented Aug 5, 2024

agarciamontoro commented Aug 5, 2024

agnivade commented Aug 6, 2024

streamer45 left a comment

streamer45 Aug 6, 2024

agnivade Aug 6, 2024

streamer45 Aug 6, 2024

agnivade Aug 6, 2024

streamer45 Aug 6, 2024

agnivade Aug 6, 2024

agarciamontoro left a comment

agarciamontoro Aug 12, 2024

agnivade commented Aug 13, 2024

	return fmt.Errorf("error upload redis exporter service file: output: %s, error: %w", out, err)
	return fmt.Errorf("error uploading redis exporter service file: output: %s, error: %w", out, err)

	// HasRedis returns whether a deployment has ElasticSaearch installed in it or not.
	// HasRedis returns whether a deployment has Redis installed in it or not.

	redisTargets = append(redisTargets, "metrics:9121")
	redisExporterPort := "9121"
	redisTargets = append(redisTargets, "metrics:"+redisExporterPort)

MM-57214: Integrate Redis into Terraform deployment #708

MM-57214: Integrate Redis into Terraform deployment #708

Conversation

agnivade commented Mar 14, 2024

agnivade commented Mar 14, 2024

agarciamontoro left a 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

agarciamontoro Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnivade commented Mar 14, 2024

streamer45 commented Mar 28, 2024

agarciamontoro commented Aug 5, 2024 • edited Loading

agnivade commented Aug 5, 2024

agarciamontoro commented Aug 5, 2024

agnivade commented Aug 6, 2024

streamer45 left a 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

agarciamontoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnivade commented Aug 13, 2024

agarciamontoro Mar 14, 2024 •

edited

Loading

agarciamontoro commented Aug 5, 2024 •

edited

Loading