Initial support for containers #71

lukehoban · 2017-09-30T05:22:49Z

Implements the Service and Task models of container-based compute, the networking and Volume abstractions needed for these, and the image and function options for specifying Docker containers as described in #6 (comment).

This is still a stepping stone on the way to full container support in our framework, but this is a natural point to stop and merge in the first wave of support.

Work included here:

Support for Service
Move to NLB
Support scaling services
Support volumes
Support for notion of Task which is long running job that can be spawned dynamically and will finish
Support for closure serialization into Docker image transparently

Work still to be done:

Support for doing Docker builds as part of provisioning (build vs. image) (tracked in Support a build option for Docker containers #87)
Solve for two services defined by external images discovering one another (tracked in Service-to-service network discovery #89)
Potentially allow using Task as a function (tracked in Tasks as functions #88)

Fixes #6.

lukehoban · 2017-09-30T05:24:45Z

BTW - It's fun to see this working - here's the example:

let nginx = new cloud.Service("nginx", {
    name: "nginx",
    image: "nginx",
    memory: 128,
    portMappings: [{containerPort: 80}],
});

let api = new cloud.HttpEndpoint("myendpoint");
api.get("/", async (req, res) => {
    try {
        console.log("timer starting")
        let hostandport = await nginx.getHostAndPort(0, 80);
        console.log("got host and port:" + hostandport);
        let resp = await fetch(`http://${hostandport}/`);
        let buffer = await resp.buffer();
        console.log(buffer.toString());
        res.status(resp.status);
        for (let header of Object.keys(resp.headers)) {
            res.setHeader(header, resp.headers.get(header));
        }
        res.setHeader("X-Forwarded-By", "my-pulumi-proxy");
        res.end(buffer);
    } catch(err) {
        console.error(err);
        res.status(500).end(`Pulumi proxy service error: ${err}`);
    }
});
api.publish().then(url => console.log(`Serving at: ${url}`));

and here's the result:

$ curl -v https://ob4ionfkp5.execute-api.us-east-1.amazonaws.com/stage/
*   Trying 13.33.151.18...
* TCP_NODELAY set
* Connected to ob4ionfkp5.execute-api.us-east-1.amazonaws.com (13.33.151.18) port 443 (#0)
* TLS 1.2 connection using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
* Server certificate: *.execute-api.us-east-1.amazonaws.com
* Server certificate: Symantec Class 3 Secure Server CA - G4
* Server certificate: VeriSign Class 3 Public Primary Certification Authority - G5
> GET /stage/ HTTP/1.1
> Host: ob4ionfkp5.execute-api.us-east-1.amazonaws.com
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Type: application/json
< Content-Length: 612
< Connection: keep-alive
< Date: Sat, 30 Sep 2017 05:23:57 GMT
< x-amzn-RequestId: 8e6423ee-a59f-11e7-b7c0-9f1990c19397
< _headers:
< X-Forwarded-By: my-pulumi-proxy
< X-Amzn-Trace-Id: sampled=0;root=1-59cf2a6c-f86882c8bf142d8f3b066535
< X-Cache: Miss from cloudfront
< Via: 1.1 f42a8d19b16850af801ce5662fc9fdab.cloudfront.net (CloudFront)
< X-Amz-Cf-Id: rj-2_QqT5AMtv9ip3Vy9r7N2XnfYjWhqR4gKM1ZxI84T-7AMEGWJ-g==
<
<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
<style>
    body {
        width: 35em;
        margin: 0 auto;
        font-family: Tahoma, Verdana, Arial, sans-serif;
    }
</style>
</head>
<body>
<h1>Welcome to nginx!</h1>
<p>If you see this page, the nginx web server is successfully installed and
working. Further configuration is required.</p>

<p>For online documentation and support please refer to
<a href="http://nginx.org/">nginx.org</a>.<br/>
Commercial support is available at
<a href="http://nginx.com/">nginx.com</a>.</p>

<p><em>Thank you for using nginx.</em></p>
</body>
</html>
* Connection #0 to host ob4ionfkp5.execute-api.us-east-1.amazonaws.com left intact

This implements the first small step on supporting containers. This first example exposes a @pulumi/cloud notion of Service - a container(s) which will run permanently for the lifetime of the program, and which may expose some ports whcih can be accessed from other compute (aka functions). To use Service, you must configure the `cloud-aws:config:ecsClusterARN` variable to point to a valid ECS cluster in the region you are deploying into. This is effectively treating that as part of the infrastructure that an execution environment must make available. The Service also exposes a runtime API to provide it's host/port pairs for each exposed port on the container(s). This is currently handled by querying the ECS data plance for info about the single instance of the service. We expect to replace this with an NLB to ensure this service discovery is more efficient (and more robust/managed). Notably, the example shows how a lambda can make requests against a running service. All APIs defined so far are provisional - we'll flesh these out and simplify+enrichen as needed based on the next round of examples. We have lots more to do for containers - including: * [ ] Move to NLB and support scaling services * [ ] Solve for two services defined by external images discovering one another * [ ] Support for doing Docker builds as part of provisioning (`build` vs. `image`) * [ ] Support for closure serialization into Docker image transparently * [ ] Support for notion of Task which is long running job that can be spawned dynamically and will finish * [ ] Potentially allow using Task as a function

All exposed container/port pairs are allocated an NLB listener. NLBs are added as needed for every 50 exposed container/port pairs.

Adds a new `scale` parameter to allow scaling a service up to multiple containers. All load balancers for exposed ports will scale out across the multiple container instances. Also adjusts the API surface area to make room for this configuration. Expect to add back convenience shortcuts for common simple use cases (single instance of single container) in the future.

Also initial scaffolding for container-mountable FileSystem.

joeduffy · 2017-10-04T14:49:14Z

aws/service.ts

+
+    getHostAndPort: (containerName: string, containerPort: number) => Promise<string>;
+
+    constructor(name: string, args: cloud.ServiceArguments) {


Note, you may want to expose dependsOn: Service[] here. This is presumably like Docker Compose's depends_on attribute, when you need to control service start order.

That said, this may not be sufficient. I have no idea what this means for rolling updates: e.g., if container B depends on container A, and container A is updated, does that imply a "replacement" for container B? I don't think Docker Compose does anything like this, but it's worth looking into. Hmmmmm...

Yeah - there's going to be a bunch of issues like this - and in particular, I'm still unsure on how we'll tie together Service updates with the Pulumi update lifecycle. One of the most concerning things I've hit so far is that Terraform doesn't wait for the Service update to be healthy before returning from the Create/Update operation (this is different than CloudFormation - which does block on this - sometimes for a long time!). That means that we won't have much control on how Service updates get coordinates, since even when we see dependencies at the Pulumi level, we can't force them to resolve carefully only once dependents are active. We will need to explore this more once we have enough working to validate some common workflows.

Does ECS actually respect the Docker HEALTHCHECK instruction? I would actually guarantee that we respect this to end users. This may increase the cost of implementing on some targets -- e.g., I think Kubernetes still doesn't understand HEALTHCHECK and instead you still need to do manual liveness probes -- but in keeping with our theme of making things radically simpler, it seems important. In fact, I would even think about how to incorporate live status checks into the container's resource page in the Pulumi Cloud Management Console (/cc: @chrsmith).

Looks like ECS does not yet - but there's a lot of demand: aws/amazon-ecs-agent#534

This is somewhat orthogonal (though also concerning) to the issue I raised above about Terraform not blocking on services passing healthchecks before treating the Create/Update as succeeding. The Terraform issue will probably block us doing anything smart at the next layer as well. I still need to think more about what the path forward should look like here.

joeduffy · 2017-10-04T14:49:43Z

aws/service.ts

+            throw new Error("Cannot create 'Service'.  Missing cluster config 'cloud-aws:config:ecsClusterARN'");
+        }
+        let containers = args.containers;
+        let scale = args.scale === undefined ? 1 : args.scale;


In general I'm not doing a deep review until this is ready, but I just happened to notice this when filing the above comment: Should we validate that scale >= 1, if specified?

We aren't really doing any validation at all of arguments - just leaving it to Terraform/AWS - we could try to pull as much validation as possible up into the @pulumi/cloud level here and in all the other libraries - for better error messages - though there will be a ton of cases that we can't catch early so we'll have to allow for errors bubbling up from the underlying platform in any case. Opened #76.

For these sorts of things, where we know we have a deficit, I'd just start getting in the habit of adding the checks where they are "obvious" and we can use #76 to come back and do a thorough sweep later. But it'd definitely be good to switch our brains over to sprinkling in checks where they make sense as we write the code.

CyrusNajmabadi · 2017-10-04T18:06:29Z

NLB
?

CyrusNajmabadi · 2017-10-04T18:08:54Z

api.publish().then(url => console.log(`Serving at: ${url}`));

Aside: can 'await' be used at the top level of a module? Seems like it would be nice so you could have async module creation and importation.

CyrusNajmabadi · 2017-10-04T18:09:31Z

api/service.ts

@@ -0,0 +1,77 @@
+// Copyright 2016-2017, Pulumi Corporation.  All rights reserved.
+
+export interface Containers {


Is this just a registry where you can find the containers you've created?

No - this is a description of the Containers that are part of a Service (or in the future, a Task).

Probably worth a JSDoc, since people might wonder about this.

CyrusNajmabadi · 2017-10-04T18:09:52Z

api/service.ts

+    /**
+     * The image to use for the container.
+     */
+    image: string;


How does lookup of this work? i.e. is it a file path? if so, how is it resolved?

It's a Docker image name - see https://docs.docker.com/engine/reference/commandline/tag/. It gets resolved by the docker pull equivalent used by the orchestration engine.

CyrusNajmabadi · 2017-10-04T18:10:41Z

api/service.ts

+     * An array of port mappings, indicating the container port to expose and
+     * the protocal that is used on that port.
+     */
+    portMappings?: {containerPort: number; protocol?: string}[];


why is the protocol needed?

Docker in general, and DockerCompose/ECS in particular, support either TCP or UDP traffic, and require that this be communicated as part of the port mapping (presumably their network layers need to know).

Unfortunately, NLB only supports TCP - so our current model can't support UDP usefully - so we may as well remove this property for now.

Can we support it in cases where you don't need service scale-out (instances == 1)?

Can we support it in cases where you don't need service scale-out (instances == 1)?

Not really - we rely on NLB to expose a stable public hostname for the service (in case a cluster host dies and ECS brings up a new one for the service).

I notice protocol is optional (?), so I assume it defaults to tcp?

As I read calling code, containerPort seems a little verbose. Also, I assume we (eventually) want the option of exposing the port as something different than the container's definition. So, how about:

ports?: { port: number, expose?: number, protocol?: string }[];

where port is the container port and expose is an optional override for what to expose it as (defaulting to port if undefined).

I notice protocol is optional (?), so I assume it defaults to tcp?

Actually, as noted above - we can only support TCP right now - so I'm just going to remove protocol entirely and we can add it back if/when we have a platform that can support it.

As I read calling code, containerPort seems a little verbose. Also, I assume we (eventually) want the option of exposing the port as something different than the container's definition. So, how about:
ports?: { port: number, expose?: number, protocol?: string }[];
where port is the container port and expose is an optional override for what to expose it as (defaulting to port if undefined).

Unless we expose the ability to manage a cluster directly, we won't offer the ability to control the port that gets exposed - that's fundamentally tied to controlling the hosts and placement on hosts.

I do think that we can use port instead of containerPort though.

CyrusNajmabadi · 2017-10-04T18:11:33Z

api/service.ts

+     */
+    portMappings?: {containerPort: number; protocol?: string}[];
+    /**
+     * The command line that is passed to the container. This parameter maps to


it's a 'command line' (indicating singular), but it takes an array of string (indicating plural). Maybe "list containing the command to execute, and all the parameters to provide to it"?

Docker is pretty confusing around this - but tools have generally standardized on the array model. See https://docs.docker.com/engine/reference/builder/#cmd.

Docker Compose allows either a string or array here, and we could do similar - though they under-specify in their docs which of the three CMD variants in Docker this maps to. I'm sort of okay staying with one option to keep it clear what the semantics are - at least initially.

CyrusNajmabadi · 2017-10-04T18:11:53Z

api/service.ts

+     * Docker `CMD` parameter, go to
+     * https://docs.docker.com/engine/reference/builder/#cmd.
+     */
+    command?: string[];


Is optional. What does it mean to not provide this?

Inferred from the Dockerfile. That's probably as common or more than specificying it at Run time.

BTW - suggest reading these resources on Docker generally to understand the various concepts here:

Dockerfile file format - https://docs.docker.com/engine/reference/builder

docker run command parameters - https://docs.docker.com/engine/reference/run/

Docker Compose file format - https://docs.docker.com/compose/compose-file/

Each is a layer of abstraction over the one before it. We are effectively designing something at the level of the Compose abstraction here (similar to the Kubernetes pod spec and ECS task definition - which also spec things at this same level but all using subtly different formats tuned to their execution environment).

CyrusNajmabadi · 2017-10-04T18:12:32Z

api/service.ts

+
+export interface ServiceArguments {
+    containers: Containers;
+    scale?: number;


what does it mean to not provide? No scaling? Auto scaling?

Default is 1. Will add docs on these.

CyrusNajmabadi · 2017-10-04T18:13:09Z

api/service.ts

+    // Inside API
+
+    /**
+     * The exposed host and port for connecting to the given containerIndex on


"containerIndex" vs "containerName"

CyrusNajmabadi · 2017-10-04T18:13:53Z

aws/config/index.ts

+
+import * as pulumi from "pulumi";
+
+let _config = new pulumi.Config("cloud-aws:config");


why the _ usage?

Not sure - I followed @joeduffy's pattern for this from one of the other libraries. I'll change it here - it's inconsistent in this codebase.

Yeah, I suck, sorry guys. I used _config in the Terraform auto-generator to avoid conflicting with any config variables whose key names were actually config (how likely is that?!) and then got into the bad habit of propagating the pattern.

CyrusNajmabadi · 2017-10-04T18:15:00Z

aws/service.ts

+    "MAC_ADMIN" | "MAC_OVERRIDE" | "MKNOD" | "NET_ADMIN" | "NET_BIND_SERVICE" | "NET_BROADCAST" | "NET_RAW" |
+    "SETFCAP" | "SETGID" | "SETPCAP" | "SETUID" | "SYS_ADMIN" | "SYS_BOOT" | "SYS_CHROOT" | "SYS_MODULE" |
+    "SYS_NICE" | "SYS_PACCT" | "SYS_PTRACE" | "SYS_RAWIO" | "SYS_RESOURCE" | "SYS_TIME" | "SYS_TTY_CONFIG" |
+    "SYSLOG" | "WAKE_ALARM";


should this list be reviewed at some point?

No - these are just copy/pasted from ECS docs. I'll add links in comments on these. We should move all of this into pulumi-aws as overlays as part of pulumi/pulumi-aws#38.

CyrusNajmabadi · 2017-10-04T18:16:44Z

aws/service.ts

+            internal: false,
+        });
+    }
+    if (!ecsClusterVpcId) {


consider moving these checks to teh start of the function before any work is done.

CyrusNajmabadi · 2017-10-04T18:32:29Z

aws/service.ts

+                    `No exposed port for ${containerName} port ${port}`,
+                );
+            }
+            return `${info.host.dnsName}:${info.port}`;


consider returning a richer object than just the concated string of the host and port. We might also consider using that same richer object for the url property of the HttpEndpoint.

Agreed - we'll have to do this in general - many other APIs expect these to be available separately. I'll define a new interface for this and return it.

For HttpEndpoint I'm less sure - since it's actually a URI, which can be parsed if needed but is most common to pass in string form. Here though its not a valid URI, it's just DNS host name and port components of a URI which generally need to be composed into other APIs in more interesting ways.

CyrusNajmabadi · 2017-10-04T18:35:05Z

examples/containers/index.ts

+                },
+            },
+        });
+        this.get = (key: string) => {


why not make async? then you can just 'await' instead of needing the .then

Indeed - unfortunately node_redis doesn't natively support promises.

redis/node-redis#864

Assuming that will get addressed soon - as promises are now more or less assumed requirement for a decent node module.

CyrusNajmabadi · 2017-10-04T18:37:52Z

examples/containers/index.ts

+                let client = require("redis").createClient(`redis://${hostandport}`, {password: redisPassword});
+                console.log(client);
+                return new Promise<string>((resolve, reject) => {
+                    client.get(key, (err: any, v: any) => {


from: https://github.com/NodeRedis/node_redis

You can use bluebird to asyncify redis. Then you could just do 'await client.getAsync(key)'

Yeah - but then even more dependencies - hoping that redis/node-redis#864 gets addressed. Or that Lambda introduces Node 8 support so we get util.promisify built in (and async/await built-in!).

CyrusNajmabadi · 2017-10-04T18:38:36Z

examples/containers/index.ts

+            res.end(page);
+            return;
+        }
+        let hostandport = await nginx.getHostAndPort("nginx", 80);


why is a number passed into getHostAndPort?

The service exposes some set of ports from each of it's set of containers. You need to ask for which container and which port on that container you want to reference. This will give you back the NLB dns name and port associated with that.

I'm definitely interested in ways to simplify all of this - but we have to support the generality of the defintiion pattern here to meet existing Docker use cases.

FWIW - The thing that bother me more than the number is that the "nginx" has to be passed as a string instead of being a strongly typed lookup on the service object.

One thing I wonder is whether we should make it easy to get the "default" endpoint:

let addr = await nginx.getDefaultAddress();

I realize Docker doesn't have such a concept but in 90%+ of the cases, there's just one port exposed, and having to sprinkle in the port at the callsite is a little unfortunate. Maybe something we should consider adding to the service definition itself, since we have all the metadata it seems like something we should be able to do pretty easily?

Note - I did make this change to make both parameters to getDefaultAddress optional and to default to the "first" container and port. This is a little brittle - but does make code a lot simpler for the common case.

CyrusNajmabadi · 2017-10-04T18:39:04Z

mock/service.ts

+    name: string;
+    getHostAndPort: (containerName: string, containerPort: number) => Promise<string>;
+    constructor(name: string, args: cloud.ServiceArguments) {
+        throw new Error(`Service not yet supported in mock implementation.`);


I should have left the TODO: @CyrusNajmabadi in there :-).

lukehoban · 2017-10-04T18:49:52Z

NLB
?

Network Load Balancer - https://aws.amazon.com/blogs/aws/new-network-load-balancer-effortless-scaling-to-millions-of-requests-per-second/

lukehoban · 2017-10-04T18:53:34Z

Aside: can 'await' be used at the top level of a module? Seems like it would be nice so you could have async module creation and importation.

Unfortunately no. This is even more painful in cases like https://github.com/pulumi/pulumi-aws/blob/1268d2d915628bcc8fa2461d3b54a1894407639e/examples/webserver/variants/zones/index.ts#L15. I continue to really hope we can find away to get rid of the Promise gunk in the programming model here. (Sort of tracked with pulumi/pulumi#331)

Also addresses some PR feedback.

Tasks provide a handle to as Container which can be used to `run` an instance of the container at runtime.

Do some promises refactoring to prepare for computing the container image from async operations - such as closure serialization.

Allows a `function` to be specified in place of an `image` in a container specification. Uses closure serialization to turn this into JavaScript which can be loaded into a fixed Docker runner image. Includes the source for the `javascriptrunner` Docker image which is published as `lukehoban/javascriptrunner`. We will need to publish this as an official Pulumi image. Also doesn't yet pass the full local folder zip into the container - which is necessary for correct closure serialization semantics. But many/most simple examples will already work.

Use `scale: 2` and add code so that it is visible which instance is being used.

lukehoban · 2017-10-09T18:27:23Z

avoid the Dockerfile altogether and just let you point us at the root directory of a Node application

I'm inclined to focus on the other three first. This is a very slight delta from build, and given that we will also have function on the other side of that in terms of truly simple options, I'm not immediately sure this fourth variant will pay for itself. It's also a lot of policy we'll have to define and manage per-language we want to support, and feels a bit like reinventing automatic-Procfile inference which many folks have tried and tends to get messy. I'm inclined to just point folks to recommended two line Dockerfiles for common cases to start and focus on making that experience and the inline function experience really strong.

we'd use a technique very much like javascriptrunner, except that it copies the full program directory and not just index.js

I haven't implemented it yet, but per the comments in the code the goal (and requirement) is that javascriptrunner will do this for inline functions as well.

runtime images that we use for executing Pulumi programs

I don't understand that goal - seems these are very different - effectively build vs. runtime environments. The build environment for examples needs all the @pulumi libraries, the runtime one does not. That said, Node.js versions, etc. should be the same - but more to the goal of aligning with the Lambda/Function environment.

As an aside, given that there are four different mutually exclusive properties, I wonder if we ought to consider different classes. I realize the bag of options is more idiomatic JavaScript, but maybe we should consider factory functions or something? Just trying to ensure the pit of success and all that jazz ...

I've thought about that - but I don't have a concrete proposal that feels better. The end result of all of these is the same kind of thing, and the bag of options is almost entirely the same regardless of which of these options you use for the container implementation. And this is for the container spec, which can be embedded inside multiple containers (potentially of different types) used in a Service.

I feel reasonably good about the pit of success angle here - one constructor, one bag of options, and simple to document that mutually exclusive properties.

My inclination is to lean on aggressive overloading as a way to make some of the simple cases here shorter.

Open to other suggestions if anyone has a full proposal for something they feel is net better.

lukehoban · 2017-10-09T20:46:16Z

@joeduffy @CyrusNajmabadi This PR is now ready for a more thorough review. What's available now is in a solid working state and this provides a good foundation for future work on containers in our framework. I'll plan to merge later today - but whether before or after would appreciate you guys taking another look to make sure we're tracking all the right follow-up work.

joeduffy · 2017-10-10T11:26:51Z

I'm inclined to focus on the other three first. This is a very slight delta from build, and given that we will also have function on the other side of that in terms of truly simple options, I'm not immediately sure this fourth variant will pay for itself. It's also a lot of policy we'll have to define and manage per-language we want to support, and feels a bit like reinventing automatic-Procfile inference which many folks have tried and tends to get messy. I'm inclined to just point folks to recommended two line Dockerfiles for common cases to start and focus on making that experience and the inline function experience really strong.

I strongly disagree.

First, there are two aspects: 1) having a declarative way of saying "this directory contains a Node.js/Python/whatever app, I don't want to mess with Docker but I want to containerize it"; and 2) inferring the kind of application in there. 1) is valuable independent of 2), but frankly, needing to state the language seems like ceremony and I suspect we'll do this as part of pulumi/pulumi#329 anyway. Interestingly, Deis ditched their entire workflow product and essentially just do this, with Draft.

Regarding 1), remember, one of the key reasons containers will be important to our model is polyglot. In that model, the container-as-function thing just doesn't work. (I write my config in JavaScript but want to run a Python app.) We already need to write and maintain these "runner" container images anyway, for the function scenario, so it's not clear to me what the added burden of you being able to name the function and have us anonymously build you a container is.

I would like us to include this in our suite of options.

joeduffy · 2017-10-10T11:30:09Z

api/service.ts

@@ -0,0 +1,77 @@
+// Copyright 2016-2017, Pulumi Corporation.  All rights reserved.
+
+export interface Containers {


Probably worth a JSDoc, since people might wonder about this.

joeduffy · 2017-10-10T11:31:14Z

api/service.ts

+     * An array of port mappings, indicating the container port to expose and
+     * the protocal that is used on that port.
+     */
+    portMappings?: {containerPort: number; protocol?: string}[];


Can we just name this ports?

(I understand that portMappings is more "technically correct", but simpler is better IMHO, and Docker tends to just use the term "ports" when referring to these.)

Yeah - ports makes sense.

joeduffy · 2017-10-10T11:33:37Z

api/service.ts

+     * An array of volume mounts, indicating a volume to mount and a path within
+     * the container at which to moung the volume.
+     */
+    mountPoints?: {containerPath: string; sourceVolume: Volume}[];


Can we just call this volumes? (Same reason as with ports.)

We could - it does feel like this one is a little different - it isn't just a list of volumes, it s a list of volume mounts. We could consider mounts? But the simplicity of volumes is appealing (and aligns with the Docker CLI), so I'll just go with that.

joeduffy · 2017-10-10T11:37:18Z

api/service.ts

+     */
+    function?: () => void;
+    /**
+     * The amount of memory to reserve for the container.


Should the comment be the minimum amount of memory?

The main question here is to understand our guarantees/semantics (hence minimum), not that I'm suggesting we rename the property or do anything crazy sophisticated yet.

Will we reject the deployment if these constraints cannot be met?

It's worth looking at the way Docker Compose surfaces limits and reservations: https://docs.docker.com/compose/compose-file/#resources.

In general, it's worth doing an audit of the Docker Compose settings all up to see if there's anything we want to explore in the future. As I think you know, my personal aesthetic aligns more closely with how Docker themselves expose concepts in Compose and Swarm (the gold standard), compared to, say, Kubernetes and ECS, both of which I find to be more complex and unnecessarily different from native Docker. Not saying anyone else has to agree, but there may be other good ideas to be inspired from.

Should the comment be the minimum amount of memory?

The main question here is to understand our guarantees/semantics (hence minimum), not that I'm suggesting we rename the property or do anything crazy sophisticated yet.

Will we reject the deployment if these constraints cannot be met?

There are two options we could expose here from the Docker level - memory and memoryReservation. Perhaps confusingly, the one called memory here is actually being mapped to memoryReservation. I did this because in general soft limits are more friendly. But we probably just need to expose both - and use their proper Docker names.

The Compose approach here is actually more "different" than Docker itself and ECS/Kube. I'm not sure I prefer this:

resources: reservations: memory: 20M

to this:

memoryReservation: 20

My suggestion would be to just expose the raw Docker options as options here - in this case both memory and memoryReservation.

joeduffy · 2017-10-10T11:42:15Z

api/service.ts

+     * An array of port mappings, indicating the container port to expose and
+     * the protocal that is used on that port.
+     */
+    portMappings?: {containerPort: number; protocol?: string}[];


I notice protocol is optional (?), so I assume it defaults to tcp?

joeduffy · 2017-10-10T12:02:29Z

aws/function.ts

-    constructor(name: string, policies: aws.ARN[], func: aws.serverless.Handler) {
+    constructor(name: string, func: aws.serverless.Handler) {
+        const policies = [
+            aws.iam.AWSLambdaFullAccess,


Aside that this reminds me about: We probably need to figure out the overall Pulumi IAM story, at least to have an answer when someone asks us "what if I don't want my function to have full access"? I'm guessing many folks will react negatively to the idea of granting full access with no overrides.

I'm nervous about the API implications if we need to go down this path, because we may eventually need to support taking () => { ... } or new Function(() => { ... }, iamRoles) or somesuch. And I honestly have no idea what such a set of IAM concepts that works at our level of abstraction looks like.

Do we have a work item tracking as part of our overall march to the first release?

#68 is tracking this.

Our plan is to have the IAM policy provide access to all (and only) resources in the account managed by the Pulumi application.

We will likely want to also provide some way to externally (config?) provide additional IAM policy to allow access to from code in an application for cases where you need to connect to external data inside that you know exists in your AWS account. We can discuss/track this in #68.

joeduffy · 2017-10-10T12:05:34Z

docker/javascriptrunner/Dockerfile

@@ -0,0 +1,4 @@
+FROM node


Even if you don't change this to have a common image, etc., can we at least rename it so that the name of the "runtime" and our terminology for runtimes elsewhere match? I.e., nodejs, not javascript.

Yeah - nodejsrunner makes sense.

BTW - totally open to other suggestions on the name.

joeduffy · 2017-10-10T12:16:16Z

docker/javascriptrunner/Dockerfile

@@ -0,0 +1,4 @@
+FROM node


FROM node:6.10.2?

Yeah - good catch.

joeduffy · 2017-10-10T12:21:14Z

api/service.ts

+     * An array of port mappings, indicating the container port to expose and
+     * the protocal that is used on that port.
+     */
+    portMappings?: {containerPort: number; protocol?: string}[];


As I read calling code, containerPort seems a little verbose. Also, I assume we (eventually) want the option of exposing the port as something different than the container's definition. So, how about:

ports?: { port: number, expose?: number, protocol?: string }[];

where port is the container port and expose is an optional override for what to expose it as (defaulting to port if undefined).

joeduffy · 2017-10-10T12:22:19Z

mock/service.ts

+    name: string;
+    getEndpoint: (containerName?: string, containerPort?: number) => Promise<cloud.Endpoint>;
+    constructor(name: string, args: cloud.ServiceArguments) {
+        throw new Error(`Service not yet supported in mock implementation.`);


Maybe file one uber work item to get containers working in the mock implementation?

Opened #92.

Mostly API renaming.

CyrusNajmabadi · 2017-10-10T19:45:13Z

api/service.ts

+ * A Task represents a container which can be [run] dynamically whenever (and
+ * as many times as) needed.
+ */
+export interface Task {


i'm finding the names Task and Service to be non explanatory on their own. Perhaps 'PersistentContainer' and 'RunOnceContainer' might be better (or something in the name to indicate the difference).

Service is standard across almost all Docker based systems for this concept, so I'm not in love with inventing something new. Task is a little less standard, but next to Service I think it is reasonably explanatory.

CyrusNajmabadi · 2017-10-10T19:45:54Z

docker/buildandpush.sh

+# buildandpush.sh builds and pushes the lukehoban/nodejsrunner Docker image to the public Docker hub.
+set -e
+
+docker build --tag lukehoban/nodejsrunner ./nodejsrunner


perhaps pulumi/nodejsrunner would be better?

CyrusNajmabadi

👍

lukehoban force-pushed the containers branch from dc15aea to 6221fba Compare September 30, 2017 05:27

lukehoban added 2 commits September 30, 2017 23:24

Move to use a Network Load Balancer

6a7f653

All exposed container/port pairs are allocated an NLB listener. NLBs are added as needed for every 50 exposed container/port pairs.

lukehoban mentioned this pull request Oct 2, 2017

Design for containers/tasks #6

Closed

Add a redis cache example

22e5d06

Also initial scaffolding for container-mountable FileSystem.

joeduffy reviewed Oct 4, 2017

View reviewed changes

CyrusNajmabadi reviewed Oct 4, 2017

View reviewed changes

lukehoban added 3 commits October 4, 2017 21:58

Initial support for Volumes

f7a2654

Also addresses some PR feedback.

Initial Task support

e275124

Tasks provide a handle to as Container which can be used to `run` an instance of the container at runtime.

Promises refactoring

d6beb0f

Do some promises refactoring to prepare for computing the container image from async operations - such as closure serialization.

lukehoban added 2 commits October 7, 2017 15:58

Extend the function-as-container example

6a40e6b

Use `scale: 2` and add code so that it is visible which instance is being used.

lukehoban mentioned this pull request Oct 9, 2017

Support richer workflows on Volumes #84

Closed

lukehoban mentioned this pull request Oct 9, 2017

Pass full local folder ZIP to container for function-based containers #86

Closed

Add some TODOs

d435e52

lukehoban changed the title ~~[WIP] Support containers~~ Initial support for containers Oct 9, 2017

lukehoban mentioned this pull request Oct 9, 2017

Support a build option for Docker containers #87

Closed

lukehoban added 2 commits October 9, 2017 13:27

Merge branch 'master' into containers

67624ff

Get const clean in service.ts

409d34d

joeduffy approved these changes Oct 10, 2017

View reviewed changes

Address PR feedback

56c52cf

Mostly API renaming.

lukehoban merged commit 06c4253 into master Oct 10, 2017

lukehoban deleted the containers branch October 10, 2017 15:19

CyrusNajmabadi reviewed Oct 10, 2017

View reviewed changes


		getHostAndPort: (containerName: string, containerPort: number) => Promise<string>;

		constructor(name: string, args: cloud.ServiceArguments) {

		@@ -0,0 +1,77 @@
		// Copyright 2016-2017, Pulumi Corporation. All rights reserved.

		export interface Containers {


		import * as pulumi from "pulumi";

		let _config = new pulumi.Config("cloud-aws:config");

Initial support for containers #71

Initial support for containers #71

Conversation

lukehoban commented Sep 30, 2017 • edited Loading

lukehoban commented Sep 30, 2017

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

joeduffy Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 4, 2017

CyrusNajmabadi commented Oct 4, 2017

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

joeduffy Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

lukehoban Oct 5, 2017 • edited Loading

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

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

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

Choose a reason for hiding this comment

lukehoban commented Oct 4, 2017

lukehoban commented Oct 4, 2017

lukehoban commented Oct 9, 2017

lukehoban commented Oct 9, 2017

joeduffy commented Oct 10, 2017 • edited Loading

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

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

lukehoban commented Sep 30, 2017 •

edited

Loading

joeduffy Oct 4, 2017 •

edited

Loading

joeduffy Oct 4, 2017 •

edited

Loading

lukehoban Oct 5, 2017 •

edited

Loading

joeduffy commented Oct 10, 2017 •

edited

Loading