closes #35702 introduce « exec_die » event by ndeloof · Pull Request #35744 · moby/moby

ndeloof · 2017-12-08T09:29:03Z

Signed-off-by: Nicolas De Loof [email protected]

- What I did

Implemented a new event "exec_die" when containerd report exec termination

- How I did it

execConfig is already updated in daemon, just added event logging

- How to verify it

docker run -d ubuntu sleep 100
docker events (on another terminal)
docker exec $ID echo hello

events console will show "exec_die" event

- Description for the changelog

introduce new exec_die event on exec termination

- A picture of a cute animal (not mandatory but encouraged)

dnephin · 2017-12-08T15:24:21Z

Would be good to t.Fatal(err) here so we have more information about the failure.

dnephin · 2017-12-08T15:24:34Z

t.Fatal("timeout hit")

dnephin · 2017-12-08T15:25:31Z

This should probably check the other fields of the event as well, at least the event name

dnephin

LGTM

thaJeztah · 2017-12-09T11:04:25Z

Some lint issues;

16:11:50 integration/system/event_test.go:1::warning: file is not gofmted with -s (gofmt)
16:11:50 integration/system/event_test.go:1::warning: file is not goimported (goimports)

thaJeztah · 2017-12-09T11:06:05Z

Can you also squash your commits? Changes look small enough to be in a single commit

thaJeztah

I'd like to have a quick check with other maintainers tomorrow at the maintainers meeting, but overall this looks good 👍

thaJeztah · 2017-12-13T17:29:41Z

Generally, I think this PR looks good; I want to double check on this one with other maintainers. I think the other exec requests;

have exec_foo:<command and arguments> as event (this was actually a mistake, but we had to keep it for backward-compatibility, and still have to work on improving that)

do not have an execId property; I think it's useful, but wondering if we should see this as a separate feature, and also consider adding the attribute to other exec_xx events (so that events can be related to each other)

FWIW, there's some weird things still with the events endpoint (e.g. "custom attributes" and "actual attributes" are merged, and can "overlap"); something that should be looked at

nit "exec_id"

thaJeztah · 2017-12-14T19:13:11Z

Discussing in the maintainers meeting; can you add a second commit that adds the execId to the other exec events as well? (Where possible) Then we should be able to move this forward 👍

thaJeztah · 2017-12-29T14:24:53Z

Thanks @ndeloof; overall this LGTM, but I have a couple of small things;

Regarding execId vs exec_id (#35744 (comment)), I had a quick look at existing properties (excluding top-level ID, Status, and From because those are deprecated),
I found timeNano (camelcase) and Actor.Attributes.exitCode (also camelcase), so I think it should be camelcase.

However, it should likely be execID instead of execId

@ndeloof can you change;

execId to execID? (unless @cpuguy83 has objections 😅)
add a note to the API version history, and document:
- the new exec_die event type
- document that the exec_create and exec_start events now also get an additional Actor.Attributes.execID property

Also, we should document properties in the swagger file. Looks like so far we didn't do so, so I'm ok with doing that separately in a follow-up

(example event stream below)

Details

[
  {
    "status": "pull",
    "id": "nginx:alpine",
    "Type": "image",
    "Action": "pull",
    "Actor": {
      "ID": "nginx:alpine",
      "Attributes": {
        "maintainer": "NGINX Docker Maintainers <[email protected]>",
        "name": "nginx"
      }
    },
    "scope": "local",
    "time": 1514553990,
    "timeNano": 1514553990091358700
  },
  {
    "status": "create",
    "id": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
    "from": "nginx:alpine",
    "Type": "container",
    "Action": "create",
    "Actor": {
      "ID": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
      "Attributes": {
        "image": "nginx:alpine",
        "maintainer": "NGINX Docker Maintainers <[email protected]>",
        "name": "foo"
      }
    },
    "scope": "local",
    "time": 1514553990,
    "timeNano": 1514553990219607300
  },
  {
    "Type": "network",
    "Action": "connect",
    "Actor": {
      "ID": "1d402bbfdaf4adba234cd5c09e66127cc9066e05b383d80c5a8eb734521bbbcc",
      "Attributes": {
        "container": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
        "name": "bridge",
        "type": "bridge"
      }
    },
    "scope": "local",
    "time": 1514553990,
    "timeNano": 1514553990237711000
  },
  {
    "status": "start",
    "id": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
    "from": "nginx:alpine",
    "Type": "container",
    "Action": "start",
    "Actor": {
      "ID": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
      "Attributes": {
        "image": "nginx:alpine",
        "maintainer": "NGINX Docker Maintainers <[email protected]>",
        "name": "foo"
      }
    },
    "scope": "local",
    "time": 1514553990,
    "timeNano": 1514553990660905000
  },
  {
    "status": "exec_create: sh ",
    "id": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
    "from": "nginx:alpine",
    "Type": "container",
    "Action": "exec_create: sh ",
    "Actor": {
      "ID": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
      "Attributes": {
        "execId": "526c1b75e58f8d66344894ecbf6645e53382ec261e3022b95211f53f959f3138",
        "image": "nginx:alpine",
        "maintainer": "NGINX Docker Maintainers <[email protected]>",
        "name": "foo"
      }
    },
    "scope": "local",
    "time": 1514554002,
    "timeNano": 1514554002318149400
  },
  {
    "status": "exec_start: sh ",
    "id": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
    "from": "nginx:alpine",
    "Type": "container",
    "Action": "exec_start: sh ",
    "Actor": {
      "ID": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
      "Attributes": {
        "execId": "526c1b75e58f8d66344894ecbf6645e53382ec261e3022b95211f53f959f3138",
        "image": "nginx:alpine",
        "maintainer": "NGINX Docker Maintainers <[email protected]>",
        "name": "foo"
      }
    },
    "scope": "local",
    "time": 1514554002,
    "timeNano": 1514554002319235000
  },
  {
    "status": "exec_die",
    "id": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
    "from": "nginx:alpine",
    "Type": "container",
    "Action": "exec_die",
    "Actor": {
      "ID": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
      "Attributes": {
        "execId": "526c1b75e58f8d66344894ecbf6645e53382ec261e3022b95211f53f959f3138",
        "exitCode": "0",
        "image": "nginx:alpine",
        "maintainer": "NGINX Docker Maintainers <[email protected]>",
        "name": "foo"
      }
    },
    "scope": "local",
    "time": 1514554021,
    "timeNano": 1514554021038903600
  },
  {
    "status": "kill",
    "id": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
    "from": "nginx:alpine",
    "Type": "container",
    "Action": "kill",
    "Actor": {
      "ID": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
      "Attributes": {
        "image": "nginx:alpine",
        "maintainer": "NGINX Docker Maintainers <[email protected]>",
        "name": "foo",
        "signal": "9"
      }
    },
    "scope": "local",
    "time": 1514554062,
    "timeNano": 1514554062808309000
  },
  {
    "status": "die",
    "id": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
    "from": "nginx:alpine",
    "Type": "container",
    "Action": "die",
    "Actor": {
      "ID": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
      "Attributes": {
        "exitCode": "137",
        "image": "nginx:alpine",
        "maintainer": "NGINX Docker Maintainers <[email protected]>",
        "name": "foo"
      }
    },
    "scope": "local",
    "time": 1514554063,
    "timeNano": 1514554063068919300
  },
  {
    "Type": "network",
    "Action": "disconnect",
    "Actor": {
      "ID": "1d402bbfdaf4adba234cd5c09e66127cc9066e05b383d80c5a8eb734521bbbcc",
      "Attributes": {
        "container": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
        "name": "bridge",
        "type": "bridge"
      }
    },
    "scope": "local",
    "time": 1514554063,
    "timeNano": 1514554063337964000
  },
  {
    "status": "destroy",
    "id": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
    "from": "nginx:alpine",
    "Type": "container",
    "Action": "destroy",
    "Actor": {
      "ID": "9833812e5cd5abd48b4c55a10cb8276b1790cc142037971c534ba885b2307e05",
      "Attributes": {
        "image": "nginx:alpine",
        "maintainer": "NGINX Docker Maintainers <[email protected]>",
        "name": "foo"
      }
    },
    "scope": "local",
    "time": 1514554063,
    "timeNano": 1514554063457728000
  }
]

ndeloof · 2018-01-08T10:02:20Z

@thaJeztah udpated to use "execID" as requested.

thaJeztah · 2018-01-08T10:19:10Z

Can you update the https://github.com/moby/moby/blob/master/docs/api/version-history.md as well? Then I think we'd be ready to merge 😅

thaJeztah · 2018-01-08T10:20:02Z

oh, and squash commits where needed

Signed-off-by: Nicolas De Loof <[email protected]>

vdemeester

LGTM 🐮

thaJeztah

LGTM

thaJeztah · 2018-01-10T19:45:10Z

Argh it's doing this again;

15:28:41 The swagger spec at "api/swagger.yaml" is valid against swagger specification 2.0
15:28:45 The result of hack/generate-swagger-api.sh differs
15:28:45 
15:28:45  M api/types/container/container_wait.go
15:28:45 
15:28:45 Please update api/swagger.yaml with any api changes, then

Let me try to add a diff to that output (because we had the same problem last time)

mdlinville · 2018-01-26T23:16:06Z

@vdemeester Please add an example of this to https://github.com/docker/cli/blob/master/docs/reference/commandline/events.md so that the docs will be updated. That will need to be cherry-picked into docker/docker-ce. @thaJeztah CC

GordonTheTurtle added the status/0-triage label Dec 8, 2017

ndeloof mentioned this pull request Dec 8, 2017

docker event for exec_die #35702

Closed

ndeloof force-pushed the 35702 branch from ea75ccc to 63c16cf Compare December 8, 2017 09:34

ndeloof requested review from dnephin and vdemeester as code owners December 8, 2017 12:55

cpuguy83 added rebuild/experimental status/2-code-review and removed status/0-triage labels Dec 8, 2017

GordonTheTurtle removed rebuild/experimental labels Dec 8, 2017

dnephin reviewed Dec 8, 2017

View reviewed changes

dnephin approved these changes Dec 8, 2017

View reviewed changes

thaJeztah added impact/api impact/changelog labels Dec 9, 2017

ndeloof force-pushed the 35702 branch 2 times, most recently from 4a692ea to cc13589 Compare December 9, 2017 13:32

thaJeztah requested changes Dec 13, 2017

View reviewed changes

ndeloof force-pushed the 35702 branch from 4cf15d9 to 1aa84db Compare December 14, 2017 23:12

GordonTheTurtle assigned thaJeztah Dec 23, 2017

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jan 8, 2018

ndeloof force-pushed the 35702 branch from 30cacf5 to ee16d1f Compare January 8, 2018 10:00

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jan 8, 2018

ndeloof force-pushed the 35702 branch from ee16d1f to 4df25be Compare January 8, 2018 10:37

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jan 8, 2018

ndeloof force-pushed the 35702 branch from 4df25be to 983f9ed Compare January 8, 2018 10:40

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jan 8, 2018

introduce « exec_die » event

aa6bb5c

Signed-off-by: Nicolas De Loof <[email protected]>

ndeloof force-pushed the 35702 branch from 983f9ed to aa6bb5c Compare January 8, 2018 10:42

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jan 8, 2018

moby deleted a comment from GordonTheTurtle Jan 10, 2018

vdemeester approved these changes Jan 10, 2018

View reviewed changes

thaJeztah added the rebuild/* label Jan 10, 2018

GordonTheTurtle removed the rebuild/* label Jan 10, 2018

thaJeztah approved these changes Jan 10, 2018

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Jan 10, 2018

vdemeester added rebuild/janky labels Jan 19, 2018

GordonTheTurtle removed rebuild/experimental labels Jan 19, 2018

vdemeester merged commit f97256c into moby:master Jan 19, 2018

gbarr01 mentioned this pull request Jan 27, 2018

Document update to API version 1.36 docker/docs#5845

Closed

This was referenced Apr 28, 2018

Missing exec_die event in event docs docker/docs#6579

Closed

Add exec_die to the list of known container events docker/docs#6580

Closed

gbarr01 mentioned this pull request Apr 28, 2018

Add exec_die to events.md docker/cli#1027

Closed

nstapelbroek mentioned this pull request Apr 29, 2018

Add exec_die to the list of known container events docker/cli#1028

Merged

phroggster mentioned this pull request Feb 15, 2019

Unsupported Event - container exec_die portainer/portainer#2717

Closed

Conversation

ndeloof commented Dec 8, 2017

Uh oh!

dnephin Dec 8, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 8, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 8, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 9, 2017

Uh oh!

thaJeztah commented Dec 9, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Dec 14, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 14, 2017

Uh oh!

thaJeztah commented Dec 29, 2017

Uh oh!

ndeloof commented Jan 8, 2018

Uh oh!

thaJeztah commented Jan 8, 2018

Uh oh!

thaJeztah commented Jan 8, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 10, 2018

Uh oh!

mdlinville commented Jan 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants