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

A previously running task on a node with state "down" should not be l… #3146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

…isted as running.

Signed-off-by: Christopher Wunder <wunder.christopher@web.de>
@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.

None yet

2 participants