Add a driver for rkt #165

achanda · 2015-09-29T22:35:24Z

Adds a driver for rkt
Adds associated tests

ryanuber · 2015-09-29T22:45:38Z

Awesome! Thanks @achanda. Would it be possible to use the native Go rkt interface here? I'm not familiar with it myself but it thinking it will be more resilient than using the CLI.

achanda · 2015-09-29T22:48:33Z

@ryanuber ah, I was looking for something like that. Thanks for the pointer. Let me look around.

catsby · 2015-09-29T22:51:49Z

client/driver/rkt.go

+        }
+        d.logger.Printf("[DEBUG] Started ACI: %q", name)
+        h := &rktHandle{
+                proc:   cmd.Process,


Should this be acmd? It's using the cmd from line 92 above, where we invoke rkt trust

Thanks, fixed.

catsby · 2015-09-29T22:56:44Z

Thanks for the contribution! Let us know what you find out about the rkt interface Ryan mentioned

achanda · 2015-09-30T06:21:18Z

@ryanuber @catsby I had a chat with the rkt guys. While they are working on a way to provide read only info (image etc.), there are no plans to drive rkt through an API. What makes that more difficult is that rkt does not run a daemon in the background. Kubernetes uses rkt in a similar way as this PR. I took a look at their code and noticed that they are spawning the binary in a similar way. However, their rkt package is too tightly integrated with kubernetes.

ryanuber · 2015-10-01T18:27:25Z

client/driver/rkt.go

+
+        node.Attributes["driver.rkt"] = "1"
+        node.Attributes["driver.rkt.version"] = rktMatches[0]
+        node.Attributes["driver.appc.version"] = appcMatches[1]


This should probably still be nested under the driver.rkt namespace, since appc itself isn't the driver.

ryanuber · 2015-10-01T18:42:17Z

@achanda thanks for looking into the client stuff! Understood, and I think this approach is fine if that's the case. I left a few minor comments but overall looking really good! Can you also add some driver documentation in the website/ directory to detail the config options, limitations, etc?

achanda · 2015-10-01T19:14:58Z

@ryanuber all done.

ryanuber · 2015-10-01T19:36:33Z

client/driver/rkt.go

+                return false, fmt.Errorf("Unable to parse Rkt version string: %#v", rktMatches)
+        }
+
+        node.Attributes["driver.rkt"] = "1"


Missed this on the first pass, we set this to true in the Docker driver here. I think we should keep these uniform.

ryanuber · 2015-10-01T19:38:48Z

Thanks! I took a final pass and just noted a few more (super) minor things. Thanks again, looks like a merge is coming soon!

dadgar · 2015-10-01T19:42:36Z

client/driver/rkt.go

+        if !ok || trust_prefix == "" {
+                return nil, fmt.Errorf("Missing trust prefix for rkt")
+        }
+        trust_args := []string{


I would remove this an inline it into the exec.Command.

dadgar · 2015-10-01T20:38:49Z

client/driver/rkt.go

+        }
+        data, err := json.Marshal(pid)
+        if err != nil {
+                log.Printf("[ERR] failed to marshal rkt PID to JSON: %s", err)


You will want to pass the logger from the Driver into the Handle. We support redirecting logs to files or other sources so this would break that it interface by using the default logger.

Make it skip if rkt is not installed

Run blocks

achanda · 2015-10-06T22:57:44Z

@ryanuber I changed to Start and rebased on master. These tests are passing on my box now. Please check.

dadgar · 2015-10-07T18:09:19Z

client/driver/rkt.go

+        cmd.Stdout = &outBuf
+        cmd.Stderr = &errBuf
+        d.logger.Printf("[DEBUG] driver.rkt: starting rkt command: %q", cmd.Args)
+        if err := cmd.Start(); err != nil {


This one should still be Run because you don't want to do rkt run before trust has finished

The trust needs to be added before anything can progress

dadgar · 2015-10-07T20:50:17Z

client/driver/rkt_test.go

+                Name: "etcd",
+                Config: map[string]string{
+                        "trust_prefix": "coreos.com/etcd",
+                        "name":  "coreos.com/etcd:v2.0.4",


You will want to run something that exits to test the wait. This test just times out right now

dadgar · 2015-10-07T21:01:09Z

Can you also run gofmt on all of the files. They are off a little. You can do this by running make format

This overrides the default exec command in the ACI

Add a driver for rkt

ryanuber · 2015-10-07T23:55:02Z

@achanda thanks for working through this with us! We are amped to have rkt supported in Nomad.

achanda · 2015-10-08T00:07:15Z

Thanks @ryanuber and all other reviewers :)

dadgar · 2015-11-13T00:07:27Z

@achanda: This was missed in the original review but the driver does not do resource enforcement which is part of the driver API. This poses a problem because Nomad can make no guarantee about the cpu/memory usage of the task and which ports it is using. Given your experience with rkt do you have any thoughts on improving this driver. Otherwise we may have to remove it until it can be implemented in a fashion that respects the resource constraints.

achanda · 2015-11-13T00:17:25Z

@dadgar CPU, memory and network isolation can be definitely be achieved using rkt. I will start working on those.

dadgar · 2015-11-13T00:23:27Z

@achanda: Awesome look forward to it. This was caught as we were reviewing for the Nomad 0.2 release. So to make the cut, this would need to be done by Monday, otherwise we could just disable for 0.2 and enable it in a latter version. Sorry for the short notice.

achanda · 2015-11-13T00:24:33Z

@dadgar ok, let me see what I can do.

achanda · 2015-11-13T23:17:45Z

@dadgar here is what I found, since the rkt CLI does not accept CPU and memory restrictions directly, the easiest way to achieve this will be using systemd-run, which will add another dependency. I am not familiar with the --pod-manifest flag, that can work but will need more effort than I can manage by Monday. Another way of doing this will be to add these options to the rkt CLI itself, but that won't work in a few days. Please let me know if we want to run rkt via systemd, otherwise the only option might be to remove this driver for now.

dadgar · 2015-11-14T01:14:45Z

@achanda: The system-d run solution is not quite enough. That provides CPU/Memory isolation but it doesn't fix port allocations. Whatever is in the manifest is what will be used. That needs to be fixed as well. My opinion is these really should be adjustable during rkt run similar to docker.

So we discussed it and we are going to leave the driver in for 0.2 but update the documentation to mark it as experimental. This allows the driver to be improved as changes to the rkt ecosystem occur.

github-actions · 2023-05-03T02:10:02Z

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.

catsby reviewed Sep 29, 2015
View reviewed changes

catsby added type/enhancement driver labels Sep 29, 2015

achanda mentioned this pull request Sep 30, 2015

Examples of using golang bindings? rkt/rkt#1506

Closed

ryanuber reviewed Oct 1, 2015
View reviewed changes

dadgar reviewed Oct 1, 2015
View reviewed changes

ryanuber mentioned this pull request Oct 1, 2015

Add driver for rkt #142

Closed

dadgar reviewed Oct 1, 2015
View reviewed changes

Abhishek Chanda added 4 commits October 6, 2015 15:56

Register rkt as a built in driver

97c2172

Fix function call

bedee8c

Make it skip if rkt is not installed

Add prefix to all message logs for rkt driver

5c76624

Use Start to run commands

4b21673

Run blocks

dadgar reviewed Oct 7, 2015
View reviewed changes

Use Run for adding trust prefix

558333d

The trust needs to be added before anything can progress

dadgar reviewed Oct 7, 2015
View reviewed changes

Abhishek Chanda and others added 2 commits October 7, 2015 15:15

Add an optional exec parameter to task config

43a8a1a

This overrides the default exec command in the ACI

Run gofmt

9140796

dadgar added a commit that referenced this pull request Oct 7, 2015

Merge pull request #165 from achanda/rkt

7ab84c2

Add a driver for rkt

dadgar merged commit 7ab84c2 into hashicorp:master Oct 7, 2015

jonboulle mentioned this pull request Oct 7, 2015

docs: add information on nomad integration rkt/rkt#1570

Closed

achanda deleted the rkt branch October 8, 2015 00:14

achanda mentioned this pull request Nov 13, 2015

Memory and CPU restriction using rkt CLI rkt/rkt#1735

Closed

snyk-bot mentioned this pull request Oct 20, 2021

[Snyk] Upgrade next-mdx-remote from 3.0.1 to 3.0.4 admon-snyk/nomad#5

Open

This was referenced Feb 18, 2022

[Snyk] Fix for 2 vulnerabilities ekmixon/nomad#72

Open

[Snyk] Fix for 2 vulnerabilities admon-snyk/nomad#11

Open

github-actions bot locked as resolved and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a driver for rkt #165

Add a driver for rkt #165

achanda commented Sep 29, 2015

ryanuber commented Sep 29, 2015

achanda commented Sep 29, 2015

catsby Sep 29, 2015

achanda Sep 29, 2015

catsby commented Sep 29, 2015

achanda commented Sep 30, 2015

ryanuber Oct 1, 2015

ryanuber commented Oct 1, 2015

achanda commented Oct 1, 2015

ryanuber Oct 1, 2015

ryanuber commented Oct 1, 2015

dadgar Oct 1, 2015

dadgar Oct 1, 2015

achanda commented Oct 6, 2015

dadgar Oct 7, 2015

dadgar Oct 7, 2015

dadgar commented Oct 7, 2015

ryanuber commented Oct 7, 2015

achanda commented Oct 8, 2015

dadgar commented Nov 13, 2015

achanda commented Nov 13, 2015

dadgar commented Nov 13, 2015

achanda commented Nov 13, 2015

achanda commented Nov 13, 2015

dadgar commented Nov 14, 2015

github-actions bot commented May 3, 2023

Add a driver for rkt #165

Add a driver for rkt #165

Conversation

achanda commented Sep 29, 2015

ryanuber commented Sep 29, 2015

achanda commented Sep 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catsby commented Sep 29, 2015

achanda commented Sep 30, 2015

Choose a reason for hiding this comment

ryanuber commented Oct 1, 2015

achanda commented Oct 1, 2015

Choose a reason for hiding this comment

ryanuber commented Oct 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achanda commented Oct 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadgar commented Oct 7, 2015

ryanuber commented Oct 7, 2015

achanda commented Oct 8, 2015

dadgar commented Nov 13, 2015

achanda commented Nov 13, 2015

dadgar commented Nov 13, 2015

achanda commented Nov 13, 2015

achanda commented Nov 13, 2015

dadgar commented Nov 14, 2015

github-actions bot commented May 3, 2023