Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean-up the use of RFC2119 keywords #4550

Merged
merged 3 commits into from
Jul 13, 2019
Merged

Conversation

duglin
Copy link

@duglin duglin commented Jun 27, 2019

Please check each change carefully. I tried to not change the semantics
meaning of anything, rather just wanted to make sure each use of an
RFC2119 keyword was actually used correctly - meaing it's "normative".
RFC2119 does not distinguish between lower-case and upper-case use of the
words, so "may" and "MAY" have the same meaning. However, in most cases
people use upper-case versions of the words so the 1) they stand-out, and
2) it's more obvious that the author used the RFC2119 keyword on purpose and
didn't just use it by mistake as part of the descriptive english text.

We should add an RFC2119 keyword checker to docs like this to catch
mis-uses of the keywords going forward (meaning using lower-case by mistake).

Signed-off-by: Doug Davis [email protected]

/lint

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 27, 2019
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 27, 2019
@markusthoemmes
Copy link
Contributor

/assign @evankanderson @dgerd

Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup and the careful eye!

A few suggested deltas, but this overall helps clarify what is in and out of scope for testing.

docs/runtime-contract.md Show resolved Hide resolved
assumes the
[Linux Container Configuration](https://github.com/opencontainers/runtime-spec/blob/master/config-linux.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 this documentation. The goal of the
Knative authors is to push as much of the required functionality into Kubernetes
2018 is not as performant as specified in this documentation. The goal of the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is "recommended", I think this is "envisioned".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

2018 is not as performant as recommended in this documentation. The goal of the
Knative authors is to push as much of the required functionality into Kubernetes
2018 is not as performant as specified in this documentation. The goal of the
Knative authors is to push as much of the needed functionality into Kubernetes
and/or Istio as possible, rather than implementing reach-around layers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a line which hasn't aged well. How about:

Suggested change
and/or Istio as possible, rather than implementing reach-around layers.
and/or HTTP routers as possible, rather than implementing reach-around layers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably make this edit yourself rather than relying on Github suggestion, since that will anger the mighty CLA-bot.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

container, and may only be visible to the system operator or platform provider.
In a highly-shared environment, containers may experience the following:
The general OCI state might not be available for introspection within the
container, and might only be visible to the system operator or platform provider.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think MAY might be correct here, but I think @dgerd has thought about this more recently than I.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually prefer removing this as it:

  1. Seems duplicative with https://github.com/knative/serving/blob/master/docs/runtime-contract.md#operations
  2. Provides little guidance/value to users or platform providers

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I thought it was non-normative anyway, I'm ok with removing it - done!

@@ -394,15 +394,15 @@ invalid, the container execution MUST be failed.

### Default Filesystems

The OCI specification describes a default container environment which may be
The OCI specification describes a default container environment which might be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The OCI specification describes a default container environment which might be
The OCI specification describes a default container environment which can be

I think "can" reads more clearly here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

recycled), so log aggregation SHOULD be provided.

In addition to the filesystems recommended in the OCI, the following filesystems
In addition to the filesystems suggested in the OCI, the following filesystems
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is RECOMMENDED if it's coming from OCI.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OCI might recommend them, but I'm not sure the use of the word here is normative since we're not actually adding a new constraint we're just saying "they tell us to do this". But let me think about it....

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be great to add the OCI recommended filesystems inline with a link to where they come from. No one is going to click out to find what are the recommended filesystems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

platform security hardening, operators MAY tune this over time as the threat
This option SHOULD only be set by the operator or platform provider, and MUST
NOT be configurable by the developer. As mount propagation might be part of the
platform security hardening, operators might tune this over time as the threat
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If 474 is MAY, this should be MAY as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I would make both of these MAY

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we really adding constraints here or just stating how things are? It didn't come across to me like we were being normative. For example, the phrase "person X might do something over time" comes across to me more like "guidance" or explaining what's going on in the system rather than saying "make sure person X is given the option of doing ....". If we really are saying "make sure the operation is given the option of doing this" then I think we need to word it more strongly so it's not so ad hoc-ish.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to explicitly word it so that these tools are available for operators.

Would "Operators MAY disable or force this setting to a particular value" be better?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see if the new text is ok

security hardening, operators may tune this from time to time as the threat
This option SHOULD only be set by the operator or platform provider, and MUST NOT
be configurable by the developer. As masked paths might be part of the platform
security hardening, operators might tune this from time to time as the threat
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Author

@duglin duglin Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same concern as above - is this guidance/explanatory text or constraints?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added new text to align with the other 2 things like this. SWYT

docs/runtime-contract.md Show resolved Hide resolved
container, and may only be visible to the system operator or platform provider.
In a highly-shared environment, containers may experience the following:
The general OCI state might not be available for introspection within the
container, and might only be visible to the system operator or platform provider.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually prefer removing this as it:

  1. Seems duplicative with https://github.com/knative/serving/blob/master/docs/runtime-contract.md#operations
  2. Provides little guidance/value to users or platform providers

@@ -126,9 +126,9 @@ the OCI specification as long as:
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`
- A container SHOULD write its own termination message to `/dev/termination-log`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this 'ought'.

Personally I would like to see us remove this in a future change. See discussion here: https://github.com/knative/serving/pull/4035/files#r283528430

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep - we have no say over this. Fixed.

by default. If no message is written by the container, the last few lines of
log output should be reported as the execution error (i.e. by
log output SHOULD be reported as the execution error (i.e. by
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do this today and I don't think we plan to do this. K8s exposes this on the Pod which is not part of the Knative API and is difficult to aggregate up to the Revision.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, who is supposed to "report this error" ? Knative? the OCI runtime?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have read it in the past as Knative. A Knative user does not have interaction with the OCI runtime. A user is able to see what is visible through our API, monitoring/logging extension points, anything observable within the container, and anything observable about the lifecycle of the container.

If this is only visible in the OCI API then it is not visible to the end user and is thus could be omitted from the runtime without consequence to user experience.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the discussion in #4035 -- it turns out that this information is collected at the Pod level, but is not aggregated beyond that level.

Having looked into it further, I'm concerned about the cascading update scenario that aggregating this to status information in the apiserver above the Pod level enables. Consider 1000 containers which crash in a coordinated manner -- that's a lot of extra updates (and possibly conflicts which need to be retried) that put load on the apiserver at exactly the wrong time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the discussion in #4035 is leaning towards removing this text altogether -- there's no place in the API for these errors to be surfaced beyond the Pod level, and Pods are really ephemeral, so the termination message isn't so helpful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to limit the semantic changes made to this PR but I'll remove this text anyway since you guys are convinced it's wrong. I removed the entire bullet since if I remove the last sentence then there's no normative text left - so there's no point in keeping the rest of it.

@@ -162,7 +162,7 @@ A read from the `stdin` file descriptor on the container SHOULD always result in
collected and retained in a developer-accessible logging repository.
(TODO:[docs#902](https://github.com/knative/docs/issues/902)).

Within the container, pipes and file descriptors may be used to communicate
Within the container, pipes and file descriptors MAY be used to communicate
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MAY keyword doesn't seem appropriate here. If a container MAY be able to do this it implies that the platform MUST allow pipes and file descriptors for IPC. Using MUST and phrasing from the platform point-of-view seems like the more appropriate wording, but the statement seems like overkill.

I cannot think of a container runtime that doesn't allow this and I cannot think of a good reason for a future runtime to disable this.

@evankanderson Any thoughts on why this is here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing it to a "can" since we have no say over what happens within the container w.r.t. these pipes - IOW, I think this statement is out of scope of Knative.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to change to "can" I would prefer removal of it. Keeping this document concise keeps it readable and usable. I don't think it clarifies much for platform providers or users as a "can" statement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was added at one point due to an internal discussion about required gvisor capabilities. In any case, it was a point-in-time thing, and I agree with removing it, unless we're going to spell out a complete set of linux syscalls which are in-scope.

@@ -411,13 +411,13 @@ MUST be provided:
| `/var/log` | MUST be a directory with write permissions for logs storage. Implementations MAY permit the creation of additional subdirectories and log rotation and renaming. |
| `/dev/log` | MUST be a writable socket to syslog |

In addition, the following files may be overridden by the runtime environment to
In addition, the following files MAY be overridden by the runtime environment to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/etc/resolve.conf is listed as "SHOULD be set". It is somewhat confusing to have the MAY paragraph prefix this table.

I would opt to change the word 'may' in the leading paragraph and rely on the Description to specify the keywords.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit bothered too as I was tweaking this... I'll do that.

@@ -470,7 +470,7 @@ Seccomp provides a mechanism for further restricting the set of linux syscalls
permitted to the processes running inside the container environment. A seccomp
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
container specification. As the seccomp policy might be part of the platform
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not 'MAY'?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have read it wrong, but I didn't think the "may" here was our way of saying "you can do this if you want" but rather it was trying to explain that something this thing happens and we're not really making a statement as to whether it's ok or not. Was it intended to introduce a constraint or was it just explaining the state of the world?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to say that it is okay; sharing a kernel is sufficiently exciting from a security perspective that we should be clear that enabling defense mechanisms here is prudent (though not required -- something like Kata Containers or gvisor has different mechanisms for protection).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want MAY in there, then I think we need to remove the "As the..." part, because then it reads kind of weird as a normative statement. See what you think of the new text

This option should only be set by the operator or platform provider, and MUST
NOT be configurable by the developer. As mount propagation may be part of the
platform security hardening, operators MAY tune this over time as the threat
This option SHOULD only be set by the operator or platform provider, and MUST
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This option SHOULD only be set by the operator or platform provider, and MUST
This option MAY be set by the operator or platform provider, and MUST

The second part of the sentence clarifies the 'only' part so that is unnecessary. We don't provide any real guidance or requirement here so SHOULD seems to be too strong of a word choice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok done

platform security hardening, operators MAY tune this over time as the threat
This option SHOULD only be set by the operator or platform provider, and MUST
NOT be configurable by the developer. As mount propagation might be part of the
platform security hardening, operators might tune this over time as the threat
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I would make both of these MAY

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
This option SHOULD only be set by the operator or platform provider, and MUST NOT
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This option SHOULD only be set by the operator or platform provider, and MUST NOT
This option MAY be set by the operator or platform provider, and MUST NOT

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -211,9 +211,9 @@ following protocol will be used:
- `h2c`: HTTP/2 transport, as described in
[section 3.4 of the HTTP2 spec (Starting HTTP/2 with Prior Knowledge)](https://http2.github.io/http2-spec/#known-http)

Developers SHOULD prefer to use automatic content negotiation where available,
Developers SHOULD use automatic content negotiation where available,
and MUST NOT set the `name` field to arbitrary values, as additional transports
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgerd is the "MUST NOT" here overstepping too? Seems like it given we're removing the other keywords from this paragraph.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "MUST NOT" is important to allow us to reserve the 'name' field for future usage. It could be argued that this is more of an API constraint than a runtime constraint, but it is important and I am okay with duplicating it here for clarity.

For example, if HTTP/3.0 comes along and we want to allow people to specify "http3" we can only enable this we know that no existing Knative Configurations allow "http3"; otherwise this will be a breaking change.

I am okay rephrasing this if you think it is unclear as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "MUST NOT" suggests that validation during admission control is allowed here, and containers with other port names may be rejected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I'll leave it as-is.

@duglin
Copy link
Author

duglin commented Jul 2, 2019

@evankanderson @dgerd thanks for the review - couple of questions for ya...

@dgerd
Copy link

dgerd commented Jul 2, 2019

Took a pass through the comments. Need to take another pass through the diff.

Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're shrinking the document with edits, which seems like a good sign.

Thanks!

by default. If no message is written by the container, the last few lines of
log output should be reported as the execution error (i.e. by
log output SHOULD be reported as the execution error (i.e. by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the discussion in #4035 is leaning towards removing this text altogether -- there's no place in the API for these errors to be surfaced beyond the Pod level, and Pods are really ephemeral, so the termination message isn't so helpful.

@@ -211,9 +211,9 @@ following protocol will be used:
- `h2c`: HTTP/2 transport, as described in
[section 3.4 of the HTTP2 spec (Starting HTTP/2 with Prior Knowledge)](https://http2.github.io/http2-spec/#known-http)

Developers SHOULD prefer to use automatic content negotiation where available,
Developers SHOULD use automatic content negotiation where available,
and MUST NOT set the `name` field to arbitrary values, as additional transports
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "MUST NOT" suggests that validation during admission control is allowed here, and containers with other port names may be rejected.

and MUST NOT set the `name` field to arbitrary values, as additional transports
may be defined in the future. Developers MUST assume all traffic is
intermediated by an L7 proxy. Developers MUST NOT assume a direct network
might be defined in the future. Developers can assume all traffic is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we remove MUST here? I think #4035 moves these to a "advice to the developer" section, but the requirement here is intended to include:

  1. Don't assume that the other end of the TCP connection is the client
  2. Don't need to implement SSL in the container

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the MUST because we can't really control what the developers "assume". If we want to add something normative here then it needs to be a constraint on the Kn impl - so something like: Implementations MUST send all traffic through L7 proxies. Do you want that instead?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with this change.

This runtime contract is still pretty long and I don't expect developers to read through it. The language is meant for operators and contributors.

I would eventually like to move these suggestions to a shorter user-focused document. I intend to break this file out into its own PR ( https://github.com/knative/serving/pull/4035/files#diff-2b3ccedac0c1b327ad7d674942f6cf56 ).

`initialDelaySeconds` to a value greater than 0, and should aim to minimize
container startup time (aka cold start time).
In order to enable scaling in response to load, setting `initialDelaySeconds`
to a value greater than 0 can be used, while striving to minimize container
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this sentence now reads backwards -- it suggests that initialDelaySeconds > 0 may improve scaling response, which is almost certainly not true.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, yeah I got that backwards.... let me fix that

when on-disk resources are kept to a minimum. Additionally, developers may not
have access to the container filesystems (or the containers may be rapidly
when on-disk resources are kept to a minimum. Additionally, developers might not
have access to the container filesystems (or the containers might be rapidly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that this sentence doesn't make sense with "container filesystems". I think it was meant to say "durable storage" or "durable filesystems" (i.e. it would be fine to put all the container writes into tmpfs and throw them away when the container exits).

Copy link
Author

@duglin duglin Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reading this as trying to say that the developer might not be able to see the container's filesystem so they can't look at log files, etc... I think the issue might be that it should be "container's" not "container". WDYT?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the container -> container's change is sufficient here, but it could be updated further for clarity.

My understanding here is that: (1) Developers don't necessarily have access to the underlying host so you can't go looking at the stopped container contents locally. (2) Unlike docker which relies on a user to rm a stopped container or run prune, containers in this environment are likely to be cleaned up quickly after exiting.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the apostrophe

In addition, the following files may be overridden by the runtime environment to
enable DNS resolution:
In addition, the following constraints are specified for the overridden files
indicated:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rewrite removes the DNS rationale. What about:

"To enable DNS resolution, the following files might be overwritten at runtime:"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much better - done

@duglin
Copy link
Author

duglin commented Jul 2, 2019

next round of rfc edits are ready for review

@duglin duglin closed this Jul 2, 2019
@duglin duglin reopened this Jul 2, 2019
@duglin
Copy link
Author

duglin commented Jul 10, 2019

@dgerd @evankanderson any other comments on this one?

Copy link

@dgerd dgerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments. Overall looks really good. Thanks for the iteration on this one.

and MUST NOT set the `name` field to arbitrary values, as additional transports
may be defined in the future. Developers MUST assume all traffic is
intermediated by an L7 proxy. Developers MUST NOT assume a direct network
might be defined in the future. Developers can assume all traffic is
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with this change.

This runtime contract is still pretty long and I don't expect developers to read through it. The language is meant for operators and contributors.

I would eventually like to move these suggestions to a shorter user-focused document. I intend to break this file out into its own PR ( https://github.com/knative/serving/pull/4035/files#diff-2b3ccedac0c1b327ad7d674942f6cf56 ).

In order to enable scaling in response to load, developers SHOULD NOT set the
`initialDelaySeconds` to a value greater than 0, and should aim to minimize
container startup time (aka cold start time).
Setting `initialDelaySeconds` to a value of 0 will enable scaling in response
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the removal of SHOULD NOT. Two nits:

(1) While line 275 explains that 0 is the default this line seems to encourage explicitly setting to 0.
(2) This explains what happens when it is set to 0, but does not explain what happens when it goes past zero or why a developer should prefer to keep it unset/0.

Here is what I suggest:

Setting `initialDelaySeconds` to a value greater than 0 impacts container startup time (aka cold start time) as a container will not serve traffic until the probe succeeds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @dgerd but s/will not serve traffic until the probe succeeds/cannot receive traffic until the probe has started/, I think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

when on-disk resources are kept to a minimum. Additionally, developers may not
have access to the container filesystems (or the containers may be rapidly
when on-disk resources are kept to a minimum. Additionally, developers might not
have access to the container filesystems (or the containers might be rapidly
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the container -> container's change is sufficient here, but it could be updated further for clarity.

My understanding here is that: (1) Developers don't necessarily have access to the underlying host so you can't go looking at the stopped container contents locally. (2) Unlike docker which relies on a user to rm a stopped container or run prune, containers in this environment are likely to be cleaned up quickly after exiting.

container specification. As the seccomp policy may be part of the platform
security hardening, operators MAY tune this over time as the threat environment
changes.
container specification. Seccomp policy MAY be part of the platform
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
container specification. Seccomp policy MAY be part of the platform
container specification. A Seccomp policy MAY be part of the platform

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

security hardening, operators MAY tune this over time as the threat environment
changes.
container specification. Seccomp policy MAY be part of the platform
security configuration so that operators can tune this over time as the
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
security configuration so that operators can tune this over time as the
security configuration that operators can tune over time as the

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

environment changes.
This option MAY be set by the operator or platform provider, and MUST
NOT be configurable by the developer. Mount propagation MAY be part of the
platform security configuration so that operators can tune this over time
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
platform security configuration so that operators can tune this over time
platform security configuration that operators can tune over time

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

environment changes.
This option MAY be set by the operator or platform provider, and MUST NOT
be configurable by the developer. Masked paths MAY be part of the platform
security configuration so operators can tune this from time to time as the
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
security configuration so operators can tune this from time to time as the
security configuration that operators can tune over time as the

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

I'll leave @dgerd to give the final /lgtm.

In order to enable scaling in response to load, developers SHOULD NOT set the
`initialDelaySeconds` to a value greater than 0, and should aim to minimize
container startup time (aka cold start time).
Setting `initialDelaySeconds` to a value of 0 will enable scaling in response
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @dgerd but s/will not serve traffic until the probe succeeds/cannot receive traffic until the probe has started/, I think.

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: duglin, evankanderson

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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 12, 2019
Doug Davis added 3 commits July 12, 2019 13:29
Please check each change carefully. I tried to not change the semantics
meaning of anything, rather just wanted to make sure each use of an
RFC2119 keyword was actually used correctly - meaing it's "normative".
RFC2119 does not distinguish between lower-case and upper-case use of the
words, so "may" and "MAY" have the same meaning. However, in most cases
people use upper-case versions of the words so the 1) they stand-out, and
2) it's more obvious that the author used the RFC2119 keyword on purpose and
didn't just use it by mistake as part of the descriptive english text.

We should add an RFC2119 keyword checker to docs like this to catch
mis-uses of the keywords going forward (meaning using lower-case by mistake).

Signed-off-by: Doug Davis <[email protected]>
Signed-off-by: Doug Davis <[email protected]>
Signed-off-by: Doug Davis <[email protected]>
@duglin
Copy link
Author

duglin commented Jul 12, 2019

@dgerd @evankanderson I think I got all of 'em - PTAL

@evankanderson
Copy link
Member

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2019
@knative-prow-robot knative-prow-robot merged commit af5750f into knative:master Jul 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants