Skip to content

pkg/signal: remove DefaultStopSignal const, and un-export container.DefaultStopTimeout#42717

Merged
cpuguy83 merged 2 commits intomoby:masterfrom
thaJeztah:move_defaults
Aug 24, 2021
Merged

pkg/signal: remove DefaultStopSignal const, and un-export container.DefaultStopTimeout#42717
cpuguy83 merged 2 commits intomoby:masterfrom
thaJeztah:move_defaults

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Aug 6, 2021

This const was previously living in pkg/signal, but with that package being moved to its own module, it didn't make much sense to put docker's defaults in a generic module.

The const from the "signal" package is currenlty used both by the CLI and the daemon as a default value when creating containers. This put up some questions:

  • a. should the default be non-exported, and private to the container package? After all, it's a default (so should be used if NOT set).
  • b. should the client actually setting a default, or instead just omit the value, unless specified by the user? having the client set a default also means that the daemon cannot change the default value because the client (or older clients) will override it.
  • c. consider defaults from the client and defaults of the daemon to be separate things, and create a default const in the CLI.

This patch implements option "a" (option "b" will be done separately, as it involves the CLI code). This still leaves "c" open as an option, if the CLI wants to set its own default.

Unfortunately, this change means we'll have to drop the alias for the deprecated pkg/signal.DefaultStopSignal const, but a comment was left instead, which can assist consumers of the const to find why it's no longer there (a search showed the Docker CLI as the only consumer though).

@thaJeztah
Copy link
Copy Markdown
Member Author

@cpuguy83 @tianon @AkihiroSuda PTAL - let me know what you think (see the PR description - should remove the default from the client?)

(related PR moby/sys#77)

@tianon
Copy link
Copy Markdown
Member

tianon commented Aug 10, 2021

I like option b 😅

@thaJeztah
Copy link
Copy Markdown
Member Author

I like option b 😅

high-five ❤️ 😂

Ok, I can make changes in this PR, and move the default to a private const inside container/ (together with defaultStopTimeout )

It's not used outside of the package itself

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This const was previously living in pkg/signal, but with that package
being moved to its own module, it didn't make much sense to put docker's
defaults in a generic module.

The const from the "signal" package is currenlty used *both* by the CLI
and the daemon as a default value when creating containers. This put up
some questions:

a. should the default be non-exported, and private to the container
   package? After all, it's a _default_ (so should be used if _NOT_ set).
b. should the client actually setting a default, or instead just omit
   the value, unless specified by the user? having the client set a
   default also means that the daemon cannot change the default value
   because the client (or older clients) will override it.
c. consider defaults from the client and defaults of the daemon to be
   separate things, and create a default const in the CLI.

This patch implements option "a" (option "b" will be done separately,
as it involves the CLI code). This still leaves "c" open as an option,
if the CLI wants to set its own default.

Unfortunately, this change means we'll have to drop the alias for the
deprecated pkg/signal.DefaultStopSignal const, but a comment was left
instead, which can assist consumers of the const to find why it's no
longer there (a search showed the Docker CLI as the only consumer though).

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah changed the title api/types/container: add DefaultStopSignal const, and un-export container.DefaultStopTimeout pkg/signal: remove DefaultStopSignal const, and un-export container.DefaultStopTimeout Aug 11, 2021
// DefaultStopTimeout is the timeout (in seconds) for the shutdown call on a container
DefaultStopTimeout = 30
// defaultStopSignal is the default syscall signal used to stop a container.
defaultStopSignal = "SIGTERM"
Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Aug 11, 2021

Choose a reason for hiding this comment

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

Yes, it's the same as the _unix const now, but I decided to define it in both as it's unlikely going to change, and this keeps it together with the corresponding defaultStopTimeout (which does differ between Unix and Windows)

@thaJeztah
Copy link
Copy Markdown
Member Author

@tianon updated; also opened docker/cli#3245 to implement "b" in the cli

@thaJeztah
Copy link
Copy Markdown
Member Author

@tianon @cpuguy83 ptal 🤗

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 merged commit a44a8e5 into moby:master Aug 24, 2021
@thaJeztah thaJeztah deleted the move_defaults branch August 24, 2021 16:55
@thaJeztah thaJeztah added this to the 21.xx milestone Oct 15, 2021
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.

3 participants