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

Make container create, start, and stop events consistent #73892

Merged
merged 1 commit into from Feb 13, 2019

Conversation

Projects
None yet
3 participants
@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 10, 2019

The messages for container lifecycle events are subtly inconsistent and should be unified.

First, the field format for containers (spec.containers{installer}) is hard to parse for a human,
so include the container name directly in the message for create and start to be consistent with kill.

Second, the pulling image event has inconsistent capitalization, fix that to be sentence without punctuation.

Third, the kill container event was unnecessarily wordy and inconsistent with the create and start events. Make the following changes:

  • Use 'Stopping' instead of 'Killing' since kill is usually reserved for when we decide to hard stop a container
  • Send the event before we dispatch the prestop hook, since this is an "in-progress" style event vs a "already completed" type event
  • Remove the 'cri-o://' / 'docker://' prefix by printing the container name instead of id (we already do that replacement at the lower level to prevent high cardinality events)
  • Use 'message' instead of 'reason' as the argument name since this is a string for humans field, not a string for machines field
  • Remove the hash values on the container spec changed event because no human will ever be able to do anything with the hash value
  • Use 'Stopping container %s(, explanation)?' form without periods to follow event conventions

The end result is a more pleasant message for humans:

35m         Normal    Created                       Pod    Created container
35m         Normal    Started                       Pod    Started container
10m         Normal    Killing                       Pod    Killing container cri-o://installer:Need to kill Pod
10m         Normal    Pulling                       Pod    pulling image "registry.svc.ci.openshift.org/openshift/origin-v4.0-2019-02-10-172026@sha256:3da5303d4384d24691721c1cf2333584ba60e8f82c9e782f593623ce8f83ddc5"

becomes

35m         Normal    Created                       Pod    Created container installer
35m         Normal    Started                       Pod    Started container installer
10m         Normal    Killing                       Pod    Stopping container installer
10m         Normal    Pulling                       Pod    Pulling image "registry.svc.ci.openshift.org/openshift/origin-v4.0-2019-02-10-172026@sha256:3da5303d4384d24691721c1cf2333584ba60e8f82c9e782f593623ce8f83ddc5"

/kind bug
/kind cleanup

Events reported for container creation, start, and stop now report the container name in the message and are more consistently formatted.
Make container create, start, and stop events consistent
The messages for container lifecycle events are subtly inconsistent
and should be unified.

First, the field format for containers is hard to parse for a human,
so include the container name directly in the message for create
and start, and for kill remove the container runtime prefix.

Second, the pulling image event has inconsistent capitalization, fix
that to be sentence without punctuation.

Third, the kill container event was unnecessarily wordy and inconsistent
with the create and start events. Make the following changes:

* Use 'Stopping' instead of 'Killing' since kill is usually reserved for
  when we decide to hard stop a container
* Send the event before we dispatch the prestop hook, since this is an
  "in-progress" style event vs a "already completed" type event
* Remove the 'cri-o://' / 'docker://' prefix by printing the container
  name instead of id (we already do that replacement at the lower level
  to prevent high cardinality events)
* Use 'message' instead of 'reason' as the argument name since this is a
  string for humans field, not a string for machines field
* Remove the hash values on the container spec changed event because no
  human will ever be able to do anything with the hash value
* Use 'Stopping container %s(, explanation)?' form without periods to
  follow event conventions

The end result is a more pleasant message for humans:

```
35m         Normal    Created                       Pod    Created container
35m         Normal    Started                       Pod    Started container
10m         Normal    Killing                       Pod    Killing container cri-o://installer:Need to kill Pod
10m         Normal    Pulling                       Pod    pulling image "registry.svc.ci.openshift.org/openshift/origin-v4.0-2019-02-10-172026@sha256:3da5303d4384d24691721c1cf2333584ba60e8f82c9e782f593623ce8f83ddc5"
```

becomes

```
35m         Normal    Created                       Pod    Created container installer
35m         Normal    Started                       Pod    Started container installer
10m         Normal    Killing                       Pod    Stopping container installer
10m         Normal    Pulling                       Pod    Pulling image "registry.svc.ci.openshift.org/openshift/origin-v4.0-2019-02-10-172026@sha256:3da5303d4384d24691721c1cf2333584ba60e8f82c9e782f593623ce8f83ddc5"
```
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 10, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Feb 10, 2019

@kubernetes/sig-node-pr-reviews

@derekwaynecarr addressing some egregious inconsistencies among container lifecycle events - the container name in the message reads ok even in big lists (better than fieldPath: spec.containers{installer} which is a bit baroque), while pod name is under involvedObject in both new and old schema.

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Feb 10, 2019

I may also update the "Kind" column to be a calculated field including the name if set (namespace is usually irrelevant).

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Feb 10, 2019

/retest

1 similar comment
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Feb 11, 2019

/retest

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Feb 11, 2019

Changed the Kind column in #73894 to improve locating the object

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Feb 11, 2019

/retest

@feiskyer
Copy link
Member

feiskyer left a comment

It's looking much better. LGTM.

Also, add a release note?

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Feb 12, 2019

Updated release note

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 12, 2019

@smarterclayton: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e-containerized 49250c6 link /test pull-kubernetes-local-e2e-containerized

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@smarterclayton smarterclayton added the lgtm label Feb 13, 2019

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Feb 13, 2019

Applying tag based on lgtm

@k8s-ci-robot k8s-ci-robot merged commit 19e57c6 into kubernetes:master Feb 13, 2019

16 of 17 checks passed

pull-kubernetes-local-e2e-containerized Job failed.
Details
cla/linuxfoundation smarterclayton authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.