worker: add Destroy functionality #4784

cirocosta · 2019-11-18T21:06:04Z

Why do we need this PR?

As part of #4783, this PR adds the
functionality required to get garden's Destroy working with containerd.

Changes proposed in this pull request

backend's Destroy high-level implementation
Destroy integration
backend's Lookup high-level implementation
~~Lookup integration~~
Stop task within running container before container.Delete
~~add Dockerfile to help integration tests run on OSX~~ split out to separate story Create a dev Dockerfile to support running integration tests on any OS #4850

Contributor Checklist

Unit tests
Integration tests (if applicable)
~~Updated documentation (located at https://github.com/concourse/docs)~~
~~Updated release notes (located at https://github.com/concourse/concourse/tree/master/release-notes)~~

Reviewer Checklist

Code reviewed
Tests reviewed
PR acceptance performed
~~Documentation reviewed~~
~~Release notes reviewed~~
~~New config flags added?~~

worker/backend/libcontainerd/client.go

worker/backend/backend.go

worker/backend/libcontainerd/client.go

deniseyu · 2019-11-26T22:02:44Z

End of Tuesday notes:

We learned that in order to destroy a container with a Task inside it, we first have to shut down the Task. This is something that Guardian did for us behind the scenes, but with containerd we will have to do it ourselves. To minimize surprise, it would be good to bring across the same behaviours (e.g. which termination signal is used, how long are the timeouts, etc). Here is an example of how a container must be gracefully shut down using containerd's API: https://containerd.io/docs/getting-started/
The task shutdown API calls are asynchronous, because containerd daemon can take any length of time to actually remove the container. To handle this we'll need to Wait on an exit code or some other signal.
There is a Container.Stop method in the Garden API which we will eventually want to connect up to the task shutdown logic that we're currently implementing inside the Destroy. We'll also want to refactor that out into its own method.
For tomorrow: ~~let's experiment and see if we can bring up a Container without any task defined. Containerd blocks deletion when there's a running task, but what if there's no task at all??~~ Update for academic curiosity we did this, and found that it is indeed possible to create a container with no task running, which is also subsequently deletable without having to do any task operations

deniseyu · 2019-11-28T19:36:41Z

Here's a useful script to destroy all containers across all containerd namespaces:

https://gist.github.com/deniseyu/55c573026a83e3e1b0011201bcf7b3fc

💥

deniseyu · 2019-11-28T22:28:13Z

End of Thursday notes (we should probably rename our branch 😭)

We forgot that before deleting a container, we have to call Task.Delete. I've added that to the case <- ctx.Done() branch of the Task.Kill logic.
That broke integration tests, which should now be fixed (NOTE: you have to un-comment out the Task.Delete in case <- exitStatus -- the real integration tests run fast and don't hit the timeout. It's just commented out now to remind Next Week Denise to add unit tests). We are very close to having the lifecycle of a container implemented!
I made a failed attempt to unit-test channels. HALP ciro

Still need to do:

Properly test the non-timeout branch of Task.Kill. The rest of the unit tests currently pass, but only because the context timeout is being triggered after I hacked some faked methods to return a channel that will never return -- this isn't the normal path and should probably not be the default behavior for "happy path" tests.
Think about parameterizing the context cancellation deadline. It's a pain to test a 10s timeout (unless anyone knows a Go equivalent for timecop?)
Ciro mentioned that perhaps the namespace can hold onto it - still needs some investigation.
We may want to dig deeper into exit codes, and what non-zero exit codes might imply. But maybe we don't care and just trust that things are dandy if no error is returned from containerd.
Consider abstracting the Task.Wait + Task.Kill + Task.delete logic into a named method that can become our implementation of Garden's Container.Stop interface. Open to debate on what philosophically constitutes a Container.Stop in the containerd world though

worker/backend/backend_test.go

deniseyu · 2019-12-05T19:15:27Z

Going to split out Lookup into a different task, because writing that integration test would be a good opportunity to build out Handle(). We also set up a Dockerfile for running containerd integration tests which will be a separate PR.

deniseyu · 2019-12-05T22:35:02Z

End of Thursday notes:

@pivotal-bin-ju and I finished building out the local integration test tooling and we're able to run and develop the tests in Docker now, woo. We thought about adding a test for Lookup, but decided that since Destroy implicitly calls Lookup, for now there's not much value in adding an integration test for it - this may change if we want to do more interesting things as part of Lookup.

Next week we'll work on filling out the rest of the Garden client and container and process interfaces! Maybe once we get the process stuff working (can refer to Alex's spike branch) we'll be able to see a basic task running in the UI.

- still need to actually invoke containerd's Destroy and add integration tests - add typed validation error for missing inputs Signed-off-by: Denise Yu <dyu@pivotal.io>

Signed-off-by: Denise Yu <dyu@pivotal.io>

- backend: lookup calls client's `GetContainer`, which wraps containerd's LoadContainer ps.: this functionality still lacks an integration test. Signed-off-by: Denise Yu <dyu@pivotal.io> Co-authored-by: Ciro S. Costa <cscosta@pivotal.io>

need to push in order to test integration tests on linux SQUASH ME LATER Signed-off-by: Denise Yu <dyu@pivotal.io>

Signed-off-by: Denise Yu <dyu@pivotal.io>

- add task fakes - start implementing finding & killing task, a prereq for removing the container Signed-off-by: Denise Yu <dyu@pivotal.io>

- This enables us to test the "happy path" where a task can be deleted before the context timeout triggers. This means that containerd was able to gracefully shut down a running task with SIGTERM. Signed-off-by: Bin Ju <bju@pivotal.io>

Reset task kill timeout to 10s Signed-off-by: Bin Ju <bju@pivotal.io> Signed-off-by: Denise Yu <dyu@pivotal.io>

worker/backend/backend.go

kcmannem · 2019-12-10T19:30:10Z

worker/backend/backend.go

+		return InputValidationError{}
+	}
+
+	container, err := b.client.GetContainer(ctx, handle)


idk what containerd does to lookup containers but I feel like we should include a timeout for this context as well.

yea, that's true - we can add this in the next PR where the client timeout is configurable, we'll just decorate every context at the start of each method with the timeout.

added it as a to-do item here: #4783 (comment)

Task runs don't have timeouts, but pretty much every other operation does :)

worker/backend/backend.go

Signed-off-by: Bin Ju <bju@pivotal.io>

worker/backend/backend.go

xtreme-sameer-vohra · 2019-12-11T17:39:37Z

worker/backend/backend.go

+		_, err = task.Delete(ctx) // todo: we're swallowing exitcodes in both these forks, do we care?
+		return err
+	case <-ctx.Done():
+		err = task.Kill(ctx, syscall.SIGKILL) // should return GRPC DeadlineExceeded error type, wrapped up


This ctx has already expired. So task.Kill may or may not be a noop. Does it make more sense to have another ctx?

xtreme-sameer-vohra · 2019-12-11T17:50:06Z

worker/backend/backend.go

+
+	select {
+	case <-exitStatus:
+		_, err = task.Delete(ctx) // todo: we're swallowing exitcodes in both these forks, do we care?


There is an edge case where the task ran to completion prior to being killed and that result gets swallowed currently.

worker/backend/backend.go

xtreme-sameer-vohra · 2019-12-11T17:56:23Z

worker/backend/backend.go

@@ -148,7 +216,20 @@ func (b *Backend) BulkMetrics(handles []string) (metrics map[string]garden.Conta
 //
 // Errors:
 // * Container not found.
-func (b *Backend) Lookup(handle string) (container garden.Container, err error) { return }
+func (b *Backend) Lookup(handle string) (garden.Container, error) {
+	ctx := namespaces.WithNamespace(context.Background(), b.namespace)


Does it make sense for this to be a context.TODO as we intend it to be replaced ?

worker/backend/backend.go

pivotal-bin-ju · 2019-12-11T19:17:00Z

worker/backend/backend.go

+	}
+
+	return b.client.Destroy(ctx, handle)
+}


what about to refactor the logic as below (move killTasks into else branch, to have less return route path):

task, err := container.Task(ctxWithTimeout, nil) if err != nil { //error occurs if !errdefs.IsNotFound(err) { //a real error but the task can not be found return ClientError { InnerError: err } } // do nothing if we could not find the task } else { // kill the task err = killTasks(ctxWithTimeout, task) if err != nil { return ClientError{ InnerError: err } } } // no matter the task exists or is killed, the container should be destroyed anyway. err = b.client.Destroy(ctxWithTimeout, handle) if err != nil { return ClientError{ InnerError: err } }

pivotal-bin-ju · 2019-12-11T19:18:42Z

worker/backend/backend.go

+	const maxTaskKillWaitTime = 10 * time.Second
+
+	if handle == "" {
+		return InputValidationError{}


would it be better if the handle check is at the first line of the function.?

pivotal-bin-ju · 2019-12-11T19:33:44Z

worker/backend/backend.go

+	}
+
+	select {
+	case <-exitStatus:


Maybe we should not ignore the exitCode. From the comment of the source code:

// ExitStatus encapsulates a process's exit status. // It is used by `Wait()` to return either a process exit code or an error

It may not return the error immediately after we SIGTERM the task.

pivotal-bin-ju · 2019-12-11T19:35:50Z

worker/backend/backend.go

+		_, err = task.Delete(ctx) // todo: we're swallowing exitcodes in both these forks, do we care?
+		return err
+	case <-ctx.Done():
+		err = task.Kill(ctx, syscall.SIGKILL) // should return GRPC DeadlineExceeded error type, wrapped up


We may need another wait after we SIGKILL. the task.Kill will return the status of GRPC call.

xtreme-sameer-vohra · 2019-12-11T19:44:10Z

worker/backend/backend_test.go

+
+func (s *BackendSuite) TestLookupGetContainer() {
+	s.client.GetContainerReturns(new(libcontainerdfakes.FakeContainer), nil)
+	container, err := s.backend.Lookup("non-existent-handle")


non-existent-handle could be changed to some existing container

yes this test is bad right now -- related to the incomplete implementation of Lookup. Will be sorted out later 😂

xtreme-sameer-vohra · 2019-12-11T19:59:27Z

worker/backend/backend_test.go

+	s.Equal(testNamespace, namespace)
+}
+
+func (s *BackendSuite) TestDestroyWaitError() {


TestDestroyWaitError -> TestDestroyTaskWaitError

xtreme-sameer-vohra · 2019-12-11T20:11:32Z

worker/backend/backend_test.go

+
+	err := s.backend.Destroy("some-handle")
+
+	s.Equal(2, fakeTask.KillCallCount())


For correctness, assert that syscall.SIGKILL was passed to fakeTask.Kill

xtreme-sameer-vohra · 2019-12-11T20:19:07Z

worker/backend/libcontainerd/client.go

+		container containerd.Container, err error,
+	)
+
+	// Destroy stops any running tasks on a container and remove the container.


nit: removes

xtreme-sameer-vohra

Added some inline comments

deniseyu · 2019-12-11T22:18:13Z

thanks @xtreme-sameer-vohra for the inline comments! We implemented your suggestions but we're going to fold them into the top of this branch: #4881 because in the last 2 days we restructured the backend and I'm a coward for merge conflicts 🏋️‍♀️

Implemented changes in branch at #4881

xtreme-sameer-vohra

Chatted with Denise and Bin, suggestions for this PR will be incorporated in https://github.com/concourse/concourse/pull/4881/files

cirocosta commented Nov 18, 2019

View reviewed changes

worker/backend/libcontainerd/client.go Show resolved Hide resolved

worker/backend/backend.go Show resolved Hide resolved

worker/backend/backend.go Show resolved Hide resolved

cirocosta changed the title ~~worker: add initial Destroy functionality~~ worker: add Destroy functionality Nov 18, 2019

deniseyu mentioned this pull request Nov 18, 2019

epic: implement Garden API for containerd #4783

Closed

30 tasks

pivotal-bin-ju reviewed Nov 20, 2019

View reviewed changes

worker/backend/libcontainerd/client.go Outdated Show resolved Hide resolved

deniseyu force-pushed the containerd-monday branch from 2a87dd6 to 5dfa7e8 Compare November 26, 2019 22:15

deniseyu reviewed Dec 1, 2019

View reviewed changes

worker/backend/backend_test.go Outdated Show resolved Hide resolved

deniseyu force-pushed the containerd-monday branch from fbe21a4 to 7251ebf Compare December 5, 2019 19:00

deniseyu marked this pull request as ready for review December 5, 2019 19:02

deniseyu requested a review from a team December 5, 2019 19:02

deniseyu force-pushed the containerd-monday branch from 7251ebf to 0dfe3c4 Compare December 5, 2019 19:04

deniseyu force-pushed the containerd-monday branch 2 times, most recently from d132766 to f65bb43 Compare December 5, 2019 19:16

deniseyu requested a review from xtreme-sameer-vohra December 5, 2019 19:17

Ciro S. Costa and others added 8 commits December 10, 2019 14:14

worker: add initial Destroy functionality

b178f4a

- still need to actually invoke containerd's Destroy and add integration tests - add typed validation error for missing inputs Signed-off-by: Denise Yu <dyu@pivotal.io>

worker: add failing integration test for Destroy

0068303

Signed-off-by: Denise Yu <dyu@pivotal.io>

worker: implement Lookup for containerd be

a974fae

- backend: lookup calls client's `GetContainer`, which wraps containerd's LoadContainer ps.: this functionality still lacks an integration test. Signed-off-by: Denise Yu <dyu@pivotal.io> Co-authored-by: Ciro S. Costa <cscosta@pivotal.io>

worker: fill out client logic

5e5267f

need to push in order to test integration tests on linux SQUASH ME LATER Signed-off-by: Denise Yu <dyu@pivotal.io>

Add testdata dir for integration tests

2b1c0e3

Signed-off-by: Denise Yu <dyu@pivotal.io>

worker: Start adding functionality for destroy

60dfef7

- add task fakes - start implementing finding & killing task, a prereq for removing the container Signed-off-by: Denise Yu <dyu@pivotal.io>

worker: Add unit test stub for Task.Wait()

c7fa765

- This enables us to test the "happy path" where a task can be deleted before the context timeout triggers. This means that containerd was able to gracefully shut down a running task with SIGTERM. Signed-off-by: Bin Ju <bju@pivotal.io>

worker: Add documentation for client methods

932eaf0

Reset task kill timeout to 10s Signed-off-by: Bin Ju <bju@pivotal.io> Signed-off-by: Denise Yu <dyu@pivotal.io>

deniseyu force-pushed the containerd-monday branch from f65bb43 to 932eaf0 Compare December 10, 2019 19:14

kcmannem reviewed Dec 10, 2019

View reviewed changes

worker/backend/backend.go Show resolved Hide resolved

kcmannem reviewed Dec 10, 2019

View reviewed changes

worker: Stylistic fixes for consistency

62db7db

Signed-off-by: Bin Ju <bju@pivotal.io>