If MaxParallel == 0 default limit to count

[clinta]

If you run a job with an update stanza but do not specify max_parallel it defaults to 0 which prevents any allocations from ever being updated. A max_parallel value of 0 should be treated the same as as a job with no update stanza specified.

[dadgar]

@clinta Do you think it is better if we make it a validation error instead. It feels like this may catch people unaware.

[clinta]

I can't think of a situation where someone could be depending on the current behavior. I know I expected an omitted max_parallel to behave the same as an omitted update. And under it's current behavior an omitted max_parallel (or an explicit max_parallel value of 0) results in a task group that can never be updated. If anybody else has encountered this I'd bet they are either working around it by specifying max_parallel or they have a job that has never been updated and re-deployed.

[jippi]

I would personally expect a value of 0 to be a validation error, rather than an implicit Count for the task group.

If we need to signal all a better value may be -1 or similar which is clearly "odd"

[dadgar]

@clinta Would you mind changing it to a validation issue? https://github.com/hashicorp/nomad/blob/master/nomad/structs/structs.go#L2065 and https://github.com/hashicorp/nomad/blob/master/nomad/structs/structs_test.go#L1394

[clinta]

If this becomes a validation error should the default value be changed? Is there anywhere else where a default value is invalid?

There's some code here that defaults to a 10% of the count (or a minimum of 1). Perhaps this should be run anytime a MaxParallel of 0 is encountered, instead of just for migrating from the old stagger syntax.

nomad/nomad/structs/structs.go

Lines 1532 to 1549 in 46e6547

	// The MaxParallel for the job should be 10% of the total count
	// unless there is just one task group then we can infer the old
	// max parallel should be the new
	tgu := base.Copy()
	if l != 1 {
	// RoundTo 10%
	var percent float64 = float64(tg.Count) * 0.1
	tgu.MaxParallel = int(percent + 0.5)
	}
	// Safety guards
	if tgu.MaxParallel == 0 {
	tgu.MaxParallel = 1
	} else if tgu.MaxParallel > tg.Count {
	tgu.MaxParallel = tg.Count
	}
	tg.Update = tgu

[dadgar]

@clinta Lets update the default to be 1. Rather error on the conservative side. Most people should be specifying this if they want more. It is something job operator should be conscious of.

You can add the defaulting here: https://github.com/hashicorp/nomad/blob/master/api/jobs.go#L379

And update the default in the nomad/structs/structs.go file that is referenced.

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.