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

Refactor the stack services command to be uniform #2131

Closed
wants to merge 1 commit into from

Conversation

rumpl
Copy link
Member

@rumpl rumpl commented Oct 10, 2019

- What I did

Running docker stack services <STACK> --orchestrator swarm would yield the message "Noting found in stack: asdf" with an exit code 0. The same command with kubernetes orchestrator would yield "nothing found in stack: adsf" (note the lower-case "nothing") and a non-zero exit code. This change makes the stack services command uniform for both orchestrators. The logic of getting and printing services is split to reuse the same formatting code.

- How to verify it

Run:

$ docker stack services unknown --orchestrator swarm
$ docker stack services unknown --orchestrator kubernetes

Both commands should return 0 and show the error message Noting found in stack: unknown.

- Description for the changelog
Stack services will return the same exit code for all orchestrators.

- A picture of a cute animal (not mandatory but encouraged)
image

@rumpl
Copy link
Member Author

rumpl commented Oct 10, 2019

Force pushed to add comments on exported functions

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @rumpl 👍

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

I left some comments; one nit, and one (slightly) bigger change.

Code looks good in its current form, but (if it's not super-urgent to have this merged right away), I'd like to see if we can incorporate the changes from moby/moby#39231, so that we don't have to change the signature of these functions (twice)

// 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!

}
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;

// 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.

cli/command/stack/services.go Outdated Show resolved Hide resolved
cli/command/stack/services.go Outdated Show resolved Hide resolved
@rumpl rumpl force-pushed the refactor-stack-services branch from f7b6ff3 to ab837de Compare October 16, 2019 09:17
@rumpl
Copy link
Member Author

rumpl commented Oct 16, 2019

Rebased with master and changed the imports of swarmtypes to be consistent with the rest of the code.

Running `docker stack services <STACK> --orchestrator swarm would yield
the message "Noting found in stack: asdf" with an exit code 0. The same
command with kubernetes orchestrator would yield "nothing found in
stack: adsf" (note the lower-case "nothing") and a non-zero exit code.
This change makes the `stack services` command uniform for both
orchestrators. The logic of getting and printing services is split to
reuse the same formatting code.

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
@thaJeztah
Copy link
Member

carrying in #2167 - which is now ready for review 👍

@rumpl
Copy link
Member Author

rumpl commented Oct 29, 2019

👍 thanks a bunch!

@rumpl rumpl closed this Oct 29, 2019
@rumpl rumpl deleted the refactor-stack-services branch October 29, 2019 16:07
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