[MM-62747] add NFS support #884

isacikgoz · 2025-01-28T13:37:09Z

Summary

Adds EFS deployment capability

Ticket Link

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

agnivade

Very interested to see the results of this. Have you run any tests yet?

agnivade · 2025-01-28T14:26:50Z

deployment/terraform/assets/cluster.tf

@@ -210,17 +251,17 @@ resource "aws_instance" "proxy_server" {

 resource "aws_iam_user" "s3user" {
  name  = "${var.cluster_name}-s3user"
-  count = var.app_instance_count > 1 && var.s3_external_bucket_name == "" ? 1 : 0
+  count = var.app_instance_count > 1 && var.s3_external_bucket_name == "" && !var.create_efs ? 1 : 0


Oh boy indeed 😂 Maybe we should make use of modules for this. Having an s3 module that can be automatically switched on/off. Something like https://stackoverflow.com/a/72867545/3248221. I haven't ever tried that, so if that's too difficult/cumbersome, we could at least refactor this whole var.app_instance_count > 1 && var.s3_external_bucket_name == "" && !var.create_efs ? 1 : 0 into a new create_s3_resources variable. Thoughts?

agnivade · 2025-01-28T14:28:46Z

deployment/terraform/assets/provisioners/app.sh

@@ -17,6 +19,9 @@ do
      sudo sh -c 'echo "deb [signed-by=/usr/share/keyrings/postgres-archive-keyring.gpg] http://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" > /etc/apt/sources.list.d/pgdg.list' && \
      sudo apt-get -y update && \
      sudo apt-get install -y mysql-client-8.0 && \
+      sudo apt-get remove -y needrestart && \


Why are we removing this?

It's the only way to disable interactivity during install. I guess it will be required even if we are going to host the compiled binary.

agnivade · 2025-01-28T14:30:45Z

deployment/terraform/assets/provisioners/app.sh

@@ -17,6 +19,9 @@ do
      sudo sh -c 'echo "deb [signed-by=/usr/share/keyrings/postgres-archive-keyring.gpg] http://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" > /etc/apt/sources.list.d/pgdg.list' && \
      sudo apt-get -y update && \
      sudo apt-get install -y mysql-client-8.0 && \
+      sudo apt-get remove -y needrestart && \
+      sudo apt-get install -y -o Dpkg::Options::="--force-confold" binutils git cargo libssl-dev make pkg-config && \
+      git clone https://github.com/aws/efs-utils && cd efs-utils && make deb && sudo sudo apt-get install -y -o Dpkg::Options::="--force-confold" ./build/amazon-efs-utils-*.deb && cd - && \


Can we just host the debian file somewhere that we can download and install? This is adding more complications to an already complicated and error-prone provisioning step.

We can do so, maybe simply fork and host on github?

Yep, that works.

Agree that we should try to make this a bit more reliable. At least using a specific tag of the repo. But I'm on board with the fork idea.

Let's make this change as well to avoid making the debian. Just download from somewhere and do a dpkg -i

agnivade · 2025-01-28T14:35:50Z

deployment/terraform/create.go

@@ -508,7 +514,7 @@ func (t *Terraform) setupLoadtestAgents(extAgent *ssh.ExtAgent, initData bool) e
 		return fmt.Errorf("error while setting up an agents: %w", err)
 	}

-	if !t.output.HasAppServers() {
+	if !t.output.HasAppServers() || !t.output.HasAgents() {


t.initLoadTest already has this condition:

if len(t.output.Agents) == 0 { return errors.New("there are no agents to initialize load-test") }

Do we still need this OR condition?

Right, my addition was to avoid the error. I don't think having zero agents is a scenario we should fail. But I'm 0/5 as in the end this is a load test tool :)

Hmm, would there be any use-case where you want to deploy app servers, but not agents?

But even if we did, then in that case, we can remove the if condition inside initLoadTest.

But let's confirm whether an error is needed or not - WDYT @agarciamontoro ?

This has been brought up before. I think it doesn't make a lot of sense right now, since we always want to run a load test, and for that we need agents. If we ended up splitting the tool in a deployer / loadtester pair, then that would make more sense. But I think we should always assume there's at least one agent. I'm happy to change my mind, but at least it should be in a separate PR, I feel it's a whole separate discussion.

Let's change this @isacikgoz as per the above discussion.

deployment/terraform/create.go

agnivade · 2025-01-28T14:37:39Z

deployment/terraform/create.go

-	if t.output.HasProxy() && t.output.HasS3Key() && t.output.HasS3Bucket() {
+	if t.output.HasEFS() {
+		cfg.FileSettings.DriverName = model.NewPointer("local")
+		cfg.FileSettings.Directory = model.NewPointer("/mnt/mattermost-data")


Let's use the const efsDirectory?

agnivade · 2025-01-28T14:42:10Z

deployment/terraform/efs.go

+		mlog.Info("Updating /etc/fstab to mount EFS", mlog.String("fsid", t.output.EFSAccessPoint.FileSystemId))
+		// setting uid and gid to ubuntu user
+		fstabEntry := fmt.Sprintf("%s.efs.%s.amazonaws.com:/ %s nfs4 nfsvers=4.1,rsize=1048576,wsize=1048576,hard,timeo=600,retrans=2,noresvport,_netdev 0 0", t.output.EFSAccessPoint.FileSystemId, t.config.AWSRegion, efsDirectory)
+		cmd = fmt.Sprintf("grep -q %q /etc/fstab || echo %q | sudo tee -a /etc/fstab > /dev/null", efsDirectory, fstabEntry)


This command is too complicated to read. Why the "||" with echo? Why are we doing tee but then redirecting to /dev/null? Why can't we do a >> to append?

I wanted to add a check whether if it is already mounted. If not, we simply skip. For tee and /dev/null it's just syntactic taste :p

What about cmd = fmt.Sprintf("grep -q %q /etc/fstab || sudo sh -c 'echo %q >> /etc/fstab'", efsDirectory, fstabEntry)?

agnivade · 2025-01-28T14:44:17Z

deployment/terraform/efs.go

+			return fmt.Errorf("error mounting the fstab entry: %w", err)
+		}
+
+		cmd = fmt.Sprintf("sudo chown -R ubuntu:ubuntu %s", efsDirectory)


Instead of running the command, and then copying the same thing again in the svcScript, can we simply upload the script and then run the script as part of chmodding it - sudo chmod +x <file> && ./<file>?

We need to change mod on every reboot otherwise ownership is set to root. I couldn't find a better workaround for that. That's why I had to create the service.

Yeah sure. All I am saying is instead of running the chmod command separately, we can simply execute the file.

But do we need the script at all? Could we write ExecStart=chown -R ubuntu:ubuntu /mnt/mattermost-data directly in the unit file?

Ah nice one!

I guess we should do it in ExecStartPre then.

isacikgoz · 2025-01-28T13:57:10Z

deployment/terraform/assets/cluster.tf

+data "aws_vpc" "selected" {
+  tags = {
+    Name = "Default VPC"
+  }
+}


I think this should get default if there is no vpc defined in the config.

isacikgoz · 2025-01-28T14:01:37Z

deployment/terraform/create.go

@@ -508,7 +514,7 @@ func (t *Terraform) setupLoadtestAgents(extAgent *ssh.ExtAgent, initData bool) e
 		return fmt.Errorf("error while setting up an agents: %w", err)
 	}

-	if !t.output.HasAppServers() {
+	if !t.output.HasAppServers() || !t.output.HasAgents() {


This was resulting with error if there were no agents. I think we should be able to deploy without agents. 0/5

isacikgoz · 2025-01-28T15:02:22Z

deployment/terraform/assets/provisioners/app.sh

@@ -17,6 +19,9 @@ do
      sudo sh -c 'echo "deb [signed-by=/usr/share/keyrings/postgres-archive-keyring.gpg] http://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" > /etc/apt/sources.list.d/pgdg.list' && \
      sudo apt-get -y update && \
      sudo apt-get install -y mysql-client-8.0 && \
+      sudo apt-get remove -y needrestart && \


It's the only way to disable interactivity during install. I guess it will be required even if we are going to host the compiled binary.

isacikgoz · 2025-01-28T15:02:52Z

deployment/terraform/assets/provisioners/app.sh

@@ -17,6 +19,9 @@ do
      sudo sh -c 'echo "deb [signed-by=/usr/share/keyrings/postgres-archive-keyring.gpg] http://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" > /etc/apt/sources.list.d/pgdg.list' && \
      sudo apt-get -y update && \
      sudo apt-get install -y mysql-client-8.0 && \
+      sudo apt-get remove -y needrestart && \
+      sudo apt-get install -y -o Dpkg::Options::="--force-confold" binutils git cargo libssl-dev make pkg-config && \
+      git clone https://github.com/aws/efs-utils && cd efs-utils && make deb && sudo sudo apt-get install -y -o Dpkg::Options::="--force-confold" ./build/amazon-efs-utils-*.deb && cd - && \


We can do so, maybe simply fork and host on github?

isacikgoz · 2025-01-28T17:35:52Z

deployment/terraform/create.go

@@ -508,7 +514,7 @@ func (t *Terraform) setupLoadtestAgents(extAgent *ssh.ExtAgent, initData bool) e
 		return fmt.Errorf("error while setting up an agents: %w", err)
 	}

-	if !t.output.HasAppServers() {
+	if !t.output.HasAppServers() || !t.output.HasAgents() {


Right, my addition was to avoid the error. I don't think having zero agents is a scenario we should fail. But I'm 0/5 as in the end this is a load test tool :)

deployment/terraform/create.go

isacikgoz · 2025-01-28T17:41:21Z

deployment/terraform/efs.go

+		mlog.Info("Updating /etc/fstab to mount EFS", mlog.String("fsid", t.output.EFSAccessPoint.FileSystemId))
+		// setting uid and gid to ubuntu user
+		fstabEntry := fmt.Sprintf("%s.efs.%s.amazonaws.com:/ %s nfs4 nfsvers=4.1,rsize=1048576,wsize=1048576,hard,timeo=600,retrans=2,noresvport,_netdev 0 0", t.output.EFSAccessPoint.FileSystemId, t.config.AWSRegion, efsDirectory)
+		cmd = fmt.Sprintf("grep -q %q /etc/fstab || echo %q | sudo tee -a /etc/fstab > /dev/null", efsDirectory, fstabEntry)


I wanted to add a check whether if it is already mounted. If not, we simply skip. For tee and /dev/null it's just syntactic taste :p

isacikgoz · 2025-01-28T17:43:13Z

deployment/terraform/efs.go

+			return fmt.Errorf("error mounting the fstab entry: %w", err)
+		}
+
+		cmd = fmt.Sprintf("sudo chown -R ubuntu:ubuntu %s", efsDirectory)


We need to change mod on every reboot otherwise ownership is set to root. I couldn't find a better workaround for that. That's why I had to create the service.

agarciamontoro

This is great, @isacikgoz, thank you for all the work here!

I left a bunch of comments below, mostly nits, a few to discuss more in depth. The only other ask I have here is to add docs on the new config setting and update the config sample files (both json and toml) to add the setting there as well.

Thanks!

agarciamontoro · 2025-02-04T18:04:29Z

deployment/terraform/assets/cluster.tf

+data "aws_vpc" "selected" {
+  tags = {
+    Name = "Default VPC"
+  }
+}
+
+data "aws_subnets" "loadtest_subnets" {
+  filter {
+    name = "vpc-id"
+    values = [data.aws_vpc.selected.id]
+  }
+
+  tags = {
+    Name = "loadtest*"
+  }
+}


Should we make use of the ClusterSubnetIDs here if they are defined? Also, we have the variable cluster_vpc_id, which is configurable by the user.

Ah yeah we should, let me add that.

agarciamontoro · 2025-02-04T18:05:47Z

deployment/terraform/assets/cluster.tf

+  count = var.create_efs ? 1 : 0
+
+  tags = {
+   Name = "Shared-fs-${var.cluster_name}"


Following the convention for the Name tag, this should be something like

Suggested change

Name = "Shared-fs-${var.cluster_name}"

Name = "${var.cluster_name}-shared-fs"

agarciamontoro · 2025-02-04T18:06:52Z

deployment/terraform/assets/cluster.tf

+resource "aws_efs_mount_target" "efs_mount" {
+  for_each = (var.create_efs ? toset(data.aws_subnets.loadtest_subnets.ids) : toset({}))
+  file_system_id = aws_efs_file_system.efs_shared.0.id
+  subnet_id      = each.value
+  security_groups = [aws_security_group.app[0].id]
+}
+
+resource "aws_efs_access_point" "shared_dir" {
+  count = var.create_efs ? 1 : 0
+
+  file_system_id = aws_efs_file_system.efs_shared.0.id
+
+  root_directory {
+    path = "/"
+  }
+}


Can we also add Name tags to these in the form of "${var.cluster_name}-whatever-whatever"?

We can add it to aws_efs_access_point but can't add it to aws_efs_mount_target, I'll go ahead and add only to the access point.

agarciamontoro · 2025-02-04T18:08:34Z

deployment/terraform/assets/cluster.tf

+resource "aws_efs_mount_target" "efs_mount" {
+  for_each = (var.create_efs ? toset(data.aws_subnets.loadtest_subnets.ids) : toset({}))


Why do we need for_each here? Can't we use count = var.create_efs ? 1 : 0 as well?

@agarciamontoro We need to create mount target for each availability zone (by using the various subnets).

agarciamontoro · 2025-02-04T18:09:25Z

deployment/terraform/assets/cluster.tf

+resource "aws_efs_mount_target" "efs_mount" {
+  for_each = (var.create_efs ? toset(data.aws_subnets.loadtest_subnets.ids) : toset({}))
+  file_system_id = aws_efs_file_system.efs_shared.0.id
+  subnet_id      = each.value


Here we should use a new entry in ClusterSubnetIDs. There's many examples of that in this file.

I think we need to use the existing subnets, because this requires access from the other resources hence we create the targets accordingly.

agarciamontoro · 2025-02-04T18:24:33Z

deployment/terraform/efs.go

+[Service]
+Type=simple
+ExecStart=/bin/bash -c "/opt/mount.sh"
+
+
+[Install]
+WantedBy=multi-user.target


Nit levels over 9000 (double new line):

Suggested change

[Service]

Type=simple

ExecStart=/bin/bash -c "/opt/mount.sh"

[Install]

WantedBy=multi-user.target

[Service]

Type=simple

ExecStart=/bin/bash -c "/opt/mount.sh"

[Install]

WantedBy=multi-user.target

agarciamontoro · 2025-02-04T18:25:56Z

deployment/terraform/efs.go

+
+func (t *Terraform) setupEFS(extAgent *ssh.ExtAgent) error {
+	for _, val := range t.output.Instances {
+		sshc, err := extAgent.NewClient(val.PublicIP)


Let's take #881 into account in case it's merged before this one. cc/@fmartingr

agarciamontoro · 2025-02-04T18:27:32Z

deployment/terraform/efs.go

+
+		mlog.Info("Updating /etc/fstab to mount EFS", mlog.String("fsid", t.output.EFSAccessPoint.FileSystemId))
+		// setting uid and gid to ubuntu user
+		fstabEntry := fmt.Sprintf("%s.efs.%s.amazonaws.com:/ %s nfs4 nfsvers=4.1,rsize=1048576,wsize=1048576,hard,timeo=600,retrans=2,noresvport,_netdev 0 0", t.output.EFSAccessPoint.FileSystemId, t.config.AWSRegion, efsDirectory)


Nit: can we make this a multiline? Something like

Suggested change

fstabEntry := fmt.Sprintf("%s.efs.%s.amazonaws.com:/ %s nfs4 nfsvers=4.1,rsize=1048576,wsize=1048576,hard,timeo=600,retrans=2,noresvport,_netdev 0 0", t.output.EFSAccessPoint.FileSystemId, t.config.AWSRegion, efsDirectory)

fstabEntry := fmt.Sprintf("%s.efs.%s.amazonaws.com:/ %s nfs4 nfsvers=4.1,rsize=1048576,wsize=1048576,hard,timeo=600,retrans=2,noresvport,_netdev 0 0",

t.output.EFSAccessPoint.FileSystemId,

t.config.AWSRegion,

efsDirectory)

agarciamontoro · 2025-02-04T18:33:14Z

deployment/terraform/efs.go

+			return fmt.Errorf("error mounting the fstab entry: %w", err)
+		}
+
+		cmd = fmt.Sprintf("sudo chown -R ubuntu:ubuntu %s", efsDirectory)


But do we need the script at all? Could we write ExecStart=chown -R ubuntu:ubuntu /mnt/mattermost-data directly in the unit file?

agarciamontoro · 2025-02-04T18:34:46Z

deployment/terraform/efs.go

+		rdr = strings.NewReader(efsService)
+		if out, err := sshc.Upload(rdr, efcServiceDir, true); err != nil {
+			return fmt.Errorf("error uploading file, dstPath: %s, output: %q: %w", efcServiceDir, out, err)
+		}
+
+		cmd = "sudo service efs-mount start"
+		_, err = sshc.RunCommand(cmd)
+		if err != nil {
+			return fmt.Errorf("error starting the service for efs mount directory ownership: %w", err)
+		}


Does this work directly? Don't we need something like sudo systemctl daemon-reload after uploading the file and before starting the service?

I think it works directly, at least it seemed to work 😅

agnivade

Just 2 things to take care of. Looking good.

agnivade · 2025-02-19T04:18:17Z

deployment/terraform/create.go

@@ -508,7 +514,7 @@ func (t *Terraform) setupLoadtestAgents(extAgent *ssh.ExtAgent, initData bool) e
 		return fmt.Errorf("error while setting up an agents: %w", err)
 	}

-	if !t.output.HasAppServers() {
+	if !t.output.HasAppServers() || !t.output.HasAgents() {


Let's change this @isacikgoz as per the above discussion.

agnivade · 2025-02-19T04:19:13Z

deployment/terraform/assets/provisioners/app.sh

@@ -17,6 +19,9 @@ do
      sudo sh -c 'echo "deb [signed-by=/usr/share/keyrings/postgres-archive-keyring.gpg] http://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" > /etc/apt/sources.list.d/pgdg.list' && \
      sudo apt-get -y update && \
      sudo apt-get install -y mysql-client-8.0 && \
+      sudo apt-get remove -y needrestart && \
+      sudo apt-get install -y -o Dpkg::Options::="--force-confold" binutils git cargo libssl-dev make pkg-config && \
+      git clone https://github.com/aws/efs-utils && cd efs-utils && make deb && sudo sudo apt-get install -y -o Dpkg::Options::="--force-confold" ./build/amazon-efs-utils-*.deb && cd - && \


Let's make this change as well to avoid making the debian. Just download from somewhere and do a dpkg -i

add NFS support

582bc51

isacikgoz added the 2: Dev Review Requires review by a core committer label Jan 28, 2025

isacikgoz requested review from agarciamontoro and agnivade January 28, 2025 13:57

agnivade reviewed Jan 28, 2025

View reviewed changes

isacikgoz commented Jan 28, 2025

View reviewed changes

agarciamontoro requested changes Feb 4, 2025

View reviewed changes

isacikgoz added 2 commits February 18, 2025 11:09

reflect review comments

7ca10d0

Merge remote-tracking branch 'origin/master' into add-nfs

cd4c9bf

isacikgoz requested review from agarciamontoro and agnivade February 18, 2025 11:42

make s3 initialization simpler

30d681d

agnivade reviewed Feb 19, 2025

View reviewed changes

reflect review comments

5c08db9

isacikgoz requested a review from agnivade February 20, 2025 13:04

agnivade approved these changes Feb 20, 2025

View reviewed changes

	Name = "Shared-fs-${var.cluster_name}"
	Name = "${var.cluster_name}-shared-fs"

		resource "aws_efs_mount_target" "efs_mount" {
		for_each = (var.create_efs ? toset(data.aws_subnets.loadtest_subnets.ids) : toset({}))

[MM-62747] add NFS support #884

Are you sure you want to change the base?

[MM-62747] add NFS support #884

Conversation

isacikgoz commented Jan 28, 2025

Summary

Ticket Link

agnivade 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro Feb 4, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro Feb 4, 2025 •

edited

Loading