Enable rkt driver to use address_mode = 'driver' #3256

dalegaard · 2017-09-20T22:51:20Z

The rkt driver currently does not allow a user to specify address_mode = 'driver' in the service because it doesn't pass a DriverNetwork instance up.

This PR fixes that, by filling out a DriverNetwork instance with the first IP found on the container and generating a matching portmap.

To do this, the UUID of the rkt container must first be tracked correctly. This was done in a pretty racy way before, so here we've also refactored the rkt driver's Start method to instead use prepare followed by run-prepared.

BR.

The rkt driver currently executes run and asks that the pod UUID is written to a file that is then polled for changes for up to five seconds. Many container fetches will take longer than this, so this method will often not be able to track the pod UUID reliably. To avoid this problem, rkt allows pods to be first prepared, which will return their UUID, and then run as a second invocation. Here we convert the rkt driver's Start method to use this method instead. This way, the UUID will always be tracked correctly.

Currently the rkt driver does not expose a DriverNetwork instance after starting the container, which means that address_mode = 'driver' does not work. To get the container network information, we can call `rkt status` on the UUID of the container and grab the container IP from there. For the port map, we need to grab the pod manifest as it will tell us which ports the container exposes. We then cross-reference the configured port name with the container port names, and use that to create a correct port mapping. To avoid doing a (bad) reimplementation of the appc schema(which rkt uses for its manifest) and rkt apis, we pull those in as vendored dependencies. The versions used are the same ones that rkt use in their glide dependency configuration for version 1.28.0.

schmichael

Absolutely fantastic PR @dalegaard! Thanks! Just a few fairly minor comments.

Would you mind also adding a test that at least asserts DriverNetwork is non-nil? This might be an ok place for that: https://github.com/hashicorp/nomad/blob/master/client/driver/rkt_test.go#L483

Note to run the rkt tests locally you must have the NOMAD_TEST_RKT=1 env var set (or just let Travis do it).

You could also add the PR title as the changelog entry as part of this PR: https://github.com/hashicorp/nomad/blob/master/CHANGELOG.md#07-unreleased

core goes first, then alphabetical order for everything else (so driver/rkt: ... would go after discovery).

I can always add the changelog entry later if you'd like.

Thanks again!

schmichael · 2017-09-25T22:54:36Z

client/driver/rkt.go

+	var outBuf, errBuf bytes.Buffer
+	cmd := exec.Command(rktCmd, statusArgs...)
+	cmd.Stdout = &outBuf
+	cmd.Stderr = &errBuf


Use ioutil.Discard for stderr or include a fixed size snippet in Run's error case.

schmichael · 2017-09-25T22:54:44Z

client/driver/rkt.go

+	var outBuf, errBuf bytes.Buffer
+	cmd := exec.Command(rktCmd, statusArgs...)
+	cmd.Stdout = &outBuf
+	cmd.Stderr = &errBuf


Use ioutil.Discard for stderr or include a fixed size snippet in Run's error case.

schmichael · 2017-09-25T23:39:47Z

client/driver/rkt.go

+			break
+		}
+	}
+	if driverNetwork == nil {


In the Docker driver we return an error if driverConfig.PortMap is set but no networks could be found. I think the same behavior would be appropriate here.

https://github.com/hashicorp/nomad/blob/master/client/driver/docker.go#L1065-L1070

Since we don't actually show these errors anywhere, just discard them right away.

To test that the rkt driver correctly sets a DriverNetwork, at least when a port mapping is requested, we amend the TestRktDriver_PortsMapping test with a small check.

If the rkt driver cannot get the network status, for a task with a configured port mapping, it will now fail the Start() call and kill the task instead of simply logging. This matches the Docker behavior. If no port map is specified, the warnings will be logged but the task will be allowed to start.

dalegaard · 2017-09-26T08:59:21Z

@schmichael looks like the tests are failing now because of rkt/rkt#3699 affecting rkt < 1.27.0, and travis is using 1.18.0.

Which fallback would you prefer for this case? I'm guessing it needs to be handled somehow, because else rkt < 1.27.0 will no longer be usable with Nomad, even when the address_mode option is not set to 'driver'.

schmichael · 2017-09-26T16:18:03Z

@dalegaard We're pretty flexible about updating the version of rkt we use and 1.18 is coming up on being a year old which seems like about 100 years in the container world. Looks like Ubuntu 17.04 was the first LTS to include rkt, but it's only 1.21.

I'd be ok with bumping our minimum supported rkt version to 1.27 if it fixes a relevant bug. Mind adding that to this PR? Sorry to keep moving the goalpost. I think these are the relevant locations:

(Not sure why we don't install the min supported version in Travis right now... that seems like an oversight.)

dalegaard · 2017-09-26T16:48:08Z

@schmichael Sure, no problem - do you want me to add a note in the changelog under a backwards incompatibilities header while I'm at it?

schmichael · 2017-09-26T16:51:25Z

@dalegaard good thinking. You're better at this than me. ;)

The changes introduces in #3256 require at least rkt 1.27.0 because of a bug in the JSON output of `rkt status` in previous versions. Here we upgrade all references to rkt's minimum version, and also make travis and vagrant use this version when running tests. Finally we add a CHANGELOG notice.

The network status poll loop for the rkt drivers `Start` method was a bit messy, and could not display the last encountered error. Here we clean it up.

The network status poll loop will now report any networks it ignored, as well as a no-networks situations.

dalegaard · 2017-09-26T19:53:01Z

So this is proving a bit more elusive than expected, since the test cases pass fine on my local machine but not in Travis. This prompted me to add some more logging in there, so I guess it's a win overall :-) Sorry for abusing your Travis setup like this ;-)

The current Travis setup scripts copy in rkt, but do not set up a default container network. Here we copy the container network setup over from the vagrant setup scripts.

The rkt port mapping test currently starts redis with --version, which obviously makes redis exit again almost immediately. This means that the container exists before the network status can be queried, and so the test fails.

dalegaard · 2017-09-26T21:18:25Z

@schmichael right, turns out this was failing because the PortsMapping test runs redis with --version, making it return before the status has been queried, so a classic race condition.

This does bring up another case however: what to do if the task returns before its status could be queried succesfully? Should we check if the task has already returned, and then ignore the network errors? Is there even a non-racy way to do such a check?

I have a small single-line commit that uses the pluginClient.Exited() method, but I'm not sure how to test that in a non-racy way :-) The container has to reliably start, boot, run whatever program is in it, and exit, within 400 ms.

schmichael · 2017-09-26T21:38:44Z

@dalegaard Argh that is unfortunate, and I've definitely run into similar issues before. Perhaps you could move the post-run status checking code into a method, and if that method returns an error, check if the executor (started in a goroutine right about the linked line) has exited. If it's exited already, there's no need to return an error from Start() as the either the task will fail or it will be restarted and Start() will get called again.

If the task fails, then the fact that a network wasn't returned won't hurt anything.

If the task is restarted, Start() will get another chance to read the network. If the initial fast exit was due to a transient failure, everything may Just Work upon restart and the network will get read. Otherwise the task will eventually fail and the lack of a successful network read won't matter.

Sorry for the verbose explanation, but I'm trying to work through it myself. 😄 The summary is: if the executor/handle exits before you're able to read the network there's no need to worry about returning an error regarding the network. It would only mask the actual failure.

If the container dies before the network can be read, we now ignore the error coming out of the network information polling loop. Nomad will restart the task regardless, so we might be masking the actual error. The polling loop for the rkt network information, inside the `Start` method, was getting a bit unwieldy. It's been refactored out so it's not a seperate function.

dalegaard · 2017-09-26T22:36:55Z

@schmichael I pushed an updated version where I've refactored out the network information polling loop, per your suggestion. Looks much neater now :-)

The build failed with a "port collision" in an unrelated test case however -.-

schmichael · 2017-09-26T22:39:07Z

@dalegaard Updates look great! I kicked the build because you deserve to see all green checks. Will merge once Travis is happy. Thanks for sticking with this! That was a really tricky problem.

schmichael · 2017-09-26T23:04:34Z

Confirmed with a coworker that failure is unrelated and it's safe to merge as-is. Sorry no greenlight from Travis, but thanks for the hard work @dalegaard!

If you're looking for some simple rkt improvements, #3207 may be a one liner. Testing - as always - will be the tricky part (although moving that validation to a method and unit testing it is totally acceptable instead of a full rkt integration test).

dalegaard · 2017-09-26T23:06:56Z

@schmichael No worries, thanks for reviewing this!

github-actions · 2023-03-22T02:12:33Z

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.

dalegaard added 2 commits September 21, 2017 00:17

schmichael requested changes Sep 25, 2017

View reviewed changes

dalegaard added 4 commits September 26, 2017 09:05

Discard errors from rkt status and cat-manifest

1d75ff8

Since we don't actually show these errors anywhere, just discard them right away.

Test for rkt driver setting DriverNetwork

3dd16a2

To test that the rkt driver correctly sets a DriverNetwork, at least when a port mapping is requested, we amend the TestRktDriver_PortsMapping test with a small check.

Update CHANGELOG.md

f6ce83c

dalegaard added 4 commits September 26, 2017 19:15

Small logging fix in rkt/driver

a7f5f8a

Refactor rkt network status loop

eade50b

The network status poll loop for the rkt drivers `Start` method was a bit messy, and could not display the last encountered error. Here we clean it up.

Improve rkt driver network status poll loop

b537f16

The network status poll loop will now report any networks it ignored, as well as a no-networks situations.

dalegaard added 2 commits September 26, 2017 22:20

Add rkt default network to Travis

5cd1d19

The current Travis setup scripts copy in rkt, but do not set up a default container network. Here we copy the container network setup over from the vagrant setup scripts.

Make rkt port mapping test not exit immediately

c359ba4

The rkt port mapping test currently starts redis with --version, which obviously makes redis exit again almost immediately. This means that the container exists before the network status can be queried, and so the test fails.

schmichael approved these changes Sep 26, 2017

View reviewed changes

dalegaard mentioned this pull request Sep 26, 2017

[feature] Expose contents of DriverNetwork via API #3285

Closed

schmichael merged commit eeeb544 into hashicorp:master Sep 26, 2017

schmichael mentioned this pull request Sep 27, 2017

Only build rkt driver on linux #3295

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable rkt driver to use address_mode = 'driver' #3256

Enable rkt driver to use address_mode = 'driver' #3256

dalegaard commented Sep 20, 2017

schmichael left a comment

schmichael Sep 25, 2017

schmichael Sep 25, 2017

schmichael Sep 25, 2017

dalegaard commented Sep 26, 2017

schmichael commented Sep 26, 2017

dalegaard commented Sep 26, 2017

schmichael commented Sep 26, 2017

dalegaard commented Sep 26, 2017

dalegaard commented Sep 26, 2017 •

edited

Loading

schmichael commented Sep 26, 2017

dalegaard commented Sep 26, 2017

schmichael commented Sep 26, 2017

schmichael commented Sep 26, 2017

dalegaard commented Sep 26, 2017

github-actions bot commented Mar 22, 2023

Enable rkt driver to use address_mode = 'driver' #3256

Enable rkt driver to use address_mode = 'driver' #3256

Conversation

dalegaard commented Sep 20, 2017

schmichael left a comment

Choose a reason for hiding this comment

schmichael Sep 25, 2017

Choose a reason for hiding this comment

schmichael Sep 25, 2017

Choose a reason for hiding this comment

schmichael Sep 25, 2017

Choose a reason for hiding this comment

dalegaard commented Sep 26, 2017

schmichael commented Sep 26, 2017

dalegaard commented Sep 26, 2017

schmichael commented Sep 26, 2017

dalegaard commented Sep 26, 2017

dalegaard commented Sep 26, 2017 • edited Loading

schmichael commented Sep 26, 2017

dalegaard commented Sep 26, 2017

schmichael commented Sep 26, 2017

schmichael commented Sep 26, 2017

dalegaard commented Sep 26, 2017

github-actions bot commented Mar 22, 2023

dalegaard commented Sep 26, 2017 •

edited

Loading