Basic implementation of the executor and the run command #6

edigaryev · 2020-07-21T20:23:20Z

With these changes you can try to self-build the CLI go run cmd/cirrus/main.go run -v on macOS.

Some things that still clearly need to be done:

enable Docker-in-Docker in .cirrus.yml
make host.docker.internal work on Linux
create more elaborate tests
handle termination signals and do a proper cleanup

fkorotkov

That looks great! Did the first run on the PR and left some initial thoughts. Will open in IntelliJ tomorrow and will run some things locally to test more.

fkorotkov · 2020-07-21T21:42:59Z

internal/executor/agent/agent.go

+	return nil
+}
+
+func createAgentVolume(


Let's move this method after GetAgentVolume where it's used. It's easier to read when things are in order of usage.

Do you have any convention or a coding style document in mind that specifically addresses this? 🤔

It seems that by changing this we're essentially introducing our own coding style which will complicate drive-by contributions (simply because it needs to be communicated via a written document, lint rule or a PR comment).

On top of that, this adds unnecessary cognitive load to the developer when making a change (due to ambiguities in determining which function should follow which), yet adds a little benefit with the advent of modern IDE's that make sure there's no practical difference between any of the approaches.

That's just how I always wrote code. Don't remember who taught me that but just check how GetAgentVolume ends:

return createAgentVolume(ctx, cli, containerNameTemporary, containerNameFinal, volumeName)

It will be very nice for human readability to see createAgentVolume right after it. We can investigate if golangci-lint has rule for it and if not we can try to implement it.

Another point here is to declare public functions first in the file.

See 8b1d285.

internal/executor/instance/instance.go

internal/executor/rpc/rpc.go

internal/executor/build/task.go

fkorotkov · 2020-07-22T02:26:33Z

internal/executor/build/build.go

+	tasks map[int64]*Task
+
+	// A mutex to guarantee safe access to this build from both the "main loop" and gRPC server handlers
+	Mutex sync.Mutex


I haven't seen any build access pattern that was not thread safe. I think it will be safe to remove Mutex. Did I miss something? Once we have parallel tasks and a fancy cmd UI then we'll need some synchronization there.

I don't like this either, but this is not just about the locking per se, more about the memory barriers and visibility.

Without following the intricate rules of the Go memory model (which is not recommended, see "Advice" section) or implementing an explicit synchronization with e.g. sync.Mutex, there's no guarantee that we'll see up-to-date task status values in Executor.Run(), since gRPC server that we also run handles requests and updates status values in separate goroutines, which easily translates to "runs in a separate thread", which in turn can easily cause problems.

Yes, task's status is the only thing that changes and CLI need to have updated value for figuring out which task to run next. We can just use some atomic in order to do the updates (make status private and use getter and setter for the atomic operation). What do you think?

I thought about wrapping it in an interface method too, but don't you think that in the near future more fields will show up (as we approach functional completeness from the protocol PoV), and it would basically end up with the same mutex wraps?

I just don't like concurrency primitives except channels and atomics. In most of the cases use of a Mutex overcomplicates things. I'll think how we can re-architecture things like instance, executor and build. 🤔

On the second though it seems we'll have serialization of the status update by nature of how things are executed at the moment: a status is updated before container is killed and the access to the task status happens after container is cleaned up in order to start a new task.

I'm currently also don't like how instance is responsible for executing itself. It simplifies things when we are running one task at a time but with the parallel execution we'll need to figure out some sort of instance manager that will be aware of local resources available to the CLI and run as many tasks in parallel as possible and there will need synchronization. Probably with some sort of a channel with task updates.

On the second though it seems we'll have serialization of the status update by nature of how things are executed at the moment: a status is updated before container is killed and the access to the task status happens after container is cleaned up in order to start a new task.

You're right, but The Go Memory Model document clearly recommends against this:

Programs that modify data being simultaneously accessed by multiple goroutines must serialize such access.

To serialize access, protect the data with channel operations or other synchronization primitives such as those in the sync and sync/atomic packages.

If you must read the rest of this document to understand the behavior of your program, you are being too clever.

Don't be clever.

I'll implement the getter/setter methods as discussed above, and if you will keep the history while merging this, we will be able to reference the sync.Mutex solution any time in the future (props to Git).

I'm currently also don't like how instance is responsible for executing itself.

Is it? As far as I can see, it is being executed by the Executor by calling the Run() method on an Instance.

It simplifies things when we are running one task at a time but with the parallel execution we'll need to figure out some sort of instance manager that will be aware of local resources available to the CLI and run as many tasks in parallel as possible and there will need synchronization. Probably with some sort of a channel with task updates.

I agree that an instance manager makes sense in the future, but hey, this is a basic implementation just so we can start moving forward!

Ok, so we are agreeing on moving the mutex to task and having getter / setter? I'll be OK with that and we can revisit it once we'll think about the parallel execution.

Added sync.Mutex-wrapped getter and setter for the task's state field in a9a2096.

Parallelization can be enabled once we'll parallelize the Docker itself too (and this needs to be done on same host, since we're using bind mounts).

internal/testutil/fs.go

This was fixed on the Docker side since Docker Desktop Community 2.3.0.2[1]. [1]: https://docs.docker.com/docker-for-mac/release-notes/#docker-desktop-community-2302 Co-authored-by: Fedor Korotkov <fedor.korotkov@gmail.com>

A missing piece for the previous commit. Co-authored-by: Fedor Korotkov <fedor.korotkov@gmail.com>

Turns out there's a difference between -p and -parallel flags[1][2]: >Note that -parallel only applies within a single test binary. >The 'go test' command may run tests for different packages >in parallel as well, according to the setting of the -p flag >(see 'go help build'). [1]: https://twitter.com/mitchellh/status/900391039252353024 [2]: https://golang.org/cmd/go/#hdr-Testing_flags

1. The only field that's modified in a concurrent fashion right now is the Task.status field. 2. Build fields that were previously protected by sync.Mutex are only modified before RPC server goroutine starts, which is a synchronization point according to The Go Memory Model[1]. Also see discussion in #6. [1]: https://golang.org/ref/mem

fkorotkov

Left a few nit picks. Overall looking excellent! 💪

fkorotkov · 2020-07-23T12:29:55Z

internal/executor/build/task.go

+	ProtoTask *api.Task
+
+	// A mutex to guarantee safe accesses from both the main loop and gRPC server handlers
+	Mutex sync.Mutex


If we are going the mutex way let's use RWMutex here.

See dce79d7.

fkorotkov · 2020-07-23T12:41:28Z

internal/executor/agent/agent.go

+	return nil
+}
+
+func createAgentVolume(


Another point here is to declare public functions first in the file.

edigaryev · 2020-07-27T11:03:39Z

enable Docker-in-Docker in .cirrus.yml

0489a14

make host.docker.internal work on Linux

05ad37b

create more elaborate tests

56ed015, 804c5d7, 293e429, d0ab9a9, 9fe3bd2

handle termination signals and do a proper cleanup

47aff47

internal/executor/rpc/rpc.go

Otherwise on Windows backslashes will be used and the agent or some other command will fail to start.

edigaryev added 4 commits July 21, 2020 23:14

Move TempDir and TempChdir to a separate testutil package

c9b1743

.golangci.yml: document reasons for linter's defaults change

ffa96a8

.golangci.yml: disable maligned linter

912e478

Basic implementation of the executor and the run command

621d4a9

edigaryev requested a review from fkorotkov July 21, 2020 20:23

fkorotkov reviewed Jul 22, 2020

View reviewed changes

edigaryev added 2 commits July 22, 2020 11:34

Move task status enum to build/taskstatus package

9536927

Executor RPC: log heartbeats

7fafcd8

edigaryev mentioned this pull request Jul 22, 2020

Enforce CPU and memory limits #7

Closed

edigaryev added 2 commits July 22, 2020 18:58

CI: run tests in Docker Builder VM

0489a14

CI: disable test parallelization

b28ad54

Parallelization can be enabled once we'll parallelize the Docker itself too (and this needs to be done on same host, since we're using bind mounts).

edigaryev force-pushed the run branch from 8136ac2 to b28ad54 Compare July 22, 2020 15:59

fkorotkov reviewed Jul 22, 2020

View reviewed changes

internal/testutil/fs.go Outdated Show resolved Hide resolved

internal/testutil/fs.go Show resolved Hide resolved

edigaryev and others added 5 commits July 22, 2020 21:08

Work around /var/folders unavailable for mounting on macOS

6756b46

This was fixed on the Docker side since Docker Desktop Community 2.3.0.2[1]. [1]: https://docs.docker.com/docker-for-mac/release-notes/#docker-desktop-community-2302 Co-authored-by: Fedor Korotkov <fedor.korotkov@gmail.com>

Work around /var/folders unavailable for mounting on macOS[1]

612f04b

A missing piece for the previous commit. Co-authored-by: Fedor Korotkov <fedor.korotkov@gmail.com>

Work around host.docker.internal missing on Linux

05ad37b

TestGetAgentVolume: check that the agent's binary exists in the volume

56ed015

edigaryev force-pushed the run branch from e9ebae6 to 56ed015 Compare July 23, 2020 09:22

edigaryev added 4 commits July 23, 2020 12:35

Introduce testutil.FetchedImage() helper

6a828b8

executor: test task authentication and task dependency check

804c5d7

Test clone instruction interception

293e429

fkorotkov approved these changes Jul 23, 2020

View reviewed changes

edigaryev added 3 commits July 23, 2020 15:48

Move GetAgentVolume() to the beginning of agent.go

8b1d285

Use sync.RWMutex instead of sync.Mutex

dce79d7

Catch os.Interrupt and do a proper cleanup

47aff47

edigaryev force-pushed the run branch from 63e98d4 to 47aff47 Compare July 27, 2020 10:23

edigaryev added 2 commits July 27, 2020 13:27

Executor.Run(): use defer to call RPC.Stop()

04327ce

Executor.Run() now returns an error when a single task fails

bd83143

edigaryev added 3 commits July 27, 2020 13:39

RPC.ReportSingleCommand(): log whether the command succeeded or failed

1f89bcb

TestExecutorClone: create a canary file to ensure that clone has worked

d0ab9a9

Add TestExecutorFails to ensure that we register failing builds

9fe3bd2

edigaryev requested a review from fkorotkov July 27, 2020 11:03

fkorotkov reviewed Jul 27, 2020

View reviewed changes

internal/executor/rpc/rpc.go Show resolved Hide resolved

internal/executor/rpc/rpc.go Show resolved Hide resolved

Use path instead of filepath when building the Docker container command

42a247d

Otherwise on Windows backslashes will be used and the agent or some other command will fail to start.

edigaryev merged commit 23d3619 into master Jul 27, 2020

edigaryev deleted the run branch July 27, 2020 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic implementation of the executor and the run command #6

Basic implementation of the executor and the run command #6

edigaryev commented Jul 21, 2020

fkorotkov left a comment

fkorotkov Jul 21, 2020

edigaryev Jul 22, 2020

fkorotkov Jul 22, 2020

fkorotkov Jul 23, 2020

edigaryev Jul 23, 2020

fkorotkov Jul 22, 2020

edigaryev Jul 22, 2020

fkorotkov Jul 22, 2020

edigaryev Jul 22, 2020 •

edited

Loading

fkorotkov Jul 22, 2020

fkorotkov Jul 22, 2020

edigaryev Jul 22, 2020

fkorotkov Jul 22, 2020

edigaryev Jul 23, 2020

fkorotkov left a comment

fkorotkov Jul 23, 2020

edigaryev Jul 23, 2020

fkorotkov Jul 23, 2020

edigaryev commented Jul 27, 2020

Basic implementation of the executor and the run command #6

Basic implementation of the executor and the run command #6

Conversation

edigaryev commented Jul 21, 2020

fkorotkov 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

edigaryev Jul 22, 2020 • 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

fkorotkov 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

edigaryev commented Jul 27, 2020

edigaryev Jul 22, 2020 •

edited

Loading