Improve service tasks grouping on printing by SaretX · Pull Request #2098 · docker/cli

SaretX · 2019-09-22T17:03:18Z

When printing services' tasks with docker service ps command, tasks are grouped only by task slot.
This leads to interleaving tasks from different services when docker service ps is called with multiple services.

Besides this, global services do not have slots at all and printing tasks for them doesn't group and
doesn't properly indent tasks with _.

With this patch all tasks are grouped by service ID, slot and node ID (relevant only for global services) and it fixes issue #533.

Signed-off-by: Andrii Berehuliak [email protected]

- What I did

Improved tasks grouping on docker service ps for multiple services and fixed issue with global services. Fixes #533

- How I did it

Changed printing func.

- How to verify it

Create multiple services (including at least one global service) on multiple nodes
Run docker service ps svc1 svc2 ...
See that tasks are properly sorted

- Description for the changelog

Improves tasks printing for docker services

thaJeztah · 2019-10-15T13:39:06Z

Thank you!

I gave this PR a spin with the following steps;

Create some services

docker service create --detach --name=a --replicas=4 nginx:alpine
docker service create --detach --name=b --replicas=3 nginx:alpine
docker service create --detach --name=c --mode=global nginx:alpine

Update the services, so that they have a "history"

docker service update --detach --env-add FOO=bar a
docker service update --detach --env-add FOO=bar b
docker service update --detach --env-add BAR=qux b
docker service update --detach --env-add FOO=bar c

Before this PR:

docker service ps a b c

ID                  NAME                              IMAGE               NODE                DESIRED STATE       CURRENT STATE                     ERROR               PORTS
xbzm6ed776yw        c.j1afavbqqhr21jvnid3nnfoyt       nginx:alpine        docker-desktop      Running             Running 5 seconds ago                                 
4mcsovp8ckwn         \_ c.j1afavbqqhr21jvnid3nnfoyt   nginx:alpine        docker-desktop      Shutdown            Shutdown 6 seconds ago                                
qpcgdsx1r21a        b.1                               nginx:alpine        docker-desktop      Running             Running 2 seconds ago                                 
kfjo1hly92l4        a.1                               nginx:alpine        docker-desktop      Running             Running 5 seconds ago                                 
pubrerosvsw5        b.1                               nginx:alpine        docker-desktop      Shutdown            Shutdown 3 seconds ago                                
fu08gfi8tfyv        a.1                               nginx:alpine        docker-desktop      Shutdown            Shutdown 7 seconds ago                                
pu6qmgyoibq4        b.2                               nginx:alpine        docker-desktop      Running             Ready 1 second ago                                    
tz1n4hjne6pk         \_ b.2                           nginx:alpine        docker-desktop      Shutdown            Shutdown less than a second ago                       
xq8dogqcbxd2        a.2                               nginx:alpine        docker-desktop      Running             Running 44 seconds ago                                
rm40lofzed0h        a.3                               nginx:alpine        docker-desktop      Running             Starting less than a second ago                       
sqqj2n9fpi82        b.3                               nginx:alpine        docker-desktop      Running             Running 5 seconds ago                                 
prv3gymkvqk6         \_ b.3                           nginx:alpine        docker-desktop      Shutdown            Shutdown 6 seconds ago                                
qn7c7jmjuo76        a.3                               nginx:alpine        docker-desktop      Shutdown            Shutdown less than a second ago                       
wi9330mbabpg        a.4                               nginx:alpine        docker-desktop      Running             Running 2 seconds ago                                 
p5oy6h7nkvc3         \_ a.4                           nginx:alpine        docker-desktop      Shutdown            Shutdown 3 seconds ago

After this PR:

docker service ps a b c

ID                  NAME                              IMAGE               NODE                DESIRED STATE       CURRENT STATE             ERROR               PORTS
xbzm6ed776yw        c.j1afavbqqhr21jvnid3nnfoyt       nginx:alpine        docker-desktop      Running             Running 32 seconds ago                        
4mcsovp8ckwn         \_ c.j1afavbqqhr21jvnid3nnfoyt   nginx:alpine        docker-desktop      Shutdown            Shutdown 33 seconds ago                       
kfjo1hly92l4        a.1                               nginx:alpine        docker-desktop      Running             Running 32 seconds ago                        
fu08gfi8tfyv         \_ a.1                           nginx:alpine        docker-desktop      Shutdown            Shutdown 34 seconds ago                       
3pam0limnn24        a.2                               nginx:alpine        docker-desktop      Running             Running 23 seconds ago                        
xq8dogqcbxd2         \_ a.2                           nginx:alpine        docker-desktop      Shutdown            Shutdown 24 seconds ago                       
rm40lofzed0h        a.3                               nginx:alpine        docker-desktop      Running             Running 26 seconds ago                        
qn7c7jmjuo76         \_ a.3                           nginx:alpine        docker-desktop      Shutdown            Shutdown 27 seconds ago                       
wi9330mbabpg        a.4                               nginx:alpine        docker-desktop      Running             Running 29 seconds ago                        
p5oy6h7nkvc3         \_ a.4                           nginx:alpine        docker-desktop      Shutdown            Shutdown 30 seconds ago                       
qpcgdsx1r21a        b.1                               nginx:alpine        docker-desktop      Running             Running 29 seconds ago                        
pubrerosvsw5         \_ b.1                           nginx:alpine        docker-desktop      Shutdown            Shutdown 30 seconds ago                       
pu6qmgyoibq4        b.2                               nginx:alpine        docker-desktop      Running             Running 26 seconds ago                        
tz1n4hjne6pk         \_ b.2                           nginx:alpine        docker-desktop      Shutdown            Shutdown 27 seconds ago                       
sqqj2n9fpi82        b.3                               nginx:alpine        docker-desktop      Running             Running 32 seconds ago                        
prv3gymkvqk6         \_ b.3                           nginx:alpine        docker-desktop      Shutdown            Shutdown 33 seconds ago

That definitely looks a lot better

Perhaps we should sort the tasks on their name (instead of service ID)? That way the sort order looks more natural (in the example above, service c would be printed last)

thaJeztah · 2019-10-15T13:39:29Z

Let me write up a quick patch with what I think would work 👍

thaJeztah · 2019-10-15T13:42:22Z

We should also consider keeping the "indent" (\_) separate from the name, so that users can create their own table --format without that part in it 🤔 (that's definitely something for a separate PR though)

thaJeztah · 2019-10-15T13:59:16Z

Ok, I think this should do the trick; I used a slightly different approach, and actually set the name of the task (which looks to be currently unused). Then, with the name being in the right format (ServiceName.<Slot|NodeName>), it looks like just sorting those using "natural sort" groups them all together in the correct order.

After that, we can detect if indentation is needed, and print it with the selected --format

Patch is below (shown collapsed), and I uploaded it as well;
suggestions.patch.txt, and pushed it to a branch on my fork (if that's more convenient); master...thaJeztah:group_service_tasks_suggestions

Details

diff --git a/cli/command/task/print.go b/cli/command/task/print.go
index 2c803b92..32a496bb 100644
--- a/cli/command/task/print.go
+++ b/cli/command/task/print.go
@@ -10,6 +10,7 @@ import (
 	"github.com/docker/cli/cli/command/idresolver"
 	"github.com/docker/cli/cli/config/configfile"
 	"github.com/docker/docker/api/types/swarm"
+	"vbom.ml/util/sortorder"
 )
 
 type tasksSortable []swarm.Task
@@ -23,23 +24,10 @@ func (t tasksSortable) Swap(i, j int) {
 }
 
 func (t tasksSortable) Less(i, j int) bool {
-	// Sort by service ID.
-	if t[i].ServiceID != t[j].ServiceID {
-		return t[i].ServiceID < t[j].ServiceID
+	if t[i].Name != t[j].Name {
+		return sortorder.NaturalLess(t[i].Name, t[j].Name)
 	}
-
-	// If same service, sort by slot.
-	if t[i].Slot != t[j].Slot {
-		return t[i].Slot < t[j].Slot
-	}
-
-	// If same service and slot, sort by node ID.
-	// This sorting is relevant only for global services.
-	if t[i].NodeID != t[j].NodeID {
-		return t[i].NodeID < t[j].NodeID
-	}
-
-	// If same service, slot and node - sort by most recent.
+	// Sort tasks for the same service and slot by most recent.
 	return t[j].Meta.CreatedAt.Before(t[i].CreatedAt)
 }
 
@@ -47,6 +35,14 @@ func (t tasksSortable) Less(i, j int) bool {
 // Besides this, command `docker node ps <node>`
 // and `docker stack ps` will call this, too.
 func Print(ctx context.Context, dockerCli command.Cli, tasks []swarm.Task, resolver *idresolver.IDResolver, trunc, quiet bool, format string) error {
+	tasks, err := generateTaskNames(ctx, tasks, resolver)
+	if err != nil {
+		return err
+	}
+
+	// First sort tasks, so that all tasks (including previous ones) of the same
+	// service and slot are together. This must be done first, to print "previous"
+	// tasks indented
 	sort.Stable(tasksSortable(tasks))
 
 	names := map[string]string{}
@@ -58,40 +54,55 @@ func Print(ctx context.Context, dockerCli command.Cli, tasks []swarm.Task, resol
 		Trunc:  trunc,
 	}
 
+	var indent string
+	if tasksCtx.Format.IsTable() {
+		indent = ` \_ `
+	}
 	prevName := ""
 	for _, task := range tasks {
-		serviceName, err := resolver.Resolve(ctx, swarm.Service{}, task.ServiceID)
-		if err != nil {
-			return err
+		if task.Name == prevName {
+			// Indent previous tasks of the same slot
+			names[task.ID] = indent
+		} else {
+			names[task.ID] = ""
 		}
+		prevName = task.Name
 
 		nodeValue, err := resolver.Resolve(ctx, swarm.Node{}, task.NodeID)
 		if err != nil {
 			return err
 		}
+		nodes[task.ID] = nodeValue
+	}
 
-		var name string
-		if task.Slot != 0 {
-			name = fmt.Sprintf("%v.%v", serviceName, task.Slot)
-		} else {
-			name = fmt.Sprintf("%v.%v", serviceName, task.NodeID)
-		}
+	return FormatWrite(tasksCtx, tasks, names, nodes)
+}
 
-		// Indent the name if necessary
-		indentedName := name
-		if name == prevName {
-			indentedName = fmt.Sprintf(" \\_ %s", indentedName)
+// generateTaskNames generates names for the given tasks, and returns a copy of
+// the slice with the 'Name' field set.
+//
+// Depending if the "--no-resolve" option is set, names have the following pattern:
+//
+// - ServiceName.Slot or ServiceID.Slot for tasks that are part of a replicated service
+// - ServiceName.NodeName or ServiceID.NodeID for tasks that are part of a global service
+//
+// Task-names are not unique in cases where "tasks" contains previous/rotated tasks.
+func generateTaskNames(ctx context.Context, tasks []swarm.Task, resolver *idresolver.IDResolver) ([]swarm.Task, error) {
+	// Use a copy of the tasks list, to not modify the original slice
+	t := append(tasks[:0:0], tasks...)
+
+	for i, task := range t {
+		serviceName, err := resolver.Resolve(ctx, swarm.Service{}, task.ServiceID)
+		if err != nil {
+			return nil, err
 		}
-		prevName = name
-
-		names[task.ID] = name
-		if tasksCtx.Format.IsTable() {
-			names[task.ID] = indentedName
+		if task.Slot != 0 {
+			t[i].Name = fmt.Sprintf("%v.%v", serviceName, task.Slot)
+		} else {
+			t[i].Name = fmt.Sprintf("%v.%v", serviceName, task.NodeID)
 		}
-		nodes[task.ID] = nodeValue
 	}
-
-	return FormatWrite(tasksCtx, tasks, names, nodes)
+	return t, nil
 }
 
 // DefaultFormat returns the default format from the config file, or table

SaretX · 2019-10-23T05:02:43Z

Hey @thaJeztah , thank you for your response. Sorry, this is my first pull request, so I don't really know what am I supposed to do now. Should I close/discard my pull request so that you can merge your code?

thaJeztah · 2019-11-05T01:09:36Z

@SaretX sorry for the delay; feel free to update your PR with my suggested changes (you can take them from the branch I pushed), but let me know if you need a hand doing so

When printing services' tasks with `docker service ps` command, tasks are grouped only by task slot. This leads to interleaving tasks from different services when `docker service ps` is called with multiple services. Besides this, global services do not have slots at all and printing tasks for them doesn't group and doesn't properly indent tasks with \_. With this patch all tasks are grouped by service ID, slot and node ID (relevant only for global services) and it fixes issue 533. Signed-off-by: Andrii Berehuliak <[email protected]>

When printing services' tasks with `docker service ps` command, tasks are grouped only by task slot. This leads to interleaving tasks from different services when `docker service ps` is called with multiple services. Besides this, global services do not have slots at all and printing tasks for them doesn't group and doesn't properly indent tasks with \_. With this patch all tasks are grouped by service ID, slot and node ID (relevant only for global services) and it fixes issue 533. Before this patch: ```console docker service ps a b c ID NAME IMAGE NODE DESIRED STATE CURRENT STATE ERROR PORTS xbzm6ed776yw c.j1afavbqqhr21jvnid3nnfoyt nginx:alpine docker-desktop Running Running 5 seconds ago 4mcsovp8ckwn \_ c.j1afavbqqhr21jvnid3nnfoyt nginx:alpine docker-desktop Shutdown Shutdown 6 seconds ago qpcgdsx1r21a b.1 nginx:alpine docker-desktop Running Running 2 seconds ago kfjo1hly92l4 a.1 nginx:alpine docker-desktop Running Running 5 seconds ago pubrerosvsw5 b.1 nginx:alpine docker-desktop Shutdown Shutdown 3 seconds ago fu08gfi8tfyv a.1 nginx:alpine docker-desktop Shutdown Shutdown 7 seconds ago pu6qmgyoibq4 b.2 nginx:alpine docker-desktop Running Ready 1 second ago tz1n4hjne6pk \_ b.2 nginx:alpine docker-desktop Shutdown Shutdown less than a second ago xq8dogqcbxd2 a.2 nginx:alpine docker-desktop Running Running 44 seconds ago rm40lofzed0h a.3 nginx:alpine docker-desktop Running Starting less than a second ago sqqj2n9fpi82 b.3 nginx:alpine docker-desktop Running Running 5 seconds ago prv3gymkvqk6 \_ b.3 nginx:alpine docker-desktop Shutdown Shutdown 6 seconds ago qn7c7jmjuo76 a.3 nginx:alpine docker-desktop Shutdown Shutdown less than a second ago wi9330mbabpg a.4 nginx:alpine docker-desktop Running Running 2 seconds ago p5oy6h7nkvc3 \_ a.4 nginx:alpine docker-desktop Shutdown Shutdown 3 seconds ago ``` After this patch: ```console docker service ps a b c ID NAME IMAGE NODE DESIRED STATE CURRENT STATE ERROR PORTS kfjo1hly92l4 a.1 nginx:alpine docker-desktop Running Running 32 seconds ago fu08gfi8tfyv \_ a.1 nginx:alpine docker-desktop Shutdown Shutdown 34 seconds ago 3pam0limnn24 a.2 nginx:alpine docker-desktop Running Running 23 seconds ago xq8dogqcbxd2 \_ a.2 nginx:alpine docker-desktop Shutdown Shutdown 24 seconds ago rm40lofzed0h a.3 nginx:alpine docker-desktop Running Running 26 seconds ago qn7c7jmjuo76 \_ a.3 nginx:alpine docker-desktop Shutdown Shutdown 27 seconds ago wi9330mbabpg a.4 nginx:alpine docker-desktop Running Running 29 seconds ago p5oy6h7nkvc3 \_ a.4 nginx:alpine docker-desktop Shutdown Shutdown 30 seconds ago qpcgdsx1r21a b.1 nginx:alpine docker-desktop Running Running 29 seconds ago pubrerosvsw5 \_ b.1 nginx:alpine docker-desktop Shutdown Shutdown 30 seconds ago pu6qmgyoibq4 b.2 nginx:alpine docker-desktop Running Running 26 seconds ago tz1n4hjne6pk \_ b.2 nginx:alpine docker-desktop Shutdown Shutdown 27 seconds ago sqqj2n9fpi82 b.3 nginx:alpine docker-desktop Running Running 32 seconds ago prv3gymkvqk6 \_ b.3 nginx:alpine docker-desktop Shutdown Shutdown 33 seconds ago xbzm6ed776yw c.j1afavbqqhr21jvnid3nnfoyt nginx:alpine docker-desktop Running Running 32 seconds ago 4mcsovp8ckwn \_ c.j1afavbqqhr21jvnid3nnfoyt nginx:alpine docker-desktop Shutdown Shutdown 33 seconds ago ``` Signed-off-by: Andrii Berehuliak <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2020-01-09T14:31:28Z

hm.. looks like tests are failing. I'll need to have a look at what's causing that;

=== FAIL: cli/command/task TestTaskPrintWithIndentation (0.00s)
    print_test.go:105: assertion failed: 
        --- expected
        +++ actual
        @@ -1,4 +1,4 @@
        -ID                  NAME                     IMAGE               NODE                DESIRED STATE       CURRENT STATE        ERROR               PORTS
        -id-foo              service-name-foo.1       myimage:mytag       node-name-bar       Ready               Failed 2 hours ago                       
        -id-bar               \_ service-name-foo.1   myimage:mytag       node-name-bar       Ready               Failed 2 hours ago                       
        +ID                  NAME                IMAGE               NODE                DESIRED STATE       CURRENT STATE        ERROR               PORTS
        +id-foo                                  myimage:mytag       node-name-bar       Ready               Failed 2 hours ago                       
        +id-bar               \_                 myimage:mytag       node-name-bar       Ready               Failed 2 hours ago

silvin-lubecki · 2020-01-16T14:17:14Z

-		return t[i].Slot < t[j].Slot
+func (t tasksSortable) Less(i, j int) bool {
+	if t[i].Name != t[j].Name {
+		return sortorder.NaturalLess(t[i].Name, t[j].Name)


Hello @SaretX, thank you for your contribution 👍
I think that as we update the sort behavior here, at one test should have been updated or added to reflect that change 🤔
Can you update your PR with that test? Please tell me if you need help for that 👍

thaJeztah · 2020-02-17T13:49:54Z

fixed the failing test and added a new test to test the sorting behavior; #2341

thaJeztah · 2020-02-17T15:36:35Z

Thanks @SaretX ! I carried your changes in #2341, which is now merged to master 👍

GordonTheTurtle added the status/0-triage label Sep 22, 2019

thaJeztah added impact/changelog kind/enhancement status/2-code-review and removed status/0-triage labels Oct 15, 2019

SaretX added 2 commits December 8, 2019 18:56

SaretX force-pushed the 533-group_service_tasks branch from 343a218 to 25cc7b7 Compare December 8, 2019 17:11

silvin-lubecki reviewed Jan 16, 2020

View reviewed changes

thaJeztah mentioned this pull request Feb 17, 2020

Improve service tasks grouping on printing #2341

Merged

silvin-lubecki closed this in #2341 Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve service tasks grouping on printing#2098

Improve service tasks grouping on printing#2098
SaretX wants to merge 2 commits into
docker:masterfrom
SaretX:533-group_service_tasks

SaretX commented Sep 22, 2019 •

edited by thaJeztah

Loading

Uh oh!

thaJeztah commented Oct 15, 2019

Uh oh!

thaJeztah commented Oct 15, 2019

Uh oh!

thaJeztah commented Oct 15, 2019

Uh oh!

thaJeztah commented Oct 15, 2019

Uh oh!

SaretX commented Oct 23, 2019

Uh oh!

thaJeztah commented Nov 5, 2019

Uh oh!

thaJeztah commented Jan 9, 2020

Uh oh!

silvin-lubecki Jan 16, 2020

Uh oh!

thaJeztah commented Feb 17, 2020

Uh oh!

thaJeztah commented Feb 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

SaretX commented Sep 22, 2019 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Oct 15, 2019

Uh oh!

thaJeztah commented Oct 15, 2019

Uh oh!

thaJeztah commented Oct 15, 2019

Uh oh!

thaJeztah commented Oct 15, 2019

Uh oh!

SaretX commented Oct 23, 2019

Uh oh!

thaJeztah commented Nov 5, 2019

Uh oh!

thaJeztah commented Jan 9, 2020

Uh oh!

silvin-lubecki Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 17, 2020

Uh oh!

thaJeztah commented Feb 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SaretX commented Sep 22, 2019 •

edited by thaJeztah

Loading