Skip to content

agent: Report ContainerStatus as part of TaskStatus.#571

Merged
aluzzardi merged 1 commit intomoby:masterfrom
aluzzardi:report-container-status
May 16, 2016
Merged

agent: Report ContainerStatus as part of TaskStatus.#571
aluzzardi merged 1 commit intomoby:masterfrom
aluzzardi:report-container-status

Conversation

@aluzzardi
Copy link
Member

  • Alter the controller to report container-specific task status
  • Added Pid and ExitStatus to ContainerStatus
  • Fill ExposedPorts in ContainerStatus
  • Misc changes so ContainerStatus finds its way to the dispatcher
  • CLI integration

For the sake of brevity, port mappings are reported in the same condensed mode as the Docker CLI does (also, note the Pid):

$ swarmctl task inspect 4d1e84mq1n8ob4l625pdqypx1
ID                     : 4d1e84mq1n8ob4l625pdqypx1
Instance               : 1
Service                : redis
Status
  Desired State        : RUNNING
  Last State           : RUNNING
  Timestamp            : 2016-05-12T05:00:10.12956455Z
  Message              : started
  Pid                  : 23189
Ports                  : 0.0.0.0:32785->6379/tcp

/cc @aaronlehmann @stevvooe @mrjana @vieux @tonistiigi

@aluzzardi
Copy link
Member Author

@stevvooe: Note that per our conversation earlier, ExitCode is currently broken since there's no way for the controller to report anything else but an error.

@aluzzardi
Copy link
Member Author

@mrjana We're loosing the name information in PortConfig since there's no way to get that back from the Engine. Maybe we could do some optimistic name reverse-lookup from the spec, but meh.

@aluzzardi
Copy link
Member Author

@aaronlehmann Hit #570 while playing with this

@aluzzardi aluzzardi force-pushed the report-container-status branch from c2e4edc to 46844d3 Compare May 12, 2016 06:27
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a todo here to fix this interface.

@mrjana
Copy link
Contributor

mrjana commented May 12, 2016

We're loosing the name information in PortConfig since there's no way to get that back from the Engine. Maybe we could do some optimistic name reverse-lookup from the spec, but meh.

With agent becoming part of engine, this should be possible to be saved as part of the engine container state. So that way we don't need to do some kind of hacky reverse-lookup from the spec in the manager. But we need that so that we have all the data for any api user to obtain name->hostPort mapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could actually pre-allocate a slice by doing make([]*api.PortConfig, 0, len(portMap)) because we already know the size of the slice needed by this time, so that no reslicing happens when you are appending to the slice.

@aluzzardi aluzzardi force-pushed the report-container-status branch 4 times, most recently from 6d63ec6 to 960a37b Compare May 13, 2016 07:23
@aluzzardi
Copy link
Member Author

Rebased and added a few extras:

  • Make sure we always send the ContainerStatus as part as the TaskStatus if possible
  • Add ContainerID - which is kinda super useful when messing around (/cc @vieux)
  • ExitCode and Pid

- Alter the controller to report container-specific task status
- Added `Pid` and `ExitStatus` to `ContainerStatus`
- Fill `ExposedPorts` in `ContainerStatus`
- Misc changes so ContainerStatus finds its way to the dispatcher
- CLI integration

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>
@aluzzardi aluzzardi force-pushed the report-container-status branch from 960a37b to 5f5a2e3 Compare May 16, 2016 21:41
@aluzzardi
Copy link
Member Author

Rebased

ping @stevvooe

@stevvooe
Copy link
Contributor

LGTM

1 similar comment
@mrjana
Copy link
Contributor

mrjana commented May 16, 2016

LGTM

@aluzzardi aluzzardi merged commit 4339fb8 into moby:master May 16, 2016
@aluzzardi aluzzardi deleted the report-container-status branch May 16, 2016 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants