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

Allow created containers to be killed #875

Merged
merged 1 commit into from
Jun 23, 2017

Conversation

mrunalp
Copy link
Contributor

@mrunalp mrunalp commented Jun 23, 2017

Signed-off-by: Mrunal Patel [email protected]

@crosbymichael
Copy link
Member

crosbymichael commented Jun 23, 2017

LGTM

Approved with PullApprove

runtime.md Outdated
@@ -123,7 +123,7 @@ This operation MUST generate an error if `process` was not set.
`kill <container-id> <signal>`

This operation MUST [generate an error](#errors) if it is not provided the container ID.
Attempting to send a signal to a container that is not running MUST have no effect on the container and MUST [generate an error](#errors).
Attempting to send a signal to a container that is not running or not created MUST have no effect on the container and MUST [generate an error](#errors).
Copy link
Contributor

@wking wking Jun 23, 2017

Choose a reason for hiding this comment

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

I'd rather tie this more clearly to the already-well-defined state values. Something like:

Attempting to send a signal to a container that is neither [`created` nor `running`](#state) MUST have no effect…

That's pretty close to what you're suggesting, although it:

  • Uses “neither”/“nor” instead of repeating “not”.
  • Links to the “State” section for formal definitions of created and running states.
  • Uses backticks to strengthen the “these are formally defined terms” semantics of running and created.
  • Lists created before running, since any given container that ends up in running will always have passed through created earlier in its lifecycle, and no container can return to created after it has entered running.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGMT with @wking's suggestion,

@tianon
Copy link
Member

tianon commented Jun 23, 2017

What's the use-case for killing a container that isn't running? What's the expected behavior?

@wking
Copy link
Contributor

wking commented Jun 23, 2017

What's the use-case for killing a container that isn't running?

Getting ready to delete a container that you hadn't started. More on containers not getting started in #701.

What's the expected behavior?

The signal gets sent to the container process, same as in the running case. The container process will still be running a runtime-supplied program, but that doesn't effect sending signals to it.

@wking
Copy link
Contributor

wking commented Jun 23, 2017

While we're cleaning up the relationship between operations and container state, I think we can replace these two start lines with:

Attempting to `start` a container that is not [`created`](#state) MUST have no effect on the container and MUST [generate an error](#errors).

And these two delete lines with:

Attempting to `delete` a container that is not [`stopped`](#state) MUST have no effect on the container and MUST [generate an error](#errors).

@mrunalp
Copy link
Contributor Author

mrunalp commented Jun 23, 2017

Updated.

@tianon
Copy link
Member

tianon commented Jun 23, 2017

LGTM

Approved with PullApprove

@mrunalp
Copy link
Contributor Author

mrunalp commented Jun 23, 2017

@tianon Orchestrator could have a sequence where it is first creating a bunch of containers and then starting them. If there is a failure in the start sequence it could just kill the remaining created containers.

@crosbymichael
Copy link
Member

crosbymichael commented Jun 23, 2017

LGTM

Approved with PullApprove

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.

5 participants