AllocRunner v2 Feature Branch PR #4792

Tons left to do and lots of churn: 1. No state saving 2. No shutdown or gc 3. Removed AR factory *for now* 4. Made all "Config" structs local to the package they configure 5. Added allocID to GC to avoid a lookup Really hating how many things use *structs.Allocation. It's not bad without state saving, but if AllocRunner starts updating its copy things get racy fast.

I think I like this pattern better as some Config vals are mutable (Alloc) and some aren't and some are used to derive other values and never used directly. Promoting them onto the TR struct is a little more work but is hopefully more clear as to how each value is used.

All hooks only need to be run once. Since only one hook can fail per run there's no need to track errors on a per hook basis.

splits apart local and remote persistence removes some locking *for now*

Restoring calls NewAR -> Restore -> Run NewAR now calls NewTR AR.Restore calls TR.Restore AR.Run calls TR.Run

also updated comments

Deregistration works but difficult to test due to terminal updates not being fully implemented in the new client/ar/tr.

Should probably be moved to an Inspect method in the Driver Plugin world

Updates are applied asynchronously but sequentially

Still WIP as it does not handle deletions.

finished but doesn't handle deleting deeply nested buckets

Saves a tiny bit of cpu and some IO. Sadly doesn't prevent all IO on duplicate writes as the transactions are still created and committed. $ go test -bench=. -benchmem goos: linux goarch: amd64 pkg: github.com/hashicorp/nomad/helper/boltdd BenchmarkWriteDeduplication_On-4 500 4059591 ns/op 23736 B/op 56 allocs/op BenchmarkWriteDeduplication_Off-4 300 4115319 ns/op 25942 B/op 55 allocs/op

Allow alloc and task runners to persist their own state when something changes instead of periodically syncing all state.

* Stopping an alloc is implemented via Updates but update hooks are *not* run. * Destroying an alloc is a best effort cleanup. * AllocRunner destroy hooks implemented. * Disk migration and blocking on a previous allocation exiting moved to its own package to avoid cycles. Now only depends on alloc broadcaster instead of also using a waitch. * AllocBroadcaster now only drops stale allocations and always keeps the latest version. * Made AllocDir safe for concurrent use Lots of internal contexts that are currently unused. Unsure if they should be used or removed.

Leaving fingerprinters in case that interface changes with plugins.

And test thoroughly.

minimize passed dependencies to ease testing

Also remove initial alloc from broadcaster as it just caused useless extra processing.

* UpdateState: set state, append event, persist, update servers * EmitEvent: append event, persist, update servers * AppendEvent: append event, persist AppendEvent may not even have to persist, but for the sake of correctness I'm going with that for now.

Make it match TaskRunner update hook behavior

Append when there's nothing blocking between appending and sending an update to the server.

Still more testing to do, but I want to get this panic fixed ASAP. All new tests pass with -race

* vendor: bump libcontainer and docker to remove Sirupsen imports * vendor: fix bad vendoring of archive package * vendor: fix api changes to cgroups in executor * vendor: fix docker api changes * vendor: update github.com/Azure/go-ansiterm to use non capitalized logrus import

* fifo: add new fifo package for named pipes

@dadgar

Use doneCh like @dadgar suggested in the original PR. Thoroughly test hook as concurrent Update calls make for a tricky concurrency problem.

Updated to hclog. It exposed fields that required an unexported lock to access. Created a getter methodn instead. Only old allocrunner currently used this feature.

Tested except for the net/rpc specific error case which may need changing in the gRPC world.

* client/executor: refactor client to remove interpolation * executor: POC libcontainer based executor * vendor: use hashicorp libcontainer fork * vendor: add libcontainer/nsenter dep * executor: updated executor interface to simplify operations * executor: implement logging pipe * logmon: new logmon plugin to manage task logs * driver/executor: use logmon for log management * executor: fix tests and windows build * executor: fix logging key names * executor: fix test failures * executor: add config field to toggle between using libcontainer and standard executors * logmon: use discover utility to discover nomad executable * executor: only call libcontainer-shim on main in linux * logmon: use seperate path configs for stdout/stderr fifos * executor: windows fixes * executor: created reusable pid stats collection utility that can be used in an executor * executor: update fifo.Open calls * executor: fix build * remove executor from docker driver * executor: Shutdown func to kill and cleanup executor and its children * executor: move linux specific universal executor funcs to seperate file * move logmon initialization to a task runner hook * client: doc fixes and renaming from code review * taskrunner: use shared config struct for logmon fifo fields * taskrunner: logmon only needs to be started once per task

From review comments on #4649 left post-merge.

Driver plugin framework to facilitate development of driver plugins. Implementing plugins only need to implement the DriverPlugin interface. The framework proxies this interface to the go-plugin GRPC interface generated from the driver.proto spec. A testing harness is provided to allow implementing drivers to test the full lifecycle of the driver plugin. An example use: func TestMyDriver(t *testing.T) { harness := NewDriverHarness(t, &MyDiverPlugin{}) // The harness implements the DriverPlugin interface and can be used as such taskHandle, err := harness.StartTask(...) }

added wrapper struct for plugin.ReattachConfig to better handle serialization

…ilities

Specify that Alloc() does not return updated task states.

* GetClientAlloc() gains nothing from using allAllocs() * getAllocatedResources was calling getAllocRunners() twice

mock_driver wasn't behaving like other driver handles.

"Ask forgiveness, not permission." Instead of peaking at TaskStates (which are no longer updated on the AR.Alloc() view of the world) to only read logs for running tasks, just try to read the logs and improve the error handling if they don't exist. This should make log streaming less dependent on AR/TR behavior. Also fixed a race where the log streamer could exit before reading an error. This caused no logs or errors to be displayed sometimes when an error occurred.

Multiple receivers raced for the WaitResult when killing tasks which could lead to a deadlock if the "wrong" receiver won. Wrap handlers in an ugly little proxy to avoid this. At first I wanted to push this into drivers, but the result is tied to the TR's handle lifecycle -- not the lifecycle of an alloc or task.

httptest.ResponseRecorder exposes a bytes.Buffer which we were reading and writing concurrently to test streaming log APIs. This is a race, so I wrapped the struct in a lock with some helpers.

The interesting decision in this commit was to expose AR's state and not a fully materialized Allocation struct. AR.clientAlloc builds an Alloc that contains the task state, so I considered simply memoizing and exposing that method. However, that would lead to AR having two awkwardly similar methods: - Alloc() - which returns the server-sent alloc - ClientAlloc() - which returns the fully materialized client alloc Since ClientAlloc() could be memoized it would be just as cheap to call as Alloc(), so why not replace Alloc() entirely? Replacing Alloc() entirely would require Update() to immediately materialize the task states on server-sent Allocs as there may have been local task state changes since the server received an Alloc update. This quickly becomes difficult to reason about: should Update hooks use the TaskStates? Are state changes caused by TR Update hooks immediately reflected in the Alloc? Should AR persist its copy of the Alloc? If so, are its TaskStates canonical or the TaskStates on TR? So! Forget that. Let's separate the static Allocation from the dynamic AR & TR state! - AR.Alloc() is for static Allocation access (often for the Job) - AR.AllocState() is for the dynamic AR & TR runtime state (deployment status, task states, etc). If code needs to know the status of a task: AllocState() If code needs to know the names of tasks: Alloc() It should be very easy for a developer to reason about which method they should call and what they can do with the return values.

Also expose NoopDB for use in tests.

Sadly can't move the fingerprint timeout tweak into the helper due to circular imports.

Although the really exciting change is making WaitForRunning return the allocations that it started. This should cut down test boilerplate significantly.

If ar.state.TaskStates has not been set, set it on the copy of ar.state. That keeps ar.state manipulations in one location and allows AllocState to only acquire read-locks.

…ontext instead

client: fingerprint driver plugins

Now passing the TaskDir struct to prestart hooks instead of just the root task dir itself as dispatch needs local/.

PR #4392 was merged to master *after* allocrunnerv2 was branched, so the client-specific portions must be ported from master to arv2.

Do not register mock_driver on release builds.

from #4777 comments

Was mock_driver before plugins, so keep the name.

was causing typed nil interface panics and served no purpose

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AllocRunner v2 Feature Branch PR #4792

AllocRunner v2 Feature Branch PR #4792

Commits on Oct 16, 2018