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.

Sign up for GitHub

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

Refactor the stack services command to be uniform #2131

Closed
wants to merge 1 commit into from
Closed

Refactor the stack services command to be uniform #2131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 10 additions & 24 deletions cli/command/stack/kubernetes/services.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"strings"

"github.com/docker/cli/cli/command/service"
"github.com/docker/cli/cli/command/stack/formatter"
"github.com/docker/cli/cli/command/stack/options"
"github.com/docker/compose-on-kubernetes/api/labels"
"github.com/docker/docker/api/types/filters"
Expand Down Expand Up @@ -79,60 +78,47 @@ func getResourcesForServiceList(dockerCli *KubeCli, filters filters.Args, labelS
return replicas, daemons, services, nil
}

// RunServices is the kubernetes implementation of docker stack services
func RunServices(dockerCli *KubeCli, opts options.Services) error {
// GetServices is the kubernetes implementation of listing stack services
func GetServices(dockerCli *KubeCli, opts options.Services) ([]swarm.Service, map[string]service.ListInfo, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at the extra return here; the service.ListInfo is basically generated from information that's in []swarm.Service, combined with information about the number of tasks running / desired. If I see it correctly, the service.ListInfo here is needed, because swarm.Service has no field for task status ("mode" is already part of the Service spec).

I just (finally) merged moby/moby#39231, which is a PR that adds exact those fields to the Service struct, which means we could just return ([]swarmService, error), and have the presentation code either convert it to a ListInfo or adjust that code to use []swarm.Service directly.

Copy link
Member

Choose a reason for hiding this comment

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

Let me prepare a vendor bump to bring in the changes from moby/moby#39231

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can wait for the vendor bump, thanks!

filters := opts.Filter.Value()
if err := filters.Validate(supportedServicesFilters); err != nil {
return err
return nil, nil, err
}
client, err := dockerCli.composeClient()
if err != nil {
return nil
return nil, nil, err
}
stacks, err := client.Stacks(false)
if err != nil {
return nil
return nil, nil, err
}
stackName := opts.Namespace
_, err = stacks.Get(stackName)
if apierrs.IsNotFound(err) {
return fmt.Errorf("nothing found in stack: %s", stackName)
return []swarm.Service{}, nil, nil
Copy link
Member

Choose a reason for hiding this comment

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

I was looking if we should return the error here, and have it handled by the caller 🤔. That way, this function (when used in different situations) would not hide this information.

I think that's ok for a follow-up, because we should have a function to convert Kubernetes API errors to errdef errors. In that case we would map (if possible) / standardise these errors, so that the non-k8s specific code understands them;

cli/vendor/k8s.io/apimachinery/pkg/api/errors/errors.go

Lines 456 to 459 in 0b6685b

// IsNotFound returns true if the specified error was created by NewNotFound.
func IsNotFound(err error) bool {
return ReasonForError(err) == metav1.StatusReasonNotFound
}

Similar to https://github.com/moby/moby/blob/81dbed4c8b87d99824eecea68c95f10056fd3d2a/errdefs/http_helpers.go#L172-L191

// statusCodeFromContainerdError returns status code for containerd errors when
// consumed directly (not through gRPC)
func statusCodeFromContainerdError(err error) int {
	switch {
	case containerderrors.IsInvalidArgument(err):
		return http.StatusBadRequest
	case containerderrors.IsNotFound(err):
		return http.StatusNotFound
	case containerderrors.IsAlreadyExists(err):
		return http.StatusConflict
	case containerderrors.IsFailedPrecondition(err):
		return http.StatusPreconditionFailed
	case containerderrors.IsUnavailable(err):
		return http.StatusServiceUnavailable
	case containerderrors.IsNotImplemented(err):
		return http.StatusNotImplemented
	default:
		return http.StatusInternalServerError
	}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I didn't return an error here is because the same code for swarm returns an empty slice when no services are found. We can do it one way or the other, we should just make sure that errors are returned in the same places for kubernetes and swarm.

}
if err != nil {
return err
return nil, nil, err
}

labelSelector := generateLabelSelector(filters, stackName)
replicasList, daemonsList, servicesList, err := getResourcesForServiceList(dockerCli, filters, labelSelector)
if err != nil {
return err
return nil, nil, err
}

// Convert Replicas sets and kubernetes services to swarm services and formatter information
services, info, err := convertToServices(replicasList, daemonsList, servicesList)
if err != nil {
return err
return nil, nil, err
}
services = filterServicesByName(services, filters.Get("name"), stackName)

if opts.Quiet {
info = map[string]service.ListInfo{}
}

format := opts.Format
if len(format) == 0 {
if len(dockerCli.ConfigFile().ServicesFormat) > 0 && !opts.Quiet {
format = dockerCli.ConfigFile().ServicesFormat
} else {
format = formatter.TableFormatKey
}
}

servicesCtx := formatter.Context{
Output: dockerCli.Out(),
Format: service.NewListFormat(format, opts.Quiet),
}
return service.ListFormatWrite(servicesCtx, services, info)
return services, info, nil
}

func filterServicesByName(services []swarm.Service, names []string, stackName string) []swarm.Service {
Expand Down
52 changes: 49 additions & 3 deletions cli/command/stack/services.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
package stack

import (
"fmt"

"github.com/docker/cli/cli"
"github.com/docker/cli/cli/command"
"github.com/docker/cli/cli/command/service"
"github.com/docker/cli/cli/command/stack/formatter"
"github.com/docker/cli/cli/command/stack/kubernetes"
"github.com/docker/cli/cli/command/stack/options"
"github.com/docker/cli/cli/command/stack/swarm"
cliopts "github.com/docker/cli/opts"
swarmtypes "github.com/docker/docker/api/types/swarm"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)
Expand Down Expand Up @@ -36,7 +41,48 @@ func newServicesCommand(dockerCli command.Cli, common *commonOptions) *cobra.Com

// RunServices performs a stack services against the specified orchestrator
func RunServices(dockerCli command.Cli, flags *pflag.FlagSet, commonOrchestrator command.Orchestrator, opts options.Services) error {
return runOrchestratedCommand(dockerCli, flags, commonOrchestrator,
func() error { return swarm.RunServices(dockerCli, opts) },
func(kli *kubernetes.KubeCli) error { return kubernetes.RunServices(kli, opts) })
services, info, err := GetServices(dockerCli, flags, commonOrchestrator, opts)
if err != nil {
return err
}
return formatWrite(dockerCli, services, opts, info)
}

// GetServices returns the services for the specified orchestrator
func GetServices(dockerCli command.Cli, flags *pflag.FlagSet, commonOrchestrator command.Orchestrator, opts options.Services) ([]swarmtypes.Service, map[string]service.ListInfo, error) {
switch {
case commonOrchestrator.HasAll():
return nil, nil, errUnsupportedAllOrchestrator
case commonOrchestrator.HasKubernetes():
kli, err := kubernetes.WrapCli(dockerCli, kubernetes.NewOptions(flags, commonOrchestrator))
if err != nil {
return nil, nil, err
}
return kubernetes.GetServices(kli, opts)
default:
return swarm.GetServices(dockerCli, opts)
}
}

func formatWrite(dockerCli command.Cli, services []swarmtypes.Service, opts options.Services, info map[string]service.ListInfo) error {
// if no services in the stack, print message and exit 0
if len(services) == 0 {
fmt.Fprintf(dockerCli.Err(), "Nothing found in stack: %s\n", opts.Namespace)
return nil
}

format := opts.Format
if len(format) == 0 {
if len(dockerCli.ConfigFile().ServicesFormat) > 0 && !opts.Quiet {
format = dockerCli.ConfigFile().ServicesFormat
} else {
format = formatter.TableFormatKey
}
}

servicesCtx := formatter.Context{
Output: dockerCli.Out(),
Format: service.NewListFormat(format, opts.Quiet),
}
return service.ListFormatWrite(servicesCtx, services, info)
}
35 changes: 9 additions & 26 deletions cli/command/stack/swarm/services.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,35 @@ package swarm

import (
"context"
"fmt"
"sort"

"github.com/docker/cli/cli/command"
"github.com/docker/cli/cli/command/service"
"github.com/docker/cli/cli/command/stack/formatter"
"github.com/docker/cli/cli/command/stack/options"
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/api/types/swarm"
"vbom.ml/util/sortorder"
)

// RunServices is the swarm implementation of docker stack services
func RunServices(dockerCli command.Cli, opts options.Services) error {
// GetServices is the swarm implementation of listing stack services
func GetServices(dockerCli command.Cli, opts options.Services) ([]swarm.Service, map[string]service.ListInfo, error) {
ctx := context.Background()
client := dockerCli.Client()

filter := getStackFilterFromOpt(opts.Namespace, opts.Filter)
services, err := client.ServiceList(ctx, types.ServiceListOptions{Filters: filter})
if err != nil {
return err
return nil, nil, err
}

// if no services in this stack, print message and exit 0
if len(services) == 0 {
fmt.Fprintf(dockerCli.Err(), "Nothing found in stack: %s\n", opts.Namespace)
return nil
return []swarm.Service{}, nil, nil
}

sort.Slice(services, func(i, j int) bool {
return sortorder.NaturalLess(services[i].Spec.Name, services[j].Spec.Name)
})

info := map[string]service.ListInfo{}
if !opts.Quiet {
taskFilter := filters.NewArgs()
Expand All @@ -43,29 +40,15 @@ func RunServices(dockerCli command.Cli, opts options.Services) error {

tasks, err := client.TaskList(ctx, types.TaskListOptions{Filters: taskFilter})
if err != nil {
return err
return nil, nil, err
}

nodes, err := client.NodeList(ctx, types.NodeListOptions{})
if err != nil {
return err
return nil, nil, err
}

info = service.GetServicesStatus(services, nodes, tasks)
}

format := opts.Format
if len(format) == 0 {
if len(dockerCli.ConfigFile().ServicesFormat) > 0 && !opts.Quiet {
format = dockerCli.ConfigFile().ServicesFormat
} else {
format = formatter.TableFormatKey
}
}

servicesCtx := formatter.Context{
Output: dockerCli.Out(),
Format: service.NewListFormat(format, opts.Quiet),
}
return service.ListFormatWrite(servicesCtx, services, info)
return services, info, nil
}