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

runtime: Remove "features the runtime chooses to support" #732

Merged

Conversation

wking
Copy link
Contributor

@wking wking commented Mar 16, 2017

Step 3 of the lifecycle from before this commit had two sentences which both landed in #384. I pushed back a bit on the entry then, but we seem to be pretty comfortable with the current "keep all lifecyle entries in a one-layer enumerated list" approach, so I'm leaving that alone in this commit. Step 3 isn't really a lifecycle step though, it's more about clarifying that you can jump around in the lifecycle instead of hitting all the steps in consecutive order. In this commit, I've addressed that in a new paragraph that follows the list.

The other sentence from the old step 3 doesn't need replacing, because the limits are already covered in more detail in the operation sections themselves. For example, the 'delete' operation has:

Attempting to delete a container that does not exist MUST generate an error. Attempting to delete a container whose process is still running MUST generate an error.

I don't see the need to call generic attention to that idea, and especially do not think that an entry in the lifecycle list is the right place for such a generic call-out.

4. The [prestart hooks](config.md#prestart) MUST be invoked by the runtime.
If any prestart hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 9.
5. The runtime MUST run the user-specified program, as specified by [`process`](config.md#process).
6. The [poststart hooks](config.md#poststart) MUST be invoked by the runtime.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides the removal of the old step 3, the rest of this hunk is just renumbering. Shown more clearly on the command line:

$ git show --word-diff origin/pr/732
…
3.[-Once the container is created additional actions MAY be performed based on the features the runtime chooses to support.-]
[-   However, some actions might only be available based on the current state of the container (e.g. only available while it is started).-]
[-4.-] Runtime's [`start`](runtime.md#start) command is invoked with the unique identifier of the container.
[-5.-]{+4.+} The [prestart hooks](config.md#prestart) MUST be invoked by the runtime.
   If any prestart hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step [-10.-]
[-6.-]{+9.+}
{+5.+} The runtime MUST run the user-specified program, as specified by [`process`](config.md#process).
[-7.-]{+6.+} The [poststart hooks](config.md#poststart) MUST be invoked by the runtime.
   If any poststart hook fails, the runtime MUST [log a warning](#warnings), but the remaining hooks and lifecycle continue as if the hook had succeeded.
[-8.-]{+7.+} The container process exits.
   This MAY happen due to erroring out, exiting, crashing or the runtime's [`kill`](runtime.md#kill) operation being invoked.
[-9.-]{+8.+} Runtime's [`delete`](runtime.md#delete) command is invoked with the unique identifier of the container.
[-10.-]{+9.+} The container MUST be destroyed by undoing the steps performed during create phase (step 2).
[-11.-]{+10.+} The [poststop hooks](config.md#poststop) MUST be invoked by the runtime.
    If any poststop hook fails, the runtime MUST [log a warning](#warnings), but the remaining hooks and lifecycle continue as if the hook had succeeded.
…

Copy link
Member

Choose a reason for hiding this comment

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

IMO GitHub's "rendered" version makes it even more clear:

image

Step 3 of the lifecycle from before this commit had two sentences
which both landed in be59415 (Split create and start, 2016-04-01,
opencontainers#384).  I pushed back a bit on the entry then [1,2], but we seem to be
pretty comfortable with the current "keep all lifecyle entries in a
one-layer enumerated list" approach, so I'm leaving that alone in this
commit.  Step 3 isn't really a lifecycle step though, it's more about
clarifying that you can jump around in the lifecycle instead of
hitting all the steps in consecutive order.  I'd floated a new
paragraph addressing that jumping, but was unable to form a consensus
around wording, and the jumping is already somewhat covered by the
current list entries (e.g. "The container process exits.").  This
commit just drops the old step 3, and Michael will follow up with
wording about jumping [3].

The other sentence from the old step 3 doesn't need replacing, because
the limits are already covered in more detail in the operation
sections themselves.  For example, the 'delete' operation has:

  Attempting to delete a container that does not exist MUST generate
  an error.  Attempting to delete a container whose process is still
  running MUST generate an error.

I don't see the need to call generic attention to that idea, and
especially do not think that an entry in the lifecycle list is the
right place for such a generic call-out.

[1]: opencontainers#384 (comment)
[2]: opencontainers#384 (comment)
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-05-10-21.03.log.html#l-79

Signed-off-by: W. Trevor King <[email protected]>
@wking wking force-pushed the drop-additional-actions-step branch from 78e15a9 to ccbc57f Compare May 10, 2017 22:47
@wking
Copy link
Contributor Author

wking commented May 10, 2017

@tianon
Copy link
Member

tianon commented May 10, 2017

LGTM

Approved with PullApprove

1 similar comment
@crosbymichael
Copy link
Member

crosbymichael commented May 10, 2017

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 8601fd7 into opencontainers:master May 10, 2017
@wking wking deleted the drop-additional-actions-step branch May 10, 2017 23:51
@vbatts vbatts mentioned this pull request Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants