Add OomScoreAdj to "docker service create" and "docker stack" by psaintlaurent · Pull Request #5145

Add OomScoreAdj to "docker service create" and "docker stack" by psaintlaurent · Pull Request #5145 · docker/cli

psaintlaurent changed the title ~~Add OomScoreAdj to "docker service create"~~ Add OomScoreAdj to "docker service create" and "docker compose"

Jun 12, 2024

Choose a reason for hiding this comment

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

Schema 3.13 was added in #5125, which has not been released yet, and will be for the v27.0 release, so we don't need to add a new schema and can update 3.13 instead.

This was referenced

Jun 18, 2024

		Binds: binds,
		ContainerIDFile: copts.containerIDFile,
		OomScoreAdj: copts.oomScoreAdj,
		OomScoreAdj: int(copts.oomScoreAdj),

Choose a reason for hiding this comment

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

Both sides should already be an int I think, so this change should probably be removed.

Comment on lines +519 to +536


		type OomScoreAdj int64

		func (o *OomScoreAdj) Type() string { return "int64" }

		func (o OomScoreAdj) Value() int64 { return int64(o) }

		func (o *OomScoreAdj) String() string {

		val := strconv.FormatInt(int64(*o), 10)
		return val
		}

		func (o *OomScoreAdj) Set(value string) error {

		var conv int64
		conv, err := strconv.ParseInt(value, 10, 64)
		if err != nil \|\| conv < -1000 \|\| conv > 1000 {
		return err
		}
		*o = OomScoreAdj(conv)
		return nil
		}

Choose a reason for hiding this comment

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

Bit on the fence here if we should have a special option-type for this. It does allow for validation on the client-side, but also makes assumption about what the daemon-side allows (given; it's unlikely to change); for docker run it looks like we leave the validation to the API, and using a regular IntVar option;

flags.IntVar(&copts.oomScoreAdj, "oom-score-adj", 0, "Tune host's OOM preferences (-1000 to 1000)")

Comment on lines +536 to +538

		if err != nil \|\| conv < -1000 \|\| conv > 1000 {
		return err
		}

Choose a reason for hiding this comment

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

In the case where err == nil but conv is out of range, the Set() call will silently be a no-op. Either generate a client-side error, or (preferably) do what @thaJeztah recommends and leave range validation to the API / daemon.

Choose a reason for hiding this comment

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

Yes, I think we can keep it simple for now, and do the same as we do for docker run, which is a regular IntVar.

I tend to reduce client-side validation to a minimum to keep the CLI agnostic and make the daemon "source of truth". That's not all set in stone, and there are some grey areas where it's worth the trade-ff but I'd like to avoid the CLI making assumptions that may not be correct.

Comment on lines +171 to +182

		"type": "object",
		"type": "object",

		"properties": {
		"driver": {"type": "string"},
		"options": {
		"type": "object",
		"patternProperties": {
		"^.+$": {"type": ["string", "number", "null"]}
		}
		}
		},
		"additionalProperties": false
		"properties": {
		"driver": {"type": "string"},
		"options": {
		"type": "object",
		"patternProperties": {
		"^.+$": {"type": ["string", "number", "null"]}
		}
		}
		},
		"additionalProperties": false

Choose a reason for hiding this comment

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

Can you undo these whitespace changes here?

Comment on lines +679 to +680

		}
		} No newline at end of file

Choose a reason for hiding this comment

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

Same here (missing newline at end of file)

Comment on lines +526 to +529

		func (o *OomScoreAdj) String() string {

		val := strconv.FormatInt(int64(*o), 10)
		return val

Choose a reason for hiding this comment

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

Don't think we need an intermediate var here, and there's a stray empty line at the start of the function

	func (o *OomScoreAdj) String() string {

	val := strconv.FormatInt(int64(*o), 10)
	return val
	func (o *OomScoreAdj) String() string {
	return strconv.FormatInt(int64(*o), 10)

Comment on lines +536 to +538

		if err != nil \|\| conv < -1000 \|\| conv > 1000 {
		return err
		}

Choose a reason for hiding this comment

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

Yes, I think we can keep it simple for now, and do the same as we do for docker run, which is a regular IntVar.

Comment on lines +259 to +260

		func updateService(ctx context.Context, apiClient client.NetworkAPIClient, flags pflag.FlagSet, spec swarm.ServiceSpec) error {

Choose a reason for hiding this comment

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

Can you remove the whitespace here?

Comment on lines +374 to +375

		if anyChanged(flags, flagOomScoreAdj) {

Choose a reason for hiding this comment

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

Can you remove whitespace here as well?

Comment on lines +135 to +136

		service, _, err := apiClient.ServiceInspectWithRaw(ctx, serviceID, types.ServiceInspectOptions{})

Choose a reason for hiding this comment

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

Same here (whitespace)

		flags.SetAnnotation(flagSysCtl, "version", []string{"1.40"})
		flags.Var(&opts.ulimits, flagUlimit, "Ulimit options")
		flags.SetAnnotation(flagUlimit, "version", []string{"1.41"})
		flags.Var(&opts.oomScoreAdj, flagOomScoreAdj, "oom score adjustment (-1000 to 1000)")

Choose a reason for hiding this comment

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

See other comments; this could probably just be an flags.IntVar

Comment on lines +533 to +534

		conv, _ = strconv.ParseInt(value, 10, 64)
		*o = OomScoreAdj(conv)

Choose a reason for hiding this comment

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

If value is not parseable as a number, the option gets silently set to 0? That doesn't seem right.

Choose a reason for hiding this comment

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

I've run into silent defaults over multiple code bases, if we want different/better standards I didn't see anything documented.

Example...

Choose a reason for hiding this comment

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

I've run into silent defaults over multiple code bases

What do "silent defaults" mean to you? Could you please elaborate?

Example...

You have linked to the definition of the String() method of a pflag.Value implementation, which contains a documented special case for how its zero value is formatted for user display.

	// NOTE: In spf13/pflag/flag.go, "0" is considered as "zero value" while "0 B" is not.
	// We return "0" in case value is 0 here so that the default value is hidden.
	// (Sometimes "default 0 B" is actually misleading)

I'm not following how this is relevant to the topic being discussed. Could you please explain?

Choose a reason for hiding this comment

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

I consider silent defaults to be any configuration options that are:

1.) set to values that have to be infered
2.) not explicitly documented outside of source code

Choose a reason for hiding this comment

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

I'm still lost. Why are we on this tangent about silent defaults? What do silent defaults have to do with a (pflag.Value).Set() method?

Choose a reason for hiding this comment

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

There is no tangent, I'm not worried about this. I'm just answering your question.

Choose a reason for hiding this comment

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

Claiming that you have answered my question is not an answer. Please answer my question:

What do silent defaults have to do with a (pflag.Value).Set() method?

Choose a reason for hiding this comment

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

I didn't actually reference that, you did.

Choose a reason for hiding this comment

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

func (o *OomScoreAdj) Set(value string) error, the body of which is the context for this discussion thread, implements a Set method necessary for *OomScoreAdj to satisfy the pflag.Value interface. In other words, func (o *OomScoreAdj) Set(value string) error is a (pflag.Value).Set() method.

		capAdd opts.ListOpts
		capDrop opts.ListOpts
		ulimits opts.UlimitOpt
		oomScoreAdj opts.OomScoreAdj

Choose a reason for hiding this comment

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

	oomScoreAdj opts.OomScoreAdj
	oomScoreAdj Uint64Opt

Choose a reason for hiding this comment

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

That's an unsigned 64 bit integer for a value that ranges from -1000 to 1000

Choose a reason for hiding this comment

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

My bad. You're correct; Uint64Opt would not be appropriate. No custom Value type is needed at all, since it's just a signed int.

	oomScoreAdj opts.OomScoreAdj
	oomScoreAdj int

Choose a reason for hiding this comment

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

It was a custom type originally because I had a range constraint in place. It should be fine, I need to fix the pipeline issues.

Choose a reason for hiding this comment

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

It was a custom type originally because I had a range constraint in place.

The history of how this PR has evolved is irrelevant. The custom type is not needed in the current iteration, therefore merging this PR with the custom type in place increases the ongoing maintenance cost of this project without any offsetting benefit.

It should be fine, I need to fix the pipeline issues.

Please stay on topic.

Comment on lines +520 to +536

		type OomScoreAdj int64

		func (o *OomScoreAdj) Type() string { return "int64" }

		func (o OomScoreAdj) Value() int64 { return int64(o) }

		func (o *OomScoreAdj) String() string {
		return strconv.FormatInt(int64(*o), 10)
		}

		func (o *OomScoreAdj) Set(value string) error {

		var conv int64
		conv, _ = strconv.ParseInt(value, 10, 64)
		*o = OomScoreAdj(conv)
		return nil
		}

Choose a reason for hiding this comment

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

There already exists a generic Uint64Opt type in the cli/command/service package. No need to define a new one.

Choose a reason for hiding this comment

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

That's an Unsigned 64 bit integer

Choose a reason for hiding this comment

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

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 61.02%. Comparing base (9bb1a62) to head (aa2c2cd).
Report is 49 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5145      +/-   ##
==========================================
- Coverage   61.48%   61.02%   -0.46%     
==========================================
  Files         298      296       -2     
  Lines       20811    20845      +34     
==========================================
- Hits        12795    12721      -74     
- Misses       7103     7207     +104     
- Partials      913      917       +4

		flags.SetAnnotation(flagSysCtl, "version", []string{"1.40"})
		flags.Var(&opts.ulimits, flagUlimit, "Ulimit options")
		flags.SetAnnotation(flagUlimit, "version", []string{"1.41"})
		flags.Int64Var(&opts.oomScoreAdj, flagOomScoreAdj, 0, "oom score adjustment (-1000 to 1000)")

Choose a reason for hiding this comment

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

Could we adjust the description to be more explicit on what it means to set a value more than 0 and less than 0?
Other command descriptions start with capital letters, I would expect this one to as well.

Maybe something like

	flags.Int64Var(&opts.oomScoreAdj, flagOomScoreAdj, 0, "oom score adjustment (-1000 to 1000)")
	flags.Int64Var(&opts.oomScoreAdj, flagOomScoreAdj, 0, "Adjust OOM score (-1000 to 1000). Lower values decrease the chance of the process being killed.")

Choose a reason for hiding this comment

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

I changed the message to "the value increases/decreases the probability of the process being killed during an OOM event"

Choose a reason for hiding this comment

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

could we start the sentence capitalized? Keeping the (-1000 to 1000) is also probably a good idea.

Choose a reason for hiding this comment

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

		flags.SetAnnotation(flagUlimitAdd, "version", []string{"1.41"})
		flags.Var(newListOptsVar(), flagUlimitRemove, "Remove a ulimit option")
		flags.SetAnnotation(flagUlimitRemove, "version", []string{"1.41"})
		flags.Int64Var(&options.oomScoreAdj, flagOomScoreAdj, 0, "oom score adjustment (-1000 to 1000)")

Choose a reason for hiding this comment

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

same here

		\| [`--network`](#network) \| `network` \| \| Network attachments \|
		\| `--no-healthcheck` \| `bool` \| \| Disable any container-specified HEALTHCHECK \|
		\| `--no-resolve-image` \| `bool` \| \| Do not query the registry to resolve image digest and supported platforms \|
		\| `--oom-score-adj` \| `int64` \| `0` \| oom score adjustment (-1000 to 1000) \|

Choose a reason for hiding this comment

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

same here

		\| `--network-rm` \| `list` \| \| Remove a network \|
		\| `--no-healthcheck` \| `bool` \| \| Disable any container-specified HEALTHCHECK \|
		\| `--no-resolve-image` \| `bool` \| \| Do not query the registry to resolve image digest and supported platforms \|
		\| `--oom-score-adj` \| `int64` \| `0` \| oom score adjustment (-1000 to 1000) \|

Choose a reason for hiding this comment

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

same here

Choose a reason for hiding this comment

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

LGTM

Is there something wrong with the validate/validate-md check? The diff in my PR doesn't show any of the changes that are reflected in the check. A git diff locally doesn't reflect any of those changes either.

Is there something wrong with the validate/validate-md check?

No.

	# check that the generated Markdown and the checked-in files match
	validate-md:
	runs-on: ubuntu-24.04
	steps:
	-
	name: Checkout
	uses: actions/checkout@v4
	-
	name: Generate
	shell: 'script --return --quiet --command "bash {0}"'
	run: \|
	make -f docker.Makefile mddocs
	-
	name: Validate
	run: \|
	if [[ $(git diff --stat) != '' ]]; then
	echo 'fail: generated files do not match checked-in files'
	git --no-pager diff
	exit 1
	fi

The markdown files are generated from the Go sources. Following Go ecosystem convention, generated files are committed to the repo alongside their sources. The validate/validate-md check is asserting that the generated files are in sync.

Run make -f docker.Makefile mddocs locally and commit the result.

		flags.SetAnnotation(flagUlimitAdd, "version", []string{"1.41"})
		flags.Var(newListOptsVar(), flagUlimitRemove, "Remove a ulimit option")
		flags.SetAnnotation(flagUlimitRemove, "version", []string{"1.41"})
		flags.Int64Var(&options.oomScoreAdj, flagOomScoreAdj, 0, "(-1000 to 1000) Increases/Decreases the probability of the process being killed during an OOM event ")

Choose a reason for hiding this comment

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

For comparison; here's the description for docker run;

      --oom-score-adj int                Tune host's OOM preferences (-1000 to 1000)

Att least we should put the range at the end

Choose a reason for hiding this comment

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

And add an annotation for the API version

		updateInt64Value(flagReserveMemory, &task.Resources.Reservations.MemoryBytes)
		}

		if anyChanged(flags, flagOomScoreAdj) {

Choose a reason for hiding this comment

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

nit as it's a single flag we check for, this could be flags.Chanced(flagOomScoreAdj) (not a blocker though)

		flags.Var(&opts.ulimits, flagUlimit, "Ulimit options")
		flags.SetAnnotation(flagUlimit, "version", []string{"1.41"})
		flags.Int64Var(&opts.oomScoreAdj, flagOomScoreAdj, 0, "Tune host's OOM preferences (-1000 to 1000) ")
		flags.SetAnnotation(flagOomScoreAdj, "version", []string{"1.41"})

Choose a reason for hiding this comment

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

API v1.41 was introduced in Moby v20.10. That doesn't seem correct. Doesn't this flag correspond to a brand new API field, which was only added in the Moby v27 Engine API client?

Signed-off-by: plaurent <patrick@saint-laurent.us>

Choose a reason for hiding this comment

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

LGTM, thanks!

backport: [27.1 backport] Add OomScoreAdj to "docker service create" and "docker stack" #5274

thaJeztah changed the title ~~Add OomScoreAdj to "docker service create" and "docker compose"~~ Add OomScoreAdj to "docker service create" and "docker stack"

Jul 19, 2024

		\| [`--network`](#network) \| `network` \| \| Network attachments \|
		\| `--no-healthcheck` \| `bool` \| \| Disable any container-specified HEALTHCHECK \|
		\| `--no-resolve-image` \| `bool` \| \| Do not query the registry to resolve image digest and supported platforms \|
		\| `--oom-score-adj` \| `int64` \| `0` \| oom score adjustment (-1000 to 1000) \|

		\| `--network-rm` \| `list` \| \| Remove a network \|
		\| `--no-healthcheck` \| `bool` \| \| Disable any container-specified HEALTHCHECK \|
		\| `--no-resolve-image` \| `bool` \| \| Do not query the registry to resolve image digest and supported platforms \|
		\| `--oom-score-adj` \| `int64` \| `0` \| oom score adjustment (-1000 to 1000) \|