Clean-up Runtime Contract by dgerd · Pull Request #4035 · knative/serving

dgerd · 2019-05-07T23:40:17Z

This change makes numerous cleanups to the runtime contract in an
attempt to improve the readability of the document and make the document
more useful for the intended audience.

Moves developer facing statements to a new runtime-user-guide.
Focuses runtime-contract on operator/platform-provider.
Add links to Conformance tests that test Runtime Contract statements.
Corrects, updates, or removes statements to more accurately represent
today's Knative runtime.
Updates to informative or removes most untestable statements
Copies in important OCI runtime requirements we previously referenced
Removes reference to OCI specification that didn't bring new
requirements.

Ref: #2539, #2973, #4014, #4027

/cc @evankanderson @mattmoor @tanzeeb @markusthoemmes @vaikas-google @tcnghia

This change makes numerous cleanups to the runtime contract in an attempt to improve the readability of the document and make the document more useful for the intended auidence. * Moves developer facing statements to a new `runtime-user-guide`. Focuses `runtime-contract` on operator/platform-provider. * Add links to Conformance tests that test Runtime Contract statements. * Corrects, updates, or removes statements to more accurately represent today's Knative runtime. * Updates to informative or removes most untestable statements * Copies in important OCI runtime requirements we previously referenced * Removes reference to OCI specification that didn't bring new requirements. Ref: knative#2539, knative#2973, knative#4014, knative#4027

Co-Authored-By: dgerd <dangerd@google.com>

tanzeeb

/lgtm

tanzeeb · 2019-05-08T14:19:42Z

@@ -181,51 +157,39 @@ for purposes of scaling CPU and removing idle containers.

 #### Protocols and Ports


LGTM 👍

Summary: http1 and h2c are supported, http1 by default. No automatic upgrades.

knative-prow-robot · 2019-05-08T14:20:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dgerd, tanzeeb
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: evankanderson

If they are not already assigned, you can assign the PR to them by writing /assign @evankanderson in a comment when ready.

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

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

mattmoor · 2019-05-10T01:22:02Z

+## Connection
+
+A Knative container must package a webserver that can respond to HTTP/1.1
+requests on a specified port.


What about http/2?

HTTP/2.0 is covered in the Protocols section.

My understanding is that we expect clients to implement h2c and not h2 which requires an initial upgrade request over HTTP/1.1. This is why I have set the expectation that responding to an HTTP/1.1 request is required.

It might be worth clarifying that containers can implement h2c with the logic you provided here.

Makes sense. I can add this.

mattmoor · 2019-05-10T23:46:59Z

/lgtm

Will leave /approve for @evankanderson

evankanderson

I have a slight preference for splitting up functional vs presentational changes in the future, as it allows the two conversations to proceed at a different pace.

Part of the reason for the structure of the existing spec was due to questions asked during the drafting about how different parts of the OCI contract applied -- in some cases, the mechanism was suitable for operator consumption but had multitenancy or security concerns, so needed to be cordoned off from developers. I'm not sure how much these notes add/remove from the spec, but most were added in response to questions. 😁

evankanderson · 2019-05-13T20:16:01Z

-particularly concerned with the expectations of _developers_ (and _language and
-tooling developers_, by extension) running code in the environment.
+This document is aimed at _operators_ with the goal of providing a consistent
+runtime environment for _developers_.


Not sure if you want to add the additional detail of language and tooling developers being additional roles that are concerned about the environment as consumers.

I can work that back in.

Suggested change

runtime environment for _developers_.

runtime environment for _developers_ (who may interact via tools created by _language and tooling developers_).

evankanderson · 2019-05-13T20:28:44Z

-  `poststop` hooks to platform operators rather than developers. All of these
-  hooks are defined in the context of the runtime namespace, rather than the
-  container namespace, and may expose system-level information (and are
+  container MUST be sent a `SIGTERM` signal when it is killed to allow for a


This can at most be a SHOULD, because there are a number of conditions which could reasonable cause a container to stop running without receiving a SIGTERM:

Kernel SIGKILL due to cgroup resource exhaustion / machine resource exhaustion (particularly memory)

Kernel SIGKILL due to AppArmor / BPF policy violation

Operator SIGKILL due to exceptional circumstances

Machine (hardware) failure

There's also the fun case of network failure isolating or partially-isolating the machine, which may not affect SIGTERM/SIGKILL, but may screw up other shutdown.

I agree here, but I would like to distinguish between this may not happen due to the reason your container is being terminated versus this may vary from platform to platform. Perhaps we can bound the situations for which a SIGTERM must be sent (i.e. normal autoscale actions or deployment actions). We can work on the exact wording, but thoughts here?

Yes, I think saying that "normal orderly shutdown MUST send a SIGTERM" would be acceptable.

evankanderson · 2019-05-13T20:43:31Z

  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.
- A container should write its own termination message to `/dev/termination-log`


/dev/termination-log is inherited from the Kubernetes terminationMessagePolicy, which is pretty helpful for collecting information from crashing containers.

I removed it because:

It is written from the standpoint of the container. Should the container write the termination message on all Knative platforms (i.e. All installs MUST implement and offer terminationMessagePolicy), or just some platforms?

We do not have test coverage for this and I couldn't quite find where we actually consume this information and present it back to users in our API. It is not clear to me that we do this at all.

Most importantly, this seems like an overly limiting runtime requirement that is not in the critical path at the MUST/SHOULD level and does not add much developer value at the MAY level.

I am not stuck on this one and could be convinced to just rewording it.

Yeah -- good catch. I don't think we need to keep this, because it could be a scalability / visibility concern to report these up to the control plane.

In particular, what happens if you have a simultaneous crash of 10k Pods -- aggregation termination messages back to the Revision could be fairly expensive, with each Pod crash causing another attempted update of the Revision. It looks like these don't get aggregated from Pod -> ReplicaSet in k8s, so let's follow that precedent.

evankanderson · 2019-05-13T20:46:54Z

-### Hooks
-
-Operation hooks SHOULD NOT be configurable by the Knative developer. Operators
-or platform providers MAY use hooks to implement their own lifecycle controls.


This was removed because it's not developer-facing?

The only hooks I could find here were the OCI hooks ( https://github.com/opencontainers/runtime-spec/blob/master/config.md#posix-platform-hooks ). We already describe these hooks starting on line 90. This was duplicative.

SGTM, I copied all the sections from OCI, I think.

evankanderson · 2019-05-13T20:48:51Z

-collected and retained in a developer-accessible logging repository.
+A read from the `stdin` file descriptor on the container [SHOULD](https://github.com/knative/serving/blob/master/test/conformance/file_descriptor_test.go)
+always result in `EOF`. The `stdout` and `stderr` file descriptors on the container
+MUST be collected and retained in a developer-accessible logging repository.


Wondering about the upgrade from SHOULD to MUST.

It would be nice to have a minimum logging requirement that is common across all Knative installs. Right now both /var/log and stdout are SHOULD which means that a developer need to go hunting through the docs or configuration for each Knative install to understand how logs are collected. My understanding is that right now in Knative Serving we collect stdout and stderr for all containers so we are already doing this.

I think one reason for SHOULD was that it wasn't entirely clear how to test that the logs were actually being retained.

I'm fine upgrading to MUST.

evankanderson · 2019-05-13T21:38:06Z

+[MUST](https://github.com/knative/serving/blob/master/test/conformance/user_test.go) run
 the container as the specified user ID if allowed by the platform (see below).
-If no `runAsUser` is specified, a platform-specific default SHALL be used.
-Platform Providers SHOULD document this default behavior.


This is removing a documentation recommendation.

Would it make sense instead to have a section of the spec recommending the most significant documentation areas where platform providers may differ?

I am going to take another stab at updating this after some more recent conversations about how OpenShift handles runAsUser.

It would be nice to have documentation recommendations and requirements elsewhere so you don't have to go through this entire document to find them all.

Is there a  here?

evankanderson · 2019-05-13T21:38:30Z

-
-The namespace configuration MUST be provided by the operator or platform
-provider; developers or container providers MUST NOT set or assume a particular
-namespace configuration.


Why remove this?

Looking at this again I am going to put it back and update it. I was assuming Kubernetes Namespace and not Container Namespace when I was deleting this.

Looks like this is still missing?

evankanderson · 2019-05-13T21:40:27Z

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


Again, wondering if it's worth documenting where the responsibility for these setting lies, given that they are in the OCI spec.

This section seemed overly limiting and does not seem to prevent the operation of Knative. Why must this not be configurable by the developer? It seemed to me like you could let the developer have control over some of these things. Given that the OCI interface should not be available within the container the only way to configure this would be through the Knative API. An extension of the Knative API to allow certain paths to be masked or read-only didn't seem crazy to me.

I'm fine with dropping this.

evankanderson · 2019-05-13T21:42:44Z

+## Connection
+
+A Knative container must package a webserver that can respond to HTTP/1.1
+requests on a specified port.


It might be worth clarifying that containers can implement h2c with the logic you provided here.

evankanderson · 2019-05-13T21:46:28Z

+
+# Logging
+
+Log statements should be sent to `stdout` and `stderr`. Log statements sent to


It would be nice to include /var/log or /dev/log as alternatives to the streams. /dev/log is particularly nice, as it natively supports multiline log statements (but I think our current fluentd configuration does not provide /dev/log).

These are not guaranteed to be in a knative install. They are listed as SHOULD items.

evankanderson · 2019-06-03T23:37:35Z

/hold

Will look at this tomorrow

* Describe current behavior for HTTP1.1 and HTTP2 * Provide links to relevant tests for keywords Ref: knative#4035 Fixes: knative#4283

evankanderson · 2019-06-06T22:07:49Z

+  the Knative cluster.
+- **Language and tooling developers** typically write tools used by
+    _developers_ to package code into containers. As such, they are concerned
+    that tooling which wraps developer code complies with this runtime contract.


Add a section clarifying that this is descriptive advice rather than prescriptive requirements? "Your container will fit better if it follows the following guidance..."

evankanderson · 2019-06-06T22:09:05Z

-particularly concerned with the expectations of _developers_ (and _language and
-tooling developers_, by extension) running code in the environment.
+This document is aimed at _operators_ with the goal of providing a consistent
+runtime environment for _developers_.


Suggested change

runtime environment for _developers_.

runtime environment for _developers_ (who may interact via tools created by _language and tooling developers_).

evankanderson · 2019-06-06T22:10:32Z

-  `poststop` hooks to platform operators rather than developers. All of these
-  hooks are defined in the context of the runtime namespace, rather than the
-  container namespace, and may expose system-level information (and are
+  container MUST be sent a `SIGTERM` signal when it is killed to allow for a


Yes, I think saying that "normal orderly shutdown MUST send a SIGTERM" would be acceptable.

evankanderson · 2019-06-06T22:11:06Z

+  [SHOULD](https://github.com/knative/serving/blob/master/test/conformance/container_test.go)
+  restrict the use of `prestart`, `poststart`, and `poststop` hooks to platform
+  operators rather than developers. All of these hooks are defined in the context
+  of the runtime namespace, rather than the


s/runtime namespace/host runtime namespace/ to be extra clear?

evankanderson · 2019-06-06T22:13:40Z

+  of the runtime namespace, rather than the
+  container namespace, and MAY expose system-level information (and are
  non-portable).
- Failures of the developer-specified process MUST be logged to a


I'm assuming this was removed because it's not currently testable?

Can we leave a comment suggesting this for future inclusion once we have appropriate log infrastructure in place? I think it would be testable if we had a standard log-tail interface:

start tailing

Deploy container which runs echo "Not gonna happen" 1>&2; exit 1

Check for string in tail results.

evankanderson · 2019-06-06T22:30:08Z

  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.
- A container should write its own termination message to `/dev/termination-log`


Yeah -- good catch. I don't think we need to keep this, because it could be a scalability / visibility concern to report these up to the control plane.

In particular, what happens if you have a simultaneous crash of 10k Pods -- aggregation termination messages back to the Revision could be fairly expensive, with each Pod crash causing another attempted update of the Revision. It looks like these don't get aggregated from Pod -> ReplicaSet in k8s, so let's follow that precedent.

evankanderson · 2019-06-06T22:31:31Z

-### Hooks
-
-Operation hooks SHOULD NOT be configurable by the Knative developer. Operators
-or platform providers MAY use hooks to implement their own lifecycle controls.


SGTM, I copied all the sections from OCI, I think.

mattmoor · 2019-06-07T03:17:38Z

 SHOULD NOT be exposed within the container. The operator or platform provider
-MAY have the ability to directly interact with the OCI interface, but that is
-beyond the scope of this specification.
+MAY have the ability to directly interact with the OCI interface.


@evankanderson @dgerd did we want to reword the OCI interface parts?

I think Dan was saying that we could probe at some well-known locations, but couldn't definitively guarantee that this was not exposed.

I'm fine with leaving as-is or moving to the developer guide.

evankanderson

Finished my pass; a few missed items from last time and some more discussion, but I think many of these can be closed with  comments rather than needing to get the final details right in this pass.

Observability in particular I expect to improve over the next 3-5 months, so there may be some better answers there soon.

evankanderson · 2019-06-10T16:54:53Z

-collected and retained in a developer-accessible logging repository.
+A read from the `stdin` file descriptor on the container [SHOULD](https://github.com/knative/serving/blob/master/test/conformance/file_descriptor_test.go)
+always result in `EOF`. The `stdout` and `stderr` file descriptors on the container
+MUST be collected and retained in a developer-accessible logging repository.


I think one reason for SHOULD was that it wasn't entirely clear how to test that the logs were actually being retained.

I'm fine upgrading to MUST.

evankanderson · 2019-06-10T17:00:24Z

 #### Dev symbolic links

-As specified by OCI.
+The following symbolic links MUST exist within the container:


Where did the OCI reference end up?

evankanderson · 2019-06-10T17:10:06Z


-The container MUST accept HTTP/1.1 requests from the environment. The
-environment
-[SHOULD offer an HTTP/2.0 upgrade option](https://http2.github.io/http2-spec/#discover-http)


Followed up there -- empty == HTTP/1.1 with later upgrade option seems reasonable.

evankanderson · 2019-06-10T17:12:50Z

-Only one inbound `containerPort` SHALL be specified in the
+The developer MAY specify a container port value at deployment; if the developer does not specify a port, the platform provider
+[MUST](https://github.com/knative/serving/blob/master/test/conformance/service_test.go) provide a default.
+Only one inbound `containerPort` [SHALL](https://github.com/knative/serving/blob/master/test/conformance/container_test.go) be specified in the


It's not 100% clear here whether not specifying a port in the spec (and just responding to $PORT in the container) is or should be acceptable.

Thoughts?

evankanderson · 2019-06-10T17:16:30Z

-of its source, the selected port will be made available in the `PORT`
-environment variable.
-
-The platform provider SHOULD configure the platform to perform HTTPS termination


I don't see a comment on HTTPS termination yet.

evankanderson · 2019-06-10T18:13:41Z

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


I'm fine with dropping this.

evankanderson · 2019-06-10T18:37:33Z

+      - name: "h2c"
+        containerPort: 8081
+...
+```


Define here or in connection that the proxy terminates SSL and may convert HTTP versions automatically, so the user container only needs to speak a single HTTP version.

(I found it strange to have "Connection" and "Protocols" so far separated, FWIW.

evankanderson · 2019-06-10T18:38:35Z

+persistent data as it is not retained in the event of container termination.
+
+It is recommended to store temporary state such as local caches or working data in `/tmp`
+as this space is guaranteed to be present and mounted Read-Write.


It's probably also worth pointing out here the option of off-node storage via API interfaces like Memcache/Redis, and recommending that for session storage.

In particular:

Sessions may not be sticky; subsequent requests from the same user may not arrive at the same container.

Local disk state for session storage management may be lost when containers are scaled down (possibly to zero).

evankanderson · 2019-06-10T18:42:29Z

+It is recommended to store temporary state such as local caches or working data in `/tmp`
+as this space is guaranteed to be present and mounted Read-Write.
+
+# Probes


Why is this H1 when the other sections above are H2?

evankanderson · 2019-06-10T18:42:42Z

+# Probes
+
+By default, the readiness of a Knative container is determined by a successful TCP
+response on the specified port for incoming traffic. If this readiness check is not


s/response/connection/

dgerd · 2019-07-29T17:20:07Z

Some of this has merged. Some of this is in conflict now. Going to close this and get the remaining updates put in separately.

knative-prow-robot requested review from evankanderson, markusthoemmes, mattmoor, tanzeeb, tcnghia and vaikas May 7, 2019 23:40

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 7, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 7, 2019

mattmoor-sockpuppet reviewed May 7, 2019

View reviewed changes

Update docs/runtime-user-guide.md

3abf2fc

Co-Authored-By: dgerd <dangerd@google.com>

mattmoor-sockpuppet reviewed May 7, 2019

View reviewed changes

Apply suggestions from code review

f51806f

Co-Authored-By: dgerd <dangerd@google.com>

mattmoor-sockpuppet reviewed May 7, 2019

View reviewed changes

mattmoor assigned evankanderson May 8, 2019

tanzeeb approved these changes May 8, 2019

View reviewed changes

knative-prow-robot assigned tanzeeb May 8, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2019

jonjohnsonjr reviewed May 8, 2019

View reviewed changes

Spelling/Typo fixes

546d6db

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 8, 2019

mattmoor-sockpuppet reviewed May 8, 2019

View reviewed changes

This was referenced May 8, 2019

Add conformance test to validate runAsUser is as specified #3223

Closed

Add conformance tests to validate OCI devices are disallowed #2973

Closed

Update wording around console presence

b157b7d

mattmoor-sockpuppet reviewed May 8, 2019

View reviewed changes

Typo fix

d425cb1

mattmoor reviewed May 10, 2019

View reviewed changes

Code Review comments

5f06315

knative-prow-robot assigned mattmoor May 10, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2019

dgerd mentioned this pull request May 13, 2019

Add additional filesystem checks for OCI devices #4074

Merged

evankanderson reviewed May 13, 2019

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2019

dgerd pushed a commit to dgerd/serving that referenced this pull request Jun 6, 2019

Runtime Contract: Update Protocols and Ports section

ca48d6c

* Describe current behavior for HTTP1.1 and HTTP2 * Provide links to relevant tests for keywords Ref: knative#4035 Fixes: knative#4283

dgerd mentioned this pull request Jun 6, 2019

Runtime Contract: Update Protocols and Ports section #4286

Closed

evankanderson reviewed Jun 6, 2019

View reviewed changes

mattmoor reviewed Jun 7, 2019

View reviewed changes

evankanderson reviewed Jun 10, 2019

View reviewed changes

evankanderson mentioned this pull request Jul 2, 2019

Clean-up the use of RFC2119 keywords #4550

Merged

dgerd closed this Jul 29, 2019

		@@ -181,51 +157,39 @@ for purposes of scaling CPU and removing idle containers.

		#### Protocols and Ports

	runtime environment for _developers_.
	runtime environment for _developers_ (who may interact via tools created by _language and tooling developers_).


		# Logging

		Log statements should be sent to `stdout` and `stderr`. Log statements sent to

Conversation

dgerd commented May 7, 2019

Uh oh!

tanzeeb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented May 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattmoor commented May 10, 2019

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment