From 902b0b567391394d89028093cfeb31bf289761bf Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 30 Sep 2020 12:09:41 -0700 Subject: [PATCH] jobspec: lower min cpu resources from 10->1 Since CPU resources are usually a soft limit it is desirable to allow setting it as low as possible to allow tasks to run only in "idle" time. Setting it to 0 is still not allowed to avoid potential unintentional side effects with allowing a zero value. While there may not be any side effects this commit attempts to minimize risk by avoiding the issue. This does *not* change the defaults. --- CHANGELOG.md | 1 + api/resources.go | 5 +- command/agent/job_endpoint_test.go | 17 ++++++ nomad/structs/structs.go | 17 +----- nomad/structs/structs_test.go | 61 +++++++++++++++++++ .../hashicorp/nomad/api/resources.go | 5 +- 6 files changed, 88 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d7d826002af..4b6a9e3f64ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ IMPROVEMENTS: * core: Improved job deregistration error logging. [[GH-8745](https://github.com/hashicorp/nomad/issues/8745)] * api: Added support for cancellation contexts to HTTP API. [[GH-8836](https://github.com/hashicorp/nomad/issues/8836)] * driver/docker: upgrade pause container and detect architecture [[GH-8957](https://github.com/hashicorp/nomad/pull/8957)] + * jobspec: Lowered minimum CPU allowed from 10 to 1. [[GH-8996](https://github.com/hashicorp/nomad/issues/8996)] BUG FIXES: diff --git a/api/resources.go b/api/resources.go index ac25d4662c9d..74ef55e4e1fb 100644 --- a/api/resources.go +++ b/api/resources.go @@ -56,7 +56,7 @@ func DefaultResources() *Resources { // IN nomad/structs/structs.go and should be kept in sync. func MinResources() *Resources { return &Resources{ - CPU: intToPtr(20), + CPU: intToPtr(1), MemoryMB: intToPtr(10), } } @@ -110,6 +110,9 @@ type NetworkResource struct { } func (n *NetworkResource) Canonicalize() { + // COMPAT(0.12) MBits is deprecated but this should not be removed + // until MBits is fully removed. Removing this *without* fully removing + // MBits would cause unnecessary job diffs and destructive updates. if n.MBits == nil { n.MBits = intToPtr(10) } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 29bf1b31fe0e..6e361b0355f5 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2849,6 +2849,23 @@ func TestJobs_ApiJobToStructsJobUpdate(t *testing.T) { require.Equal(t, group2, *structsJob.TaskGroups[1].Update) } +// TestJobs_Matching_Resources asserts: +// api.{Default,Min}Resources == structs.{Default,Min}Resources +// +// While this is an odd place to test that, this is where both are imported, +// validated, and converted. +func TestJobs_Matching_Resources(t *testing.T) { + t.Parallel() + + // api.MinResources == structs.MinResources + structsMinRes := ApiResourcesToStructs(api.MinResources()) + assert.Equal(t, structs.MinResources(), structsMinRes) + + // api.DefaultResources == structs.DefaultResources + structsDefaultRes := ApiResourcesToStructs(api.DefaultResources()) + assert.Equal(t, structs.DefaultResources(), structsDefaultRes) +} + // TestHTTP_JobValidate_SystemMigrate asserts that a system job with a migrate // stanza fails to validate but does not panic (see #5477). func TestHTTP_JobValidate_SystemMigrate(t *testing.T) { diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index c0b938103e8d..bce817474fd6 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -2099,7 +2099,7 @@ func DefaultResources() *Resources { // api/resources.go and should be kept in sync. func MinResources() *Resources { return &Resources{ - CPU: 20, + CPU: 1, MemoryMB: 10, } } @@ -2223,11 +2223,6 @@ func (r *Resources) MeetsMinResources() error { if r.MemoryMB < minResources.MemoryMB { mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum MemoryMB value is %d; got %d", minResources.MemoryMB, r.MemoryMB)) } - for i, n := range r.Networks { - if err := n.MeetsMinResources(); err != nil { - mErr.Errors = append(mErr.Errors, fmt.Errorf("network resource at index %d failed: %v", i, err)) - } - } return mErr.ErrorOrNil() } @@ -2444,16 +2439,6 @@ func (n *NetworkResource) Canonicalize() { } } -// MeetsMinResources returns an error if the resources specified are less than -// the minimum allowed. -func (n *NetworkResource) MeetsMinResources() error { - var mErr multierror.Error - if n.MBits < 1 { - mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum MBits value is 1; got %d", n.MBits)) - } - return mErr.ErrorOrNil() -} - // Copy returns a deep copy of the network resource func (n *NetworkResource) Copy() *NetworkResource { if n == nil { diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index d2e7a283c9a1..942710db769f 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -1378,6 +1378,67 @@ func TestTask_Validate(t *testing.T) { } } +func TestTask_Validate_Resources(t *testing.T) { + cases := []struct { + name string + res *Resources + }{ + { + name: "Minimum", + res: MinResources(), + }, + { + name: "Default", + res: DefaultResources(), + }, + { + name: "Full", + res: &Resources{ + CPU: 1000, + MemoryMB: 1000, + IOPS: 1000, + Networks: []*NetworkResource{ + { + Mode: "host", + Device: "localhost", + CIDR: "127.0.0.0/8", + IP: "127.0.0.1", + MBits: 1000, + DNS: &DNSConfig{ + Servers: []string{"localhost"}, + Searches: []string{"localdomain"}, + Options: []string{"ndots:5"}, + }, + ReservedPorts: []Port{ + { + Label: "reserved", + Value: 1234, + To: 1234, + HostNetwork: "loopback", + }, + }, + DynamicPorts: []Port{ + { + Label: "dynamic", + Value: 5678, + To: 5678, + HostNetwork: "loopback", + }, + }, + }, + }, + }, + }, + } + + for i := range cases { + tc := cases[i] + t.Run(tc.name, func(t *testing.T) { + require.NoError(t, tc.res.Validate()) + }) + } +} + func TestTask_Validate_Services(t *testing.T) { s1 := &Service{ Name: "service-name", diff --git a/vendor/github.com/hashicorp/nomad/api/resources.go b/vendor/github.com/hashicorp/nomad/api/resources.go index ac25d4662c9d..74ef55e4e1fb 100644 --- a/vendor/github.com/hashicorp/nomad/api/resources.go +++ b/vendor/github.com/hashicorp/nomad/api/resources.go @@ -56,7 +56,7 @@ func DefaultResources() *Resources { // IN nomad/structs/structs.go and should be kept in sync. func MinResources() *Resources { return &Resources{ - CPU: intToPtr(20), + CPU: intToPtr(1), MemoryMB: intToPtr(10), } } @@ -110,6 +110,9 @@ type NetworkResource struct { } func (n *NetworkResource) Canonicalize() { + // COMPAT(0.12) MBits is deprecated but this should not be removed + // until MBits is fully removed. Removing this *without* fully removing + // MBits would cause unnecessary job diffs and destructive updates. if n.MBits == nil { n.MBits = intToPtr(10) }