Initial transcription (with minor updates) of Knative Runtime Contract. #1471

evankanderson · 2018-07-03T18:58:25Z

Fixes #627

Proposed Changes

Adds a definition of the common (expected) execution environment for containers in Knative.
This was substantially discussed in this Google doc with various amendments which improved it. It's time to commit some of this to formal contract; I've selected a handful of reviewers from that doc, but encourage additional suggestions/improvements.

One additional item to follow on from this would be some guidance to implementers & operators about where to expose contextual information to containers; the main avenues would be environment variables, mounted config maps, network services, and request headers. That will be a subsequent PR, though.

Release Note

Defines expectations for the environment presented to containers.

evankanderson · 2018-07-03T18:58:50Z

/assign @RichieEscarez for tech writing if he's interested.

grantr

Minor nits but otherwise LGTM. Holding for nits.

/lgtm
/hold

grantr · 2018-07-03T22:24:49Z

docs/runtime-contract.md

+
+| Name | Meaning |
+|------|---------|
+| `PORT` | Incress `containerPort` for ingress requests and health checks. See [Inbound network connectivity](#inbound-network-connectivity) for more details.


typo: "Incress"

grantr · 2018-07-03T22:25:29Z

docs/runtime-contract.md

+### Devices
+
+Developers and container providers MUST NOT request additional devices beyond
+the "Default Devices".


Is there a Default Devices section? I only see Default Filesystems.

I think it refers to this part of the OCI spec.

However, that section says "In addition to any devices configured with this setting...", where "this setting" appears to be the enclosing "Devices" section. The Devices section in the OCI says that devices is an OPTIONAL list of devices that MUST be available, which MAY be supplied any way the runtime prefers.

So my reading is that "Default Devices" allows, by inclusion, non-Default Devices, meaning it contradicts this section.

On the interpretation rule given in "Background" ("... this document is assumed to override the general OCI recommendations"), it still makes sense, but it could be differently phrased. For example:

Developers and container providers MUST NOT use OCI devices to request additional devices beyond the OCI specification "Default Devices".

Thanks, this wording with the explicit reference to the OCI spec is much clearer.

grantr · 2018-07-03T22:27:52Z

docs/runtime-contract.md

+> propagation.
+
+This option should only be set by the operator or platform provider, and MUST
+NOT be configurable by the Knative developer. As mount propagation may be part


Does "Knative developer" mean a developer who contributes to Knative, or a developer who interacts with a Knative system? It seems to mean the former at https://github.com/knative/serving/pull/1471/files#diff-e5a9114cbfb7f985c0da2dfe04221c26R393, but I'm not sure what it means here or in the next 3 sections.

Applied @jchesterpivotal's suggestion to define the personas of interest in the "Background" section. In this doc, "developer" or "Knative developer" should always mean an end-user who is writing code which will be run on knative (either as the main workload, or as part of a knative-aware tool or framework).

grantr · 2018-07-03T22:29:01Z

docs/runtime-contract.md

+* `/sys/fs/cgroup/cpu/cpu.cfs_quota_us`
+
+Additionally, operators [may restrict or prevent CPU scheduling for instances
+when no requests are active](https://github.com/knative/serving/issues/848)).


Extra close parens here.

dprotaso

My only other nit is the inconsistency between the usage of kubernetes and Kubernetes

dprotaso · 2018-07-04T00:47:53Z

docs/runtime-contract.md

+same port as HTTP/1.1. The developer MAY specify this port at deployment; if the
+developer does not specify a port, the platform provider MUST provide a default.
+Only one inbound `containerPort` SHALL be specified in the
+`[core.v1.Container](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#containerport-v1-core)`


formatting issue - code block ticks need to be inside the square brackets

dprotaso · 2018-07-04T00:49:47Z

docs/runtime-contract.md

+#### Meta Requests
+
+The
+`[core.v1.Container](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#container-v1-core)`


formatting issue - code block ticks need to be inside the square brackets

dprotaso · 2018-07-04T00:51:56Z

docs/runtime-contract.md

+
+| Name   | Meaning                                                                                                                                             |
+| ------ | --------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `PORT` | Incress `containerPort` for ingress requests and health checks. See [Inbound network connectivity](#inbound-network-connectivity) for more details. |


dprotaso · 2018-07-04T01:03:38Z

docs/runtime-contract.md

+- `/sys/fs/cgroup/cpu/cpu.cfs_quota_us`
+
+Additionally, operators
+[may restrict or prevent CPU scheduling for instances when no requests are active](https://github.com/knative/serving/issues/848)).


Extra ) on the end

dprotaso · 2018-07-04T01:04:55Z

docs/runtime-contract.md

+
+Additionally, operators
+[may restrict or prevent CPU scheduling for instances when no requests are active](https://github.com/knative/serving/issues/848)).
+This is not currently feasible on Kubernetes, but is desireable for multitenant


glyn · 2018-07-04T07:55:00Z

docs/runtime-contract.md

+### Control Groups
+
+The control groups (cgroups) MUST be configured by the operator or platform
+provider. The cgroup devices SHOULD be mounted as read-only.


Which cgroups, if any, are mandatory? This paragraph could be interpreted as saying "it doesn't matter which cgroups are configured, but it is the operator's or platform provider's responsibility to do the configuration" or even as "all known control groups must be configured" (which will be vague if new control groups are added to Linux in future).

New cgroups are almost certain to appear -- eg, a large IaaS might like to provide fractional GPUs, TPUs or other specialist processing hardware.

Thanks, rephrased this to more closely follow the OCI spec and refer to "cgroup controllers", and added the the selection of cgroup controllers was an operator decision.

glyn

LGTM except for clarification needed over which cgroups must be configured.

jchesterpivotal

One thing we might like to add is a section defining personae referred to in the document -- operator, developer, customer, etc. We use these a little inconsistently.

jchesterpivotal · 2018-07-05T13:41:12Z

docs/runtime-contract.md

+
+In particular, the default Knative implementation relies on Kubernetes behavior
+to implement container operation. In some cases, current Kubernetes behavior in
+2018 is not as performant as recommended in that documentation. The goal of the


What is "that documentation" in this context? Does it mean the OCI spec? Linux container configuration? Both?

Oops, that was "this document". Thanks!

jchesterpivotal · 2018-07-05T13:42:33Z

docs/runtime-contract.md

+In particular, the default Knative implementation relies on Kubernetes behavior
+to implement container operation. In some cases, current Kubernetes behavior in
+2018 is not as performant as recommended in that documentation. The goal of the
+Knative developers is to push as much of the required functionality into


Perhaps "Knative authors", "Knative contributors", or "Knative contributing developers".

Thanks, I'd missed this. "Knative authors" seems accurate here, and was probably what drove the "who is a developer" question.

jchesterpivotal · 2018-07-05T13:44:09Z

docs/runtime-contract.md

+In order to achieve these properties, containers which are operated as part of a
+serverless platform SHOULD observe the following properties:
+
+- Fast startup time (<1s in the presence of container image layer caching),


Perhaps "<1s until a request or event can be processed, given the presence of container image layer caching"

jchesterpivotal · 2018-07-05T13:45:45Z

docs/runtime-contract.md

+  contents from a particular execution. Because containers (particularly failing
+  containers) can experience frequent starts, operators or platform providers
+  SHOULD limit the total space consumed by these failures.
+- The termination message on container exit should be written to


Suggest:

The termination message on container exit, or the last few lines of log output (as per Kubernetes termination messages), SHOULD be written to /dev/termination-log.

I'm not sure which of "and/or" and "exclusive or" is meant by "or" here. I'm also not sure what is intended by "per Kubernetes termination messages" -- does this mean they are part of this standard by reference, or are they given as an example?

Sorry, was attempting to reference FallbackToLogsOnError as reasonable behavior. Mentioned that explicitly instead of by partially elided reference.

jchesterpivotal · 2018-07-05T13:59:42Z

docs/runtime-contract.md

+of its source, the selected port will be made available in the `PORT`
+environment variable.
+
+It is expected that the serverless platform will provide HTTPS termination and


"It is expected ... will" is a little vague -- does this mean SHOULD or MAY?

SHOULD, thanks.

jchesterpivotal · 2018-07-05T14:55:58Z

docs/runtime-contract.md

+sandbox MAY be enforced by the platform operator; any such application profiles
+SHOULD be configured and applied in a consistent mechanism outside of the
+container specification. As the seccomp policy may be part of the platform
+security hardening, operators may tune this from time to time as the threat


"may" -> "MAY"; or perhaps even "may" -> "SHOULD"

jchesterpivotal · 2018-07-05T14:56:47Z

docs/runtime-contract.md

+
+This option should only be set by the operator or platform provider, and MUST
+NOT be configurable by the Knative developer. As mount propagation may be part
+of the platform security hardening, operators may tune this from time to time as


"may" -> "MAY" or "SHOULD"

jchesterpivotal · 2018-07-05T14:57:38Z

docs/runtime-contract.md

+
+### Masked Paths
+
+This option should only be set by the operator or platform provider, and MUST


"should" -> "SHOULD". Though it's unclear why this wouldn't be "MUST" if developers are given "MUST NOT".

Or "MAY", come to think of it.

jchesterpivotal · 2018-07-05T14:57:49Z

docs/runtime-contract.md

+
+This option should only be set by the operator or platform provider, and MUST
+NOT be configurable by the Knative developer. As masked paths may be part of the
+platform security hardening, operators may tune this from time to time as the


"may" -> "MAY" or "SHOULD".

jchesterpivotal · 2018-07-05T14:58:11Z

docs/runtime-contract.md

+
+### Readonly Paths
+
+This option should only be set by the operator or platform provider, and MUST


"should" -> "SHOULD" or "MAY"

evankanderson · 2018-07-17T23:55:15Z

Thanks for the review, especially @jchesterpivotal

Standardize capitalization of Kubernetes.

evankanderson · 2018-07-20T03:22:50Z

/hold cancel

This is ready for another look, and I totally missed that it still had a hold.

grantr

I think all of @jchesterpivotal's comments have been resolved, but if I'm wrong about that @jchesterpivotal, please open an issue or PR correcting me.

I have some minor comments but since time is of the essence I'm giving @evankanderson the option to ignore them.

/lgtm
/hold

grantr · 2018-07-20T18:17:14Z

docs/runtime-contract.md

-on the inbound container port for readiness and liveness checks; platform
-providers SHOULD disallow other probe methods.
+definition below). If specified, liveness and readiness probes are REQUIRED to
+be of the the `httpGet` or `tcpSocket` types, and MUST target the inbound


Double the here

grantr · 2018-07-20T18:22:45Z

docs/runtime-contract.md

+Additionally, operators or the platform MAY restrict or prevent CPU scheduling
+for instances when no requests are active,
+[where this capability is available](https://github.com/knative/serving/issues/848).
+The Knative developers are currently discussing the best implementations options


This should be "Knative authors" like https://github.com/knative/serving/pull/1471/files/f952dc8273e034b37f075c4e8b1f3db6cac96d6a..da4d6a7b6e0ab70310a7bb4d8346b52d4d20f173#diff-e5a9114cbfb7f985c0da2dfe04221c26R46

Thanks, excised "Knative developers" everywhere. It's now "Knative authors" here, and "developers" elsewhere.

jchesterpivotal

@grantr I believe all my nitpicks have been fixed and my concerns have been discussed.

/lgtm

Fix typo.

google-prow-robot · 2018-07-20T18:36:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, grantr, jchesterpivotal

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [evankanderson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

grantr

/lgtm

evankanderson · 2018-07-20T19:10:07Z

/hold cancel

Initial transcription (with minor updates) of Knative Runtime Contract.

e1ac940

google-prow-robot assigned glyn, jchesterpivotal, josephburnett, mbehrendt and mdemirhan Jul 3, 2018

google-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 3, 2018

google-prow-robot requested review from mattmoor and vaikas July 3, 2018 18:58

google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2018

evankanderson added 2 commits July 3, 2018 13:04

Re-add PORT env var. Fix a few html-isms missed earlier.

221cf9a

Fix a few html-isms missed earlier.

dcfe5f1

evankanderson mentioned this pull request Jul 3, 2018

Publish runtime contract knative/docs#50

Closed

google-prow-robot assigned grantr Jul 3, 2018

grantr approved these changes Jul 3, 2018

View reviewed changes

google-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jul 3, 2018

Run https://github.com/prettier/prettier --prose-wrap=always

f952dc8

google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2018

dprotaso reviewed Jul 4, 2018

View reviewed changes

glyn reviewed Jul 4, 2018

View reviewed changes

glyn suggested changes Jul 4, 2018

View reviewed changes

jchesterpivotal reviewed Jul 5, 2018

View reviewed changes

evankanderson added 2 commits July 17, 2018 15:55

Resolve feedback from multiple sources.

37fcad3

"Container providers" is not used elsewhere in the doc.

d6195ac

Further justify startup-once behavior.

da4d6a7

Standardize capitalization of Kubernetes.

google-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2018

grantr approved these changes Jul 20, 2018

View reviewed changes

google-prow-robot added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 20, 2018

jchesterpivotal approved these changes Jul 20, 2018

View reviewed changes

Remove "Knative developer" for either "Knative authors" or "developer".

03ef87d

Fix typo.

google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2018

grantr approved these changes Jul 20, 2018

View reviewed changes

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2018

google-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2018

google-prow-robot merged commit 97cf40d into knative:master Jul 20, 2018

evankanderson deleted the run-walk-crawl branch July 20, 2018 20:43


		### Masked Paths

		This option should only be set by the operator or platform provider, and MUST


		### Readonly Paths

		This option should only be set by the operator or platform provider, and MUST

Initial transcription (with minor updates) of Knative Runtime Contract. #1471

Initial transcription (with minor updates) of Knative Runtime Contract. #1471

Conversation

evankanderson commented Jul 3, 2018

Proposed Changes

evankanderson commented Jul 3, 2018

grantr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso left a comment • 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

glyn left a comment

Choose a reason for hiding this comment

jchesterpivotal 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

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

evankanderson commented Jul 17, 2018

evankanderson commented Jul 20, 2018

grantr 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

jchesterpivotal left a comment

Choose a reason for hiding this comment

google-prow-robot commented Jul 20, 2018

grantr left a comment

Choose a reason for hiding this comment

evankanderson commented Jul 20, 2018

dprotaso left a comment •

edited

Loading