Skip to content

Conversation

@devchris123
Copy link

The docker service ls command on a manager would report previously running tasks on a node that went down as running.
I updated the ListServiceStatuses function not to include these tasks in the output for running tasks
Fixes #45922

- What I did
I changed the listing of service statuses.

- How I did it
I checked if the node the service task is running on is down, and only if it's not, I upped the counter for running tasks.

- How to test it
Use the description of the related issue or run the test ListServiceStatuses in services_test.go

- Description for the changelog
Exclude tasks on non-responding nodes from the service status ouput.

@devchris123
Copy link
Author

Another possible (maybe more elegant) solution that occurs to me is that the orchestrator sets the current task for such nodes to "TaskStateOrphaned". Although I'm not sure how this works together with the desired state of a shutdown.

@dperny
Copy link
Collaborator

dperny commented Aug 2, 2023

I'm not sure this is the best location to make this change. The Client can easily separate out tasks on Down nodes by looking at the Desired State of those tasks. Tasks with Down nodes should report a desired state of Shutdown.

The actual state of the Task is Running because that's the last state the manager observed the task in. It's actually important to include such tasks, because, for example, a node may have lost its connection to the manager but not to other nodes, and the Task may actually still be running.

While the ListServiceStatuses function is intended as a shortcut, this may be a bit too much shortcut for something that can be settled out client side.

@devchris123
Copy link
Author

devchris123 commented Aug 13, 2023

I think you are right about settling this on the client side. It appears, that the status logic has been pushed to the daemon a while ago.

The code for setting the status is still there for compatibility reasons and it did indeed not include running tasks from shutdown nodes. Do you think it might be better to change this "back" or change this one status detail on the fly by inspecting the nodes of all involved tasks?

You can find it right here: https://github.com/docker/cli/blob/cdabfa2aa55a3560d0fe5f63074571afae840ab3/cli/command/service/list.go in line 167.

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.

2 participants