Add docker runtime for agent #730

chrisdoherty4 · 2023-05-11T02:40:38Z

Summary

This PR introduces a Docker runtime that is usable by the agent to run actions. The code is intended to introduce a functional runtime, therefore it doesn't implement the full set of features available on actions such as volumes; the additional features will be implemented separately.

This PR also doesn't address debugability of actions - for example, action logs are discarded - though this may not be required if we continue with syslog and routing container logs to an external service.

Future work

The tests are integration tests that ensure the code is robust enough. Some functionality like ensuring volume configuration, environment variables, commands and args are all configured correctly would be better implemented as unit tests. A subsequent PR will look to introduce an interface so we can easily test these feature as they're added.

codecov · 2023-05-11T02:46:35Z

Codecov Report

Merging #730 (d36ffe4) into main (3723588) will increase coverage by 0.99%.
The diff coverage is 66.66%.

❗ Current head d36ffe4 differs from pull request most recent head 3e45b96. Consider uploading reports for the commit 3e45b96 to get more accurate results

@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
+ Coverage   55.32%   56.31%   +0.99%     
==========================================
  Files          25       28       +3     
  Lines        1099     1243     +144     
==========================================
+ Hits          608      700      +92     
- Misses        475      511      +36     
- Partials       16       32      +16

Impacted Files	Coverage Δ
internal/agent/runtime/docker.go	`62.63% <62.63%> (ø)`
internal/agent/runtime/internal/failure_files.go	`74.46% <74.46%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

moadqassem · 2023-05-13T17:53:02Z

internal/ptr/ptr.go

@@ -0,0 +1,90 @@
+/*


Would it make sense to rename the folder where this file is created to utils and keep the file name to ptr.go

We tend to avoid top level util packages and sign up to every package having at least some scoped purpose.

internal/agent/runtime/docker.go

moadqassem

LGTM 🚀

chrisdoherty4 · 2023-05-15T15:52:06Z

internal/agent/runtime/docker.go

+type DockerOption func(*Docker)
+
+// WithLogger returns an option to configure the logger on a Docker instance.
+func WithLogger(log logr.Logger) DockerOption {


I've realized the logger is actually required because we log when a container couldn't be gracefully stopped. I'll tweak this when adding the other features as noted in the OP.

jacobweinstock · 2023-05-18T17:14:21Z

internal/agent/runtime/docker.go

+	// Always try to remove the container on exit.
+	defer func() {
+		// We can't use the context passed to Run() as it may have been cancelled.
+		err := d.client.ContainerRemove(context.Background(), create.ID, types.ContainerRemoveOptions{})


should we enable all the ContainerRemoveOptions?

type ContainerRemoveOptions struct { RemoveVolumes bool RemoveLinks bool Force bool }

On Mac with Docker Desktop I've seen force leave objects lying on disk that couldn't be cleaned up by a following docker system prune --volumes (I had to manually remove files). I don't know how widespread that impact is, or how challenging it would be to figure out, so I'm not sure if we'd want that enabled, particularly given this is all in-memory and goes away on restart. Given action names are unique I don't foresee any conflicts with respect to launching more actions.

I don't think we want to remove volumes because we allow users to share data between actions via volumes.

I don't see any issues with link removal.

With that in mind, what do you think about enabling link removal only?

On container name uniqueness, this has made me realized the action ID is unique within the context of a workflow, but if a second workflow executes (i.e. the first didn't restart the machine) there could be a collision. I think we could fix that by committing to UUIDs for action IDs.

thanks, Chris.

You're right we probably shouldn't remove volumes.

Currently, the main use case for the agent is to run in Hook. The status-quo is to use Force: true, so we know it works in Hook. I'm concerned about resource constrained machines where memory and disk space in Hook are an issue.

OK, lets stick with the status quo and force delete. The volumes will likely be in-memory and cleaned on restart anyway.

Should be addressed.

jacobweinstock · 2023-05-19T16:03:43Z

internal/agent/runtime/docker.go

+// Run satisfies agent.ContainerRuntime.
+func (d *Docker) Run(ctx context.Context, a workflow.Action) error {
+	// We need the image to be available before we can create a container.
+	image, err := d.client.ImagePull(ctx, a.Image, types.ImagePullOptions{})


might think about adding retries here. a transient issue to the registry could cause a whole workflow to fail.

I went with github.com/avast/retry-go as it seems quite modular and has all the bits you'd expect from a retry library.

jacobweinstock · 2023-05-19T16:09:58Z

internal/agent/runtime/docker.go

+		fn(o)
+	}
+
+	if o.log.GetSink() == nil {


if you put the logr.Discard into o := &Docker{}, i dont think you'll need this check. Same with the client setting below. These will be the defaults and then only overwritten if a consumer passes either one of them in as a DockerOption.

chrisdoherty4 · 2023-05-30T02:19:19Z

internal/agent/runtime/docker.go

+		// In rare cases this may create orphaned volumes that the Docker CLI won't clean up.
+		opts := types.ContainerRemoveOptions{
+			Force:       true,
+			RemoveLinks: true,


This is causing issues in tests and its not clear why. I no longer think I understand what it does so I'm disabling; we can enable it at a later date.

jacobweinstock · 2023-06-03T14:51:59Z

internal/agent/runtime/docker.go

+}
+
+// NewDocker creates a new Docker instance.
+func NewDocker(opts ...DockerOption) (*Docker, error) {


nit: variables, types, and New functions go at the top of the file.

jacobweinstock · 2023-06-03T15:12:31Z

internal/agent/runtime/docker.go

+		Env:   toDockerEnv(a.Env),
+	}
+
+	failureFiles, err := internal.NewFailureFiles()


Coming back to this after a bit the interaction between a runtime implementation, the agent, the eventing system, and the action, etc being run in terms of failures feels difficult to follow with just the code. I think we should figure out a way to make this very understandable. docs, code comments, etc. I just want to be sure that all the different parts of the code are resilient and easy to understand how they fit into the whole failure notification system.

jacobweinstock · 2023-06-03T15:22:07Z

internal/agent/runtime/internal/failure_files.go

+
+// Close closes all files tracked by f.
+func (f *FailureFiles) Close() error {
+	os.Remove(f.reason.Name())


if the model is going to be to not capture the errors here, maybe we should change the interface to not return an error?

Also, the name Close actually doesn't feel accurate here. deleting files is different than closing them. is this satisfying an interface method?

jacobweinstock · 2023-06-03T15:24:08Z

internal/agent/runtime/internal/failure_files.go

+	return strings.TrimRight(message.String(), "\n"), nil
+}
+
+func (f *FailureFiles) ToError() error {


Code comment?

If we inject a data structure for failure files to runtimes I suspect this method will be unnecessary so defering.

jacobweinstock · 2023-06-04T00:21:39Z

internal/agent/runtime/docker.go

+	// TODO: Support all the other things on the action such as volumes.
+	cfg := container.Config{
+		Image: a.Image,
+		Env:   toDockerEnv(a.Env),


we might want to set the failure file locations to be env vars for all containers. that way actions dont need to "know" where this location is and we can change it if needed. action writers just look up the env var and get the files.

chrisdoherty4 · 2023-06-14T14:39:05Z

In an effort to move this along we've decided to merge this. This comment serves as a summary of outstanding items:

Inject failure files as they should be common across runtimes.
Rename Close() for failure files to Remove() and drop returning an error.
Restructure the sources to follow Code Review comments guideliens for source organization.

The new agent requires a container runtime to execute workflow actions. This commit introduces a Docker runtime. Signed-off-by: Chris Doherty <chris.doherty4@gmail.com>

chrisdoherty4 force-pushed the feature/runtime branch from 259e277 to 2e6111e Compare May 11, 2023 02:40

chrisdoherty4 force-pushed the feature/runtime branch 3 times, most recently from bbeda78 to b3b3d4a Compare May 13, 2023 14:57

chrisdoherty4 marked this pull request as ready for review May 13, 2023 15:02

chrisdoherty4 requested review from moadqassem and jacobweinstock May 13, 2023 15:03

moadqassem reviewed May 13, 2023

View reviewed changes

moadqassem previously approved these changes May 15, 2023

View reviewed changes

chrisdoherty4 force-pushed the feature/runtime branch from b3b3d4a to 076af5a Compare May 15, 2023 15:48

chrisdoherty4 commented May 15, 2023

View reviewed changes

chrisdoherty4 mentioned this pull request May 15, 2023

Add transport implementation for agent #734

Merged

jacobweinstock reviewed May 18, 2023

View reviewed changes

jacobweinstock reviewed May 19, 2023

View reviewed changes

chrisdoherty4 dismissed moadqassem’s stale review via 139f6b6 May 30, 2023 00:32

chrisdoherty4 force-pushed the feature/runtime branch 4 times, most recently from 6fbb52e to 3db4cdf Compare May 30, 2023 00:59

chrisdoherty4 commented May 30, 2023

View reviewed changes

chrisdoherty4 force-pushed the feature/runtime branch 2 times, most recently from 42a3072 to 1cb2e5d Compare May 30, 2023 02:22

chrisdoherty4 requested a review from jacobweinstock May 30, 2023 02:24

jacobweinstock reviewed Jun 3, 2023

View reviewed changes

jacobweinstock reviewed Jun 4, 2023

View reviewed changes

jacobweinstock approved these changes Jun 13, 2023

View reviewed changes

chrisdoherty4 force-pushed the feature/runtime branch from 1cb2e5d to 8c2d736 Compare June 14, 2023 14:39

Add docker runtime for agent

3e45b96

The new agent requires a container runtime to execute workflow actions. This commit introduces a Docker runtime. Signed-off-by: Chris Doherty <chris.doherty4@gmail.com>

chrisdoherty4 force-pushed the feature/runtime branch from 8c2d736 to 3e45b96 Compare June 14, 2023 14:41

chrisdoherty4 added the ready-to-merge Signal to Mergify to merge the PR. label Jun 14, 2023

mergify bot merged commit 07c3595 into tinkerbell:main Jun 14, 2023

chrisdoherty4 mentioned this pull request Jun 15, 2023

Handle review feedback from initial agent runtime implementation #755

Open

chrisdoherty4 mentioned this pull request Jul 17, 2023

[Agent] agent docker runtime unit test require docker #770

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docker runtime for agent #730

Add docker runtime for agent #730

chrisdoherty4 commented May 11, 2023 •

edited

Loading

codecov bot commented May 11, 2023 •

edited

Loading

moadqassem May 13, 2023

chrisdoherty4 May 14, 2023

moadqassem left a comment

chrisdoherty4 May 15, 2023

jacobweinstock May 18, 2023

chrisdoherty4 May 18, 2023 •

edited

Loading

chrisdoherty4 May 18, 2023 •

edited

Loading

jacobweinstock May 19, 2023

chrisdoherty4 May 30, 2023

chrisdoherty4 May 30, 2023

jacobweinstock May 19, 2023

chrisdoherty4 May 30, 2023 •

edited

Loading

jacobweinstock May 19, 2023

chrisdoherty4 May 30, 2023

chrisdoherty4 May 30, 2023

jacobweinstock Jun 3, 2023

jacobweinstock Jun 3, 2023 •

edited

Loading

jacobweinstock Jun 3, 2023

jacobweinstock Jun 3, 2023

jacobweinstock Jun 3, 2023

chrisdoherty4 Jun 14, 2023

jacobweinstock Jun 4, 2023

chrisdoherty4 commented Jun 14, 2023

Add docker runtime for agent #730

Add docker runtime for agent #730

Conversation

chrisdoherty4 commented May 11, 2023 • edited Loading

Summary

Future work

codecov bot commented May 11, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moadqassem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdoherty4 May 18, 2023 • edited Loading

Choose a reason for hiding this comment

chrisdoherty4 May 18, 2023 • 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

chrisdoherty4 May 30, 2023 • 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

jacobweinstock Jun 3, 2023 • 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

chrisdoherty4 commented Jun 14, 2023

chrisdoherty4 commented May 11, 2023 •

edited

Loading

codecov bot commented May 11, 2023 •

edited

Loading

chrisdoherty4 May 18, 2023 •

edited

Loading

chrisdoherty4 May 18, 2023 •

edited

Loading

chrisdoherty4 May 30, 2023 •

edited

Loading

jacobweinstock Jun 3, 2023 •

edited

Loading