Add OomScoreAdj to "docker service create" and "docker stack" #5145

psaintlaurent · 2024-06-11T16:55:07Z

depends on Add OOMScoreAdj to the moby API moby/moby#47950
depends on vendor: github.com/moby/swarmkit/v2 v2.0.0-20240611172349-ea1a7cec35cb #5169
depends on vendor: github.com/docker/docker 1a1f3cff45ec (master, v27.0-dev) #5170

- What I did
I added oom-score-adj flag to docker service create and docker service update

- How I did it
By modifying container service to accept the option as well as adding a flag to the CLI

- How to test it
Compile this PR and copy the CLI binary to a running moby dev shell using moby/moby#47950

- Description for the changelog

Add OOMScoreAdj to docker service create and docker stack

thaJeztah · 2024-06-18T11:36:56Z

cli/compose/schema/data/config_schema_v3.14.json

@@ -0,0 +1,680 @@
+{


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.

thaJeztah · 2024-06-18T18:08:33Z

cli/command/container/opts.go

-		OomScoreAdj:     copts.oomScoreAdj,
+		OomScoreAdj:     int(copts.oomScoreAdj),


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

thaJeztah · 2024-06-18T18:15:43Z

opts/opts.go

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


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;

cli/cli/command/container/opts.go

Line 306 in 64206ae

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

corhere · 2024-06-20T19:47:36Z

opts/opts.go

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


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.

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.

thaJeztah · 2024-06-20T20:30:13Z

cli/compose/schema/data/config_schema_v3.13.json

-            "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


Can you undo these whitespace changes here?

thaJeztah · 2024-06-20T20:30:34Z

cli/compose/schema/data/config_schema_v3.13.json

-}
+}


Same here (missing newline at end of file)

thaJeztah · 2024-06-20T20:37:05Z

opts/opts.go

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


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

Suggested change

func (o *OomScoreAdj) String() string {

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

return val

func (o *OomScoreAdj) String() string {

return strconv.FormatInt(int64(*o), 10)

thaJeztah · 2024-06-20T20:39:20Z

opts/opts.go

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


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.

thaJeztah · 2024-06-20T20:40:15Z

cli/command/service/update.go

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


Can you remove the whitespace here?

thaJeztah · 2024-06-20T20:40:29Z

cli/command/service/update.go

+	if anyChanged(flags, flagOomScoreAdj) {
+


Can you remove whitespace here as well?

thaJeztah · 2024-06-20T20:40:44Z

cli/command/service/update.go

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


Same here (whitespace)

thaJeztah · 2024-06-20T20:41:50Z

cli/command/service/create.go

@@ -68,6 +68,7 @@ func newCreateCommand(dockerCli command.Cli) *cobra.Command {
 	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)")


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

corhere · 2024-06-21T17:29:20Z

opts/opts.go

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


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

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

Example...

cli/opts/opts.go

Line 454 in e174fe5

return "0"

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.

cli/opts/opts.go

Lines 448 to 450 in e174fe5

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

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

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?

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

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?

I didn't actually reference that, you did.

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.

corhere · 2024-06-21T17:31:35Z

cli/command/service/opts.go

@@ -529,6 +529,7 @@ type serviceOptions struct {
 	capAdd          opts.ListOpts
 	capDrop         opts.ListOpts
 	ulimits         opts.UlimitOpt
+	oomScoreAdj     opts.OomScoreAdj


Suggested change

oomScoreAdj opts.OomScoreAdj

oomScoreAdj Uint64Opt

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

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.

Suggested change

oomScoreAdj opts.OomScoreAdj

oomScoreAdj int

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.

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.

corhere · 2024-06-21T17:32:31Z

opts/opts.go

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


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

That's an Unsigned 64 bit integer

Right, my mistake. You don't need any custom type at all! https://pkg.go.dev/github.com/spf13/pflag#FlagSet.IntVar

codecov-commenter · 2024-06-24T15:35:47Z

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

Benehiko · 2024-07-09T07:37:44Z

cli/command/service/create.go

@@ -68,6 +68,7 @@ func newCreateCommand(dockerCli command.Cli) *cobra.Command {
 	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)")


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

Suggested change

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.")

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

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

I'll make the change however its going to ruin formatting of docs/reference/commandline/service_create.md and
docs/reference/commandline/service_update.md

I was warned about introducing unnecessary whitespace formatting changes previously.

Benehiko · 2024-07-09T07:38:33Z

cli/command/service/update.go

@@ -108,6 +108,7 @@ func newUpdateCommand(dockerCli command.Cli) *cobra.Command {
 	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)")


Benehiko · 2024-07-09T07:39:53Z

docs/reference/commandline/service_create.md

@@ -46,6 +46,7 @@ Create a new service
 | [`--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)                                                                |


Benehiko · 2024-07-09T07:39:59Z

docs/reference/commandline/service_update.md

@@ -59,6 +59,7 @@ Update a service
 | `--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)                                                                |


Benehiko

LGTM

psaintlaurent · 2024-07-11T13:45:55Z

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.

corhere · 2024-07-11T18:32:37Z

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

No.

cli/.github/workflows/validate.yml

Lines 38 to 57 in 2f529a1

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

thaJeztah · 2024-07-18T18:15:04Z

cli/command/service/update.go

@@ -108,6 +108,7 @@ func newUpdateCommand(dockerCli command.Cli) *cobra.Command {
 	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 ")


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

And add an annotation for the API version

thaJeztah · 2024-07-18T18:18:51Z

cli/command/service/update.go

@@ -367,6 +368,10 @@ func updateService(ctx context.Context, apiClient client.NetworkAPIClient, flags
 		updateInt64Value(flagReserveMemory, &task.Resources.Reservations.MemoryBytes)
 	}

+	if anyChanged(flags, flagOomScoreAdj) {


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

corhere · 2024-07-19T14:53:33Z

cli/command/service/create.go

@@ -68,6 +68,8 @@ func newCreateCommand(dockerCli command.Cli) *cobra.Command {
 	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, "Tune host's OOM preferences (-1000 to 1000) ")
+	flags.SetAnnotation(flagOomScoreAdj, "version", []string{"1.41"})


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>

thaJeztah

LGTM, thanks!

thaJeztah · 2024-07-19T17:09:44Z

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

psaintlaurent force-pushed the ENGINE-903 branch from 7c5a646 to 4a82dc0 Compare June 11, 2024 21:07

psaintlaurent changed the title ~~Add OomScoreAdj to "docker service create"~~ Add OomScoreAdj to "docker service create" and "docker compose" Jun 12, 2024

psaintlaurent force-pushed the ENGINE-903 branch from 6f0f551 to fd3a9cd Compare June 14, 2024 12:43

thaJeztah added status/2-code-review area/swarm area/stack labels Jun 18, 2024

thaJeztah reviewed Jun 18, 2024

View reviewed changes

This was referenced Jun 18, 2024

vendor: github.com/moby/swarmkit/v2 v2.0.0-20240611172349-ea1a7cec35cb #5169

Merged

vendor: github.com/docker/docker 1a1f3cff45ec (master, v27.0-dev) #5170

Merged

thaJeztah added the impact/changelog label Jun 18, 2024

thaJeztah added this to the 27.0.0 milestone Jun 18, 2024

thaJeztah reviewed Jun 18, 2024

View reviewed changes

psaintlaurent force-pushed the ENGINE-903 branch 3 times, most recently from 2b9f928 to 25083e3 Compare June 20, 2024 19:33

corhere reviewed Jun 20, 2024

View reviewed changes

thaJeztah reviewed Jun 20, 2024

View reviewed changes

psaintlaurent force-pushed the ENGINE-903 branch 2 times, most recently from feaf642 to e174fe5 Compare June 21, 2024 16:58

corhere reviewed Jun 21, 2024

View reviewed changes

vvoland modified the milestones: 27.0.0, 28.0.0 Jun 26, 2024

psaintlaurent force-pushed the ENGINE-903 branch from bd276ea to cf1a770 Compare June 26, 2024 14:16

psaintlaurent requested a review from a team as a code owner June 26, 2024 14:16

psaintlaurent force-pushed the ENGINE-903 branch 2 times, most recently from ef69fb9 to 02761fd Compare June 28, 2024 20:04

psaintlaurent force-pushed the ENGINE-903 branch from 02761fd to 20c4ad9 Compare July 8, 2024 16:08

psaintlaurent force-pushed the ENGINE-903 branch from 3154800 to 73b33e6 Compare July 8, 2024 17:40

corhere approved these changes Jul 8, 2024

View reviewed changes

Benehiko reviewed Jul 9, 2024

View reviewed changes

psaintlaurent force-pushed the ENGINE-903 branch 2 times, most recently from 78a2ad5 to 10c8422 Compare July 9, 2024 15:20

Benehiko approved these changes Jul 10, 2024

View reviewed changes

psaintlaurent force-pushed the ENGINE-903 branch from 10c8422 to 2636520 Compare July 12, 2024 11:34

thaJeztah reviewed Jul 18, 2024

View reviewed changes

psaintlaurent force-pushed the ENGINE-903 branch from 2636520 to d25f677 Compare July 19, 2024 14:03

corhere reviewed Jul 19, 2024

View reviewed changes

Allow for OomScoreAdj

aa2c2cd

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

psaintlaurent force-pushed the ENGINE-903 branch from d25f677 to aa2c2cd Compare July 19, 2024 17:02

thaJeztah approved these changes Jul 19, 2024

View reviewed changes

thaJeztah merged commit 6559d86 into docker:master Jul 19, 2024
89 checks passed

thaJeztah mentioned this pull request Jul 19, 2024

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

Merged

3 tasks

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

		OomScoreAdj: copts.oomScoreAdj,
		OomScoreAdj: int(copts.oomScoreAdj),

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

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

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

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

	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.")

Add OomScoreAdj to "docker service create" and "docker stack" #5145

Add OomScoreAdj to "docker service create" and "docker stack" #5145

Conversation

psaintlaurent commented Jun 11, 2024 • edited by thaJeztah Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psaintlaurent Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psaintlaurent Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psaintlaurent Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

psaintlaurent Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benehiko left a comment

Choose a reason for hiding this comment

psaintlaurent commented Jul 11, 2024

corhere commented Jul 11, 2024

thaJeztah Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corhere Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Jul 19, 2024

psaintlaurent commented Jun 11, 2024 •

edited by thaJeztah

Loading

psaintlaurent Jun 21, 2024 •

edited

Loading

psaintlaurent Jun 25, 2024 •

edited

Loading

psaintlaurent Jun 21, 2024 •

edited

Loading

codecov-commenter commented Jun 24, 2024 •

edited

Loading

psaintlaurent Jul 9, 2024 •

edited

Loading

thaJeztah Jul 18, 2024 •

edited

Loading

corhere Jul 19, 2024 •

edited

Loading