enable hierarchy memory limit, config cgroup root #1195

adnxn · 2018-01-19T18:46:01Z

Summary

Includes the following changes:

enable memory.use_hierarchy scoped to the task's cgroup root
make the cgroup root path configurable through ecs.config and resolved at runtime, let default be /sys/fs/cgroup/memory/. this addresses [WIP] Fix an error case with task limits #1147 (review)
refactored DockerTaskEngine to receive agent.resource instead of creating it's own instance.

We enforce the memory.use_hierarchy flag to the task level and not the /ecs root because the kernel does not allow modifications to anything under the /ecs/ tree if child cgroups have already been created. This means that we will not be able to toggle the memory.use_hierarchy flag for existing ECS instances. Reports of this issue are also tracked here lxc/lxc#236 and linuxkit/linuxkit#373.

Implementation details

echo 1 > memory.use_hierarchy scoped to task cgroup
added ApplyConfigDependencies(cfg *config.Config) to type Resource interface
added memorySubsystemPath string to type cgroupWrapper struct
refactored function signatures for NewTaskEngine and NewDockerTaskEngine

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:

Description for the changelog

Licensing

This contribution is under the terms of the Apache 2.0 License: yes

richardpen

You may also want to update the header of all the touched file to 2018.

richardpen · 2018-01-24T00:49:14Z

agent/config/config.go

@@ -280,6 +280,8 @@ func environmentConfig() (Config, error) {

 	dockerStopTimeout := getDockerStopTimeout()

+	cgroupMemorySubsystemPath := os.Getenv("ECS_CGROUP_MEMORY_SUBSYSTEM_PATH")


This needs to be updated in the readme file.

richardpen · 2018-01-24T00:49:59Z

agent/config/config_unix.go

@@ -27,6 +27,8 @@ const (
 	defaultCredentialsAuditLogFile = "/log/audit.log"
 	// Default cgroup prefix for ECS tasks
 	DefaultTaskCgroupPrefix = "/ecs"
+
+	defaultCgroupMemorySubsystemPath = "/sys/fs/cgroup/memory/"


Can you add a comment for this?

richardpen · 2018-01-24T00:59:36Z

agent/resources/resources_linux.go

+	// echo 1 > memory.use_hierarchy
+	memorySubsystem := c.memorySubsystemPath
+
+	err = c.ioutil.WriteFile(filepath.Join(memorySubsystem, cgroupRoot, memoryUseHierarchy), []byte(strconv.Itoa(1)), os.FileMode(0))


Can you make the 1 and 0 constant and add some comment?

Unless you feel strongly about it, I don't think this needs to have its own constants since they're only being used in this specific instance, also I think the comment echo 1 > ... conveys context. Let me know though.

should we do this for each task? Isn't it enough if we did this once at the top level ecs cgroup?

@aaithal you're right, ill move this up a level and make it part of cgroupInit()

adnxn · 2018-01-24T01:05:02Z

@richardpen righttt the header dates. will do.

aaithal · 2018-01-24T17:16:43Z

agent/acs/update_handler/updater_test.go

@@ -105,7 +105,7 @@ func TestPerformUpdateWithUpdatesDisabled(t *testing.T) {
 		Reason:            ptr("Updates are disabled").(*string),
 	}})

-	u.performUpdateHandler(statemanager.NewNoopStateManager(), engine.NewTaskEngine(cfg, nil, nil, nil, nil, nil, nil))(&ecsacs.PerformUpdateMessage{
+	u.performUpdateHandler(statemanager.NewNoopStateManager(), engine.NewTaskEngine(cfg, nil, nil, nil, nil, nil, nil, nil))(&ecsacs.PerformUpdateMessage{


minor: this is an opportunity for you to split these long lines into multiple lines :)

aaithal · 2018-01-24T17:17:19Z

agent/app/agent.go

@@ -308,13 +311,13 @@ func (agent *ecsAgent) newTaskEngine(containerChangeEventStream *eventstream.Eve
 	if !agent.cfg.Checkpoint {
 		seelog.Info("Checkpointing not enabled; a new container instance will be created each time the agent is run")
 		return engine.NewTaskEngine(agent.cfg, agent.dockerClient,
-			credentialsManager, containerChangeEventStream, imageManager, state, agent.metadataManager), "", nil
+			credentialsManager, containerChangeEventStream, imageManager, state, agent.metadataManager, agent.resource), "", nil


minor: again, this is an opportunity for you to split these long lines into multiple lines :)

aaithal · 2018-01-24T17:23:29Z

README.md

@@ -176,6 +176,7 @@ additional details on each available environment variable.
 | `ECS_ENABLE_CONTAINER_METADATA` | `true` | When `true`, the agent will create a file describing the container's metadata and the file can be located and consumed by using the container enviornment variable `$ECS_CONTAINER_METADATA_FILE` | `false` | `false` |
 | `ECS_HOST_DATA_DIR` | `/var/lib/ecs` | The source directory on the host from which ECS_DATADIR is mounted. We use this to determine the source mount path for container metadata files in the case the ECS Agent is running as a container. We do not use this value in Windows because the ECS Agent is not running as container in Windows. | `/var/lib/ecs` | `Not used` |
 | `ECS_ENABLE_TASK_CPU_MEM_LIMIT` | `true` | Whether to enable task-level cpu and memory limits | `true` | `false` |
+| `ECS_CGROUP_MEMORY_SUBSYSTEM_PATH` | `/sys/fs/cgroup/memory/` | The root path of the cgroup memory subsystem  | `/sys/fs/cgroup/memory/` | Not applicable |


is this the path on the host or in the agent mount? We should make that more clear here as well.

aaithal · 2018-01-24T17:23:56Z

agent/app/agent.go

@@ -145,6 +145,9 @@ func newAgent(
 		metadataManager = containermetadata.NewManager(dockerClient, cfg)
 	}

+	resource := resources.New()
+	resource.ApplyConfigDependencies(cfg)


shouldn't this be happening only if cgroup feature is enabled?

That logic is within resources, I believe.

The idea was that 'resources' could hold on to more things than just cgroups.

i can put it after the agent.cfg.TaskCPUMemLimit.Enabled check

aaithal · 2018-01-24T17:25:10Z

agent/resources/resources_linux.go

+	// echo 1 > memory.use_hierarchy
+	memorySubsystem := c.memorySubsystemPath
+
+	err = c.ioutil.WriteFile(filepath.Join(memorySubsystem, cgroupRoot, memoryUseHierarchy), []byte(strconv.Itoa(1)), os.FileMode(0))


minor: can you split this into multiple lines?

aaithal · 2018-01-24T17:26:28Z

agent/resources/resources_linux.go

+	// echo 1 > memory.use_hierarchy
+	memorySubsystem := c.memorySubsystemPath
+
+	err = c.ioutil.WriteFile(filepath.Join(memorySubsystem, cgroupRoot, memoryUseHierarchy), []byte(strconv.Itoa(1)), os.FileMode(0))


should we do this for each task? Isn't it enough if we did this once at the top level ecs cgroup?

petderek

nice

petderek · 2018-01-24T20:34:47Z

README.md

@@ -176,6 +176,7 @@ additional details on each available environment variable.
 | `ECS_ENABLE_CONTAINER_METADATA` | `true` | When `true`, the agent will create a file describing the container's metadata and the file can be located and consumed by using the container enviornment variable `$ECS_CONTAINER_METADATA_FILE` | `false` | `false` |
 | `ECS_HOST_DATA_DIR` | `/var/lib/ecs` | The source directory on the host from which ECS_DATADIR is mounted. We use this to determine the source mount path for container metadata files in the case the ECS Agent is running as a container. We do not use this value in Windows because the ECS Agent is not running as container in Windows. | `/var/lib/ecs` | `Not used` |
 | `ECS_ENABLE_TASK_CPU_MEM_LIMIT` | `true` | Whether to enable task-level cpu and memory limits | `true` | `false` |
+| `ECS_CGROUP_MEMORY_SUBSYSTEM_PATH` | `/sys/fs/cgroup/memory/` | The root path of the cgroup memory subsystem  | `/sys/fs/cgroup/memory/` | Not applicable |


I would prefer ECS_CGROUP_PATH and not include the memory subsystem at this level.

It makes it more open for reuse, and I think its important given that it is a configuration variable.

this sounds reasonable.

petderek · 2018-01-24T20:36:23Z

agent/app/agent.go

@@ -145,6 +145,9 @@ func newAgent(
 		metadataManager = containermetadata.NewManager(dockerClient, cfg)
 	}

+	resource := resources.New()
+	resource.ApplyConfigDependencies(cfg)


That logic is within resources, I believe.

The idea was that 'resources' could hold on to more things than just cgroups.

aaithal

code lgtm. I have stylistic and lint comments.

aaithal · 2018-01-25T00:13:55Z

agent/config/types.go

@@ -194,4 +194,6 @@ type Config struct {
 	// OverrideAWSLogsExecutionRole is config option used to enable awslogs
 	// driver authentication over the task's execution role
 	OverrideAWSLogsExecutionRole bool
+
+	CgroupPath string


Needs a comment

aaithal · 2018-01-25T00:15:10Z

agent/resources/interface.go

@@ -26,4 +27,6 @@ type Resource interface {
 	Setup(task *api.Task) error
 	// Cleanup removes the resource
 	Cleanup(task *api.Task) error
+
+	ApplyConfigDependencies(cfg *config.Config)


Needs a comment

aaithal · 2018-01-25T00:15:30Z

agent/acs/update_handler/updater_test.go

@@ -159,7 +160,7 @@ func TestFullUpdateFlow(t *testing.T) {

 			require.Equal(t, "update-tar-data", writtenFile.String(), "incorrect data written")

-			u.performUpdateHandler(statemanager.NewNoopStateManager(), engine.NewTaskEngine(cfg, nil, nil, nil, nil, nil, nil))(&ecsacs.PerformUpdateMessage{
+			u.performUpdateHandler(statemanager.NewNoopStateManager(), engine.NewTaskEngine(cfg, nil, nil, nil, nil, nil, nil, nil))(&ecsacs.PerformUpdateMessage{


Can you also split this and other long lines as well?

petderek

Minor changes. FFTI the formatting comment.

petderek · 2018-01-25T00:18:59Z

agent/acs/update_handler/updater_test.go

@@ -105,7 +105,8 @@ func TestPerformUpdateWithUpdatesDisabled(t *testing.T) {
 		Reason:            ptr("Updates are disabled").(*string),
 	}})

-	u.performUpdateHandler(statemanager.NewNoopStateManager(), engine.NewTaskEngine(cfg, nil, nil, nil, nil, nil, nil, nil))(&ecsacs.PerformUpdateMessage{
+	u.performUpdateHandler(statemanager.NewNoopStateManager(),


I think we are trying to do a little too much in one line, even with line breaks. Maybe assign some variables above?

petderek · 2018-01-25T00:19:36Z

agent/config/config_unix.go

@@ -30,7 +30,7 @@ const (

 	// Default cgroup memory system root path, this is the default used if the
 	// path has not been configured through ECS_CGROUP_MEMORY_SUBSYSTEM_PATH


change the comment

richardpen

code LGTM, some minor comments. Also please make sure the header of the modified file is updated to 2018.

richardpen · 2018-01-25T19:08:20Z

agent/resources/resources_linux.go

+func (c *cgroupWrapper) ApplyConfigDependencies(cfg *config.Config) {
+	c.cgroupPath = cfg.CgroupPath
+}
+
 // cgroupInit is used to create the root '/ecs/ cgroup


Can you update this comment?

richardpen · 2018-01-25T19:09:39Z

agent/functional_tests/tests/functionaltests_unix_test.go

@@ -98,6 +98,17 @@ func TestOOMContainer(t *testing.T) {
 	assert.NoError(t, err)
 }

+// TestOOMTask verifies that a task with a memory limit returns an error


Can you add the agent.RequireVersion(">=1.16.0") for this test?

richardpen · 2018-01-25T19:11:02Z

agent/config/types.go

@@ -194,4 +194,7 @@ type Config struct {
 	// OverrideAWSLogsExecutionRole is config option used to enable awslogs
 	// driver authentication over the task's execution role
 	OverrideAWSLogsExecutionRole bool
+
+	// The expected Cgroup root path in the agent


Please use the format CgroupPath is xxx.

richardpen · 2018-01-25T19:23:18Z

agent/resources/resources_linux_test.go

@@ -39,13 +42,18 @@ func TestInitHappyPath(t *testing.T) {
 	defer ctrl.Finish()

 	mockControl := mock_cgroup.NewMockControl(ctrl)
+	mockIO := mock_ioutilwrapper.NewMockIOUtil(ctrl)
+	cgroupPath := fmt.Sprintf("/sys/fs/cgroup/memory/ecs/memory.use_hierarchy")


you don't need the fmt.Sprintf here.

aaithal

lgtm, i have some minor comments.

aaithal · 2018-01-25T21:53:48Z

agent/config/types.go

@@ -194,4 +194,7 @@ type Config struct {
 	// OverrideAWSLogsExecutionRole is config option used to enable awslogs
 	// driver authentication over the task's execution role
 	OverrideAWSLogsExecutionRole bool
+
+	// The expected Cgroup path in the agent is /sys/fs/cgroup


lint: CgroupPath is ...

aaithal · 2018-01-25T21:55:40Z

agent/resources/resources_linux.go

+	// echo 1 > memory.use_hierarchy
+	cgroupRoot := c.cgroupPath
+
+	err = c.ioutil.WriteFile(filepath.Join(cgroupRoot, memorySubsystem, config.DefaultTaskCgroupPrefix, memoryUseHierarchy), []byte(strconv.Itoa(1)), os.FileMode(0))


nit: please split this into multiple lines. Also, please add documentation for all those magic numbers:

[]byte(strconv.Itoa(1)

os.FileMode(0)

oops missed this one for splits, and will doc numbers.

aaithal · 2018-01-30T20:59:44Z

agent/app/agent_test.go

@@ -170,6 +170,8 @@ func TestDoStartTaskLimitsFail(t *testing.T) {
 		saveableOptionFactory.EXPECT().AddSaveable(gomock.Any(), gomock.Any()).AnyTimes(),
 		stateManagerFactory.EXPECT().NewStateManager(gomock.Any(), gomock.Any(), gomock.Any(),
 			gomock.Any(), gomock.Any()).Return(statemanager.NewNoopStateManager(), nil),
+
+		resource.EXPECT().ApplyConfigDependencies(gomock.Any()).AnyTimes(),


can we make this more strict? MinTimes() if the exact number of times is hard to predict?

aaithal · 2018-01-30T21:01:08Z

agent/engine/default.go

-	return NewDockerTaskEngine(cfg, client, credentialsManager, containerChangeEventStream, imageManager, state, metadataManager)
+	metadataManager containermetadata.Manager,
+	resource resources.Resource) TaskEngine {
+	return NewDockerTaskEngine(cfg, client, credentialsManager, containerChangeEventStream, imageManager, state, metadataManager, resource)


nit: we can probably split this into multiple lines as well

aaithal · 2018-01-30T21:01:42Z

agent/resources/resources_linux.go

@@ -93,6 +113,12 @@ func (c *cgroupWrapper) setupCgroup(task *api.Task) error {
 		return errors.Wrapf(err, "resource: setup cgroup: unable to create cgroup at %s for task: %s", cgroupRoot, task.Arn)
 	}

+	// echo 1 > memory.use_hierarchy
+	err = c.ioutil.WriteFile(filepath.Join(c.cgroupPath, memorySubsystem, cgroupRoot, memoryUseHierarchy), []byte(strconv.Itoa(1)), os.FileMode(0))


this too, please split this into multiple lines

how do i keep missing this one. FIXED 😬

aaithal

Just have that testing comment. Otherwise, lgtm

aaithal · 2018-01-30T23:03:48Z

agent/resources/resources_linux.go

@@ -93,6 +113,13 @@ func (c *cgroupWrapper) setupCgroup(task *api.Task) error {
 		return errors.Wrapf(err, "resource: setup cgroup: unable to create cgroup at %s for task: %s", cgroupRoot, task.Arn)
 	}

+	// echo 1 > memory.use_hierarchy
+	memoryHierarchyPath := filepath.Join(c.cgroupPath, memorySubsystem, cgroupRoot, memoryUseHierarchy)
+	err = c.ioutil.WriteFile(memoryHierarchyPath, []byte(strconv.Itoa(1)), os.FileMode(0))


these are still magic numbers. you should be able to const both of these with descriptive names. sorry for missing this in the last round.

ah it's fine. i made this change earlier and i think it got lost while trying to revert back to per task memory.use_hierarchy

sharanyad · 2018-01-30T23:34:07Z

agent/resources/resources_linux.go

+}
+
+// cgroupInit is used to create the root '/ecs/ cgroup and enable
+// memory.use_hierarchy at the '/ecs/' level


Can you edit this comment to reflect the new changes?

good catch.

aaithal · 2018-01-30T23:38:23Z

agent/resources/resources_linux.go

@@ -93,6 +114,13 @@ func (c *cgroupWrapper) setupCgroup(task *api.Task) error {
 		return errors.Wrapf(err, "resource: setup cgroup: unable to create cgroup at %s for task: %s", cgroupRoot, task.Arn)
 	}

+	// echo 1 > memory.use_hierarchy
+	memoryHierarchyPath := filepath.Join(c.cgroupPath, memorySubsystem, cgroupRoot, memoryUseHierarchy)
+	err = c.ioutil.WriteFile(memoryHierarchyPath, []byte(strconv.Itoa(setMemoryHierarchy)), os.FileMode(rootReadOnly))


Apologies for not being more clear about this. I think you can do something like to make it more readable:

const ( rootReadOnlyPermissions = os.FileMode(400) ) var ( enableMemoryHierarchy = []byte(strconv.Itoa(1)) )

since we "Emit a couple of events for the task before cleanup finishes." We should also add corresponding client.EXPECT().StopContainer()

adnxn changed the title ~~[wip] config cgroup root~~ [wip] config cgroup root, enable hierarchy memory limit Jan 19, 2018

adnxn force-pushed the config-cgroup-root branch from 200ad66 to 081449f Compare January 19, 2018 18:51

adnxn changed the title ~~[wip] config cgroup root, enable hierarchy memory limit~~ [wip] enable hierarchy memory limit, config cgroup root Jan 19, 2018

adnxn force-pushed the config-cgroup-root branch from 081449f to d46ad8d Compare January 19, 2018 20:05

adnxn mentioned this pull request Jan 23, 2018

[WIP] Fix an error case with task limits #1147

Closed

8 tasks

adnxn force-pushed the config-cgroup-root branch from d46ad8d to 0299a3a Compare January 23, 2018 21:51

adnxn requested review from aaithal, petderek, narehayrapetyan, yhlee-aws, sharanyad and richardpen and removed request for narehayrapetyan January 23, 2018 21:59

adnxn force-pushed the config-cgroup-root branch 4 times, most recently from 133102a to 79bf8f4 Compare January 24, 2018 01:01

richardpen reviewed Jan 24, 2018

View reviewed changes

adnxn changed the title ~~[wip] enable hierarchy memory limit, config cgroup root~~ enable hierarchy memory limit, config cgroup root Jan 24, 2018

aaithal reviewed Jan 24, 2018

View reviewed changes

petderek reviewed Jan 24, 2018

View reviewed changes

aaithal reviewed Jan 25, 2018

View reviewed changes

petderek approved these changes Jan 25, 2018

View reviewed changes

adnxn force-pushed the config-cgroup-root branch from 9523667 to faa63ba Compare January 25, 2018 00:53

richardpen approved these changes Jan 25, 2018

View reviewed changes

adnxn force-pushed the config-cgroup-root branch 2 times, most recently from 44bdd17 to 4e7b2ad Compare January 25, 2018 21:33

aaithal approved these changes Jan 25, 2018

View reviewed changes

adnxn force-pushed the config-cgroup-root branch from 4e7b2ad to cc2a352 Compare January 25, 2018 23:47

adnxn force-pushed the config-cgroup-root branch 4 times, most recently from dfe8ba9 to 5ed88ec Compare January 30, 2018 20:51

aaithal reviewed Jan 30, 2018

View reviewed changes

petderek and others added 8 commits January 30, 2018 14:49

resources: enable hierarchy memory limit by flag

d22a45e

resources, config: make cgroup root configurable

b57182c

resources: ran gogenerate

4ca9654

engine: pass single instance of resource to engine

5d352f3

resources: update unit test for config cgroup root

1e81a7a

agent: added documentation for cgroup config

2d5f320

agent: config only cgroup root, and formatting

c4cd87d

functional_tests: fix cgroups task limit test

083f41b

adnxn force-pushed the config-cgroup-root branch 2 times, most recently from f653300 to 4d84c8f Compare January 30, 2018 22:57

aaithal reviewed Jan 30, 2018

View reviewed changes

adnxn force-pushed the config-cgroup-root branch from 4d84c8f to caa861b Compare January 30, 2018 23:11

sharanyad reviewed Jan 30, 2018

View reviewed changes

aaithal reviewed Jan 30, 2018

View reviewed changes

adnxn added 2 commits January 30, 2018 16:09

stats: fix breaking tests, formatting

a26f3c1

changelog: update for memory.use_hierarchy fix

0c65868

adnxn force-pushed the config-cgroup-root branch 2 times, most recently from 7f1c65d to cd79407 Compare January 31, 2018 01:06

engine: fix flakey TestRemoveEvents

0c883fa

since we "Emit a couple of events for the task before cleanup finishes." We should also add corresponding client.EXPECT().StopContainer()

adnxn force-pushed the config-cgroup-root branch from cd79407 to 0c883fa Compare January 31, 2018 18:11

aaithal approved these changes Jan 31, 2018

View reviewed changes

adnxn merged commit 27666f6 into aws:dev Jan 31, 2018

adnxn mentioned this pull request Feb 22, 2018

Cannot start containers after upgrade to 1.17.0 on Ubuntu 14.04 #1252

Closed

adnxn deleted the config-cgroup-root branch June 4, 2018 16:01

		@@ -280,6 +280,8 @@ func environmentConfig() (Config, error) {

		dockerStopTimeout := getDockerStopTimeout()

		cgroupMemorySubsystemPath := os.Getenv("ECS_CGROUP_MEMORY_SUBSYSTEM_PATH")

		@@ -30,7 +30,7 @@ const (

		// Default cgroup memory system root path, this is the default used if the
		// path has not been configured through ECS_CGROUP_MEMORY_SUBSYSTEM_PATH

enable hierarchy memory limit, config cgroup root #1195

enable hierarchy memory limit, config cgroup root #1195

Conversation

adnxn commented Jan 19, 2018 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

richardpen 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

adnxn commented Jan 24, 2018

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

adnxn Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petderek 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

aaithal 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

petderek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpen 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

aaithal 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

aaithal 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

adnxn commented Jan 19, 2018 •

edited

Loading

adnxn Jan 24, 2018 •

edited

Loading