Add network lifecycle management #5626

nickethier · 2019-04-29T17:45:29Z

This PR adds a few notable changes to support lifecycle management of network namespaces.

Alloc Runner Hooks

The first big change here was adding a call to initialize platform dependent hooks since the network hook is currently only supported on linux. There is new dependency here on a small packaged names ns that only builds on linux due to references to the syscall package that are platform dependent.

The second is adding the actual network hook that includes checking if a driver needs to initialize the network.

`plugins/drivers` Changes

New proto messages and rpcs were added to support network namespaces. These aren't implemented by any driver in this PR but I did add a helper struct that can be embedded when driver network management is not needed (aka everything but docker). The typical proto to go conversion funcs are there as well.

Adds a new Prerun and Postrun hooks to manage set up of network namespaces on linux. Work still needs to be done to make the code platform agnostic and support Docker style network initalization.

notnoop

I did one round of code comments but I'll read other networking PRs for more context before reviewing it again.

client/allocrunner/alloc_runner_hooks.go

client/allocrunner/network_hook_linux.go

plugins/drivers/driver.go

client/allocrunner/alloc_runner_linux.go

notnoop · 2019-05-13T18:07:15Z

client/allocrunner/network_hook_linux.go

+			return
+		}
+
+		// Put this thread back to the orig ns, since it might get reused (pre go1.10)


curious how you noticed this issue? I assume you aren't running golang 1.10?

This func is actually borrowed from: https://github.com/containernetworking/plugins/blob/master/pkg/testutils/netns_linux.go#L34

Though the background on this is covered pretty well here if interested: https://github.com/containernetworking/plugins/tree/master/pkg/ns#namespace-switching

Awesome - might be nice to include a link; also may need to copy the whole file along with the copyright and license notice on top.

+1 to what @notnoop said. This code is 💥 and we shouldn't copy it without proper attribution.

schmichael

I think it might be worth refactoring the default device manager out of the network hook and seeing if we can ship the general network support as fully crossplatform even if there's no non-linux network manager.

NewAllocRunner errors

I also have mixed opinions on adding another error return from NewAR. We were hoping to someday make NewAR/TR never return an error, but I have a feeling we're never going to get there. The current way client.go handles errors from NewAR seems fine, so it's probably not worth worrying about.

Adding a new AR/TR Validation method or hook might be useful? Or it might just complicate things more.

I think it's safe to accept your approach as-is and treat my ideas for out-of-scope of your networking efforts.

schmichael · 2019-05-14T18:43:30Z

client/allocrunner/alloc_runner_nonlinux.go

+
+// noop for non linux clients
+func (ar *allocRunner) initPlatformRunnerHooks(hookLogger hclog.Logger) ([]interfaces.RunnerHook, error) {
+	return nil, nil


Shouldn't we detect if they're trying to use an unsupported network mode and return an error?

schmichael · 2019-05-14T19:03:20Z

client/allocrunner/network_hook_linux.go

@@ -0,0 +1,243 @@
+// +build linux


Docker on Windows being able to run Linux images really complicates this. #2633

Not something we need to worry about right now, but it is interesting Windows introduces new complexity where a task's OS doesn't match the host OS!

schmichael · 2019-05-14T19:03:40Z

client/allocrunner/network_hook_linux.go

+
+	// manager is used when creating the network namespace. This defaults to
+	// bind mounting a network namespace descritor under /var/run/netns but
+	// can be created by a driver if nessicary


Suggested change

// can be created by a driver if nessicary

// can be created by a driver if necessary

schmichael · 2019-05-14T19:05:31Z

client/allocrunner/network_hook_linux.go

+	// alloc should only be read from
+	alloc *structs.Allocation
+
+	// spec described the network namespace and is syncronized by specLock


Suggested change

// spec described the network namespace and is syncronized by specLock

// spec described the network namespace and is syncronized by specLock

Suggested change

// spec described the network namespace and is syncronized by specLock

// spec described the network namespace and is synchronized by specLock

schmichael · 2019-05-14T19:05:43Z

client/allocrunner/network_hook_linux.go

+	// spec described the network namespace and is syncronized by specLock
+	spec     *drivers.NetworkIsolationSpec
+	specLock sync.Mutex
+	logger   hclog.Logger


Suggested change

logger hclog.Logger

logger hclog.Logger

schmichael · 2019-05-14T19:48:59Z

plugins/drivers/driver.go

+	NetIsolationModeTask = NetIsolationMode("task")
+
+	// NetIsolationModeNone indicates that there is no network to isolate and is
+	// inteded to be used for tasks that the client manages remotely


Suggested change

// inteded to be used for tasks that the client manages remotely

// intended to be used for tasks that the client manages remotely

Although maybe just remove that part as (a) it doesn't exist yet and (b) none may be useful for other things like system batch jobs that also don't exist yet but can be faked.

schmichael · 2019-05-14T19:49:40Z

plugins/drivers/proto/driver.proto

@@ -280,6 +288,27 @@ message ExecTaskResponse {
    ExitResult result = 3;
 }

+message CreateNetworkRequest {
+
+    // AllodID of the allocation the network is associated with


Suggested change

// AllodID of the allocation the network is associated with

// AllocID of the allocation the network is associated with

schmichael · 2019-05-14T19:49:51Z

plugins/drivers/proto/driver.proto

+
+message DestroyNetworkRequest {
+
+    // AllodID of the allocation the network is associated with


Suggested change

// AllodID of the allocation the network is associated with

// AllocID of the allocation the network is associated with

schmichael · 2019-05-14T19:50:28Z

plugins/drivers/proto/driver.proto

@@ -350,6 +397,10 @@ message TaskConfig {

    // AllocId is the ID of the associated allocation
    string alloc_id = 15;
+
+    // NetworkIsolationSpec specifies the configuration for the network namespace
+    // to use for the task. *Only supported on Linux


Suggested change

// to use for the task. *Only supported on Linux

// to use for the task. Only supported on Linux

...although that may not be true if you rearrange the hook code above? Couldn't somebody make a Windows driver that provided Create/Destroy Network?

schmichael · 2019-05-14T20:05:39Z

scheduler/util.go

@@ -414,6 +406,26 @@ func tasksUpdated(jobA, jobB *structs.Job, taskGroup string) bool {
 	return false
 }

+func networkUpdated(netA, netB []*structs.NetworkResource) bool {


Huh, I wonder why this doesn't use https://godoc.org/github.com/hashicorp/nomad/nomad/structs#Networks.Equals

github-actions · 2023-02-10T02:18:15Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

nickethier force-pushed the f-network-lifecycle branch from feb9dff to 82333ac Compare May 3, 2019 18:14

nickethier marked this pull request as ready for review May 8, 2019 15:26

nickethier requested review from schmichael and notnoop May 8, 2019 17:01

nickethier changed the base branch from f-network-group-stanza to f-network-namespaces May 8, 2019 17:46

nickethier added 7 commits May 8, 2019 21:32

Add network lifecycle management

6c8ba5c

Adds a new Prerun and Postrun hooks to manage set up of network namespaces on linux. Work still needs to be done to make the code platform agnostic and support Docker style network initalization.

ar: initial driver based network management

427fa7c

plugin/driver: fix tests and add new dep to vendor

5a67290

ar: move linux specific code to it's own file and add tests

70d0107

ar: cleanup lint errors

1dc0c4c

ar: add tests for network hook

89f6d92

ar: plumb error handling into alloc runner hook initialization

ddf125f

nickethier force-pushed the f-network-lifecycle branch from 3944cf5 to ddf125f Compare May 9, 2019 01:33

notnoop reviewed May 13, 2019

View reviewed changes

nickethier added 2 commits May 13, 2019 21:26

plugins/driver: make DriverNetworkManager interface optional

937123b

ar: fix test that failed due to error renaming

0a57dc8

nickethier merged commit 0587751 into f-network-namespaces May 14, 2019

schmichael reviewed May 14, 2019

View reviewed changes

nickethier mentioned this pull request May 21, 2019

ar: rearrange network hook to support building on windows #5735

Merged

nickethier mentioned this pull request Jul 31, 2019

Merge network namespaces work into master #6046

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add network lifecycle management #5626

Add network lifecycle management #5626

nickethier commented Apr 29, 2019 •

edited

Loading

notnoop left a comment

notnoop May 13, 2019

nickethier May 14, 2019

notnoop May 14, 2019

schmichael May 14, 2019

schmichael left a comment

schmichael May 14, 2019

schmichael May 14, 2019

schmichael May 14, 2019

schmichael May 14, 2019

schmichael May 14, 2019

schmichael May 14, 2019

schmichael May 14, 2019

schmichael May 14, 2019

schmichael May 14, 2019

schmichael May 14, 2019

github-actions bot commented Feb 10, 2023

	// can be created by a driver if nessicary
	// can be created by a driver if necessary

	// spec described the network namespace and is syncronized by specLock
	// spec described the network namespace and is syncronized by specLock

	// inteded to be used for tasks that the client manages remotely
	// intended to be used for tasks that the client manages remotely

	// AllodID of the allocation the network is associated with
	// AllocID of the allocation the network is associated with


		message DestroyNetworkRequest {

		// AllodID of the allocation the network is associated with

	// to use for the task. *Only supported on Linux
	// to use for the task. Only supported on Linux

Add network lifecycle management #5626

Add network lifecycle management #5626

Conversation

nickethier commented Apr 29, 2019 • edited Loading

Alloc Runner Hooks

plugins/drivers Changes

notnoop 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

schmichael left a comment

Choose a reason for hiding this comment

NewAllocRunner errors

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

Choose a reason for hiding this comment

github-actions bot commented Feb 10, 2023

nickethier commented Apr 29, 2019 •

edited

Loading

`plugins/drivers` Changes