From 622d3ddb92ea7e656ef831641c02024cb5a5d6d1 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle Date: Mon, 13 Nov 2017 12:05:30 -0500 Subject: [PATCH 1/4] Fixed test and moved constants into standalone func In #3520, work was done to true up the defaults for Nomad resource stanzas with the documentation. This fixes the tests that I accidentally broke in the process. Some questions were raised about using dynamic elements as part of expects, which is why I opted to copy the MinResources pattern. During this refactor I also noticed that structs.go had a similar issue and an inconsistent minium for CPU. --- api/jobs_test.go | 8 ++++---- api/resources.go | 27 +++++++++++++++++++++++---- nomad/structs/structs.go | 27 +++++++++++++++++++++++---- 3 files changed, 50 insertions(+), 12 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index 90e51ddfc2f..de4ae634dfb 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -139,7 +139,7 @@ func TestJobs_Canonicalize(t *testing.T) { { KillTimeout: helper.TimeToPtr(5 * time.Second), LogConfig: DefaultLogConfig(), - Resources: MinResources(), + Resources: DefaultResources(), }, }, }, @@ -201,7 +201,7 @@ func TestJobs_Canonicalize(t *testing.T) { { Name: "task1", LogConfig: DefaultLogConfig(), - Resources: MinResources(), + Resources: DefaultResources(), KillTimeout: helper.TimeToPtr(5 * time.Second), }, }, @@ -550,7 +550,7 @@ func TestJobs_Canonicalize(t *testing.T) { { Name: "task1", LogConfig: DefaultLogConfig(), - Resources: MinResources(), + Resources: DefaultResources(), KillTimeout: helper.TimeToPtr(5 * time.Second), }, }, @@ -582,7 +582,7 @@ func TestJobs_Canonicalize(t *testing.T) { { Name: "task1", LogConfig: DefaultLogConfig(), - Resources: MinResources(), + Resources: DefaultResources(), KillTimeout: helper.TimeToPtr(5 * time.Second), }, }, diff --git a/api/resources.go b/api/resources.go index e9e165419ec..be415a4a5c1 100644 --- a/api/resources.go +++ b/api/resources.go @@ -12,28 +12,47 @@ type Resources struct { Networks []*NetworkResource } +// Canonicalize will supply missing values in the cases +// where they are not provided. func (r *Resources) Canonicalize() { + defaultResources := DefaultResources() if r.CPU == nil { - r.CPU = helper.IntToPtr(100) + r.CPU = defaultResources.CPU } if r.MemoryMB == nil { - r.MemoryMB = helper.IntToPtr(300) + r.MemoryMB = defaultResources.MemoryMB } if r.IOPS == nil { - r.IOPS = helper.IntToPtr(0) + r.IOPS = defaultResources.IOPS } for _, n := range r.Networks { n.Canonicalize() } } +// DefaultResources is a small resources object that contains the +// default resources requests that we will provide to an object. +// --- THIS FUNCTION IS REPLICATED IN nomad/structs/structs.go +// and should be kept in sync. +func DefaultResources() *Resources { + return &Resources{ + CPU: helper.IntToPtr(100), + MemoryMB: helper.IntToPtr(300), + IOPS: helper.IntToPtr(0), + } +} + +// MinResources is a small resources object that contains the +// absolute minimum resources that we will provide to an object. +// This should not be confused with the defaults which are +// provided in DefaultResources() --- THIS LOGIC IS REPLICATED +// IN nomad/structs/structs.go and should be kept in sync. func MinResources() *Resources { return &Resources{ CPU: helper.IntToPtr(100), MemoryMB: helper.IntToPtr(10), IOPS: helper.IntToPtr(0), } - } // Merge merges this resource with another resource. diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 3b8b7ca496d..4efe8e60a09 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1222,8 +1222,25 @@ const ( BytesInMegabyte = 1024 * 1024 ) -// DefaultResources returns the default resources for a task. + +// DefaultResources is a small resources object that contains the +// default resources requests that we will provide to an object. +// --- THIS FUNCTION IS REPLICATED IN api/resources.go and should +// be kept in sync. func DefaultResources() *Resources { + return &Resources{ + CPU: 100, + MemoryMB: 300, + IOPS: 0, + } +} + +// MinResources is a small resources object that contains the +// absolute minimum resources that we will provide to an object. +// This should not be confused with the defaults which are +// provided in Canonicalize() --- THIS FUNCTION IS REPLICATED IN +// api/resources.go and should be kept in sync. +func MinResources() *Resources { return &Resources{ CPU: 100, MemoryMB: 10, @@ -1269,15 +1286,17 @@ func (r *Resources) Canonicalize() { // MeetsMinResources returns an error if the resources specified are less than // the minimum allowed. +// This is based on the minimums defined in the Resources type func (r *Resources) MeetsMinResources() error { var mErr multierror.Error - if r.CPU < 20 { + minResources := MinResources() + if r.CPU < minResources.CPU { mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum CPU value is 20; got %d", r.CPU)) } - if r.MemoryMB < 10 { + if r.MemoryMB < minResources.MemoryMB { mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum MemoryMB value is 10; got %d", r.MemoryMB)) } - if r.IOPS < 0 { + if r.IOPS < minResources.IOPS { mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum IOPS value is 0; got %d", r.IOPS)) } for i, n := range r.Networks { From 904d1711086f6bdcdcbc0742ea746e1e8bafc714 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle Date: Mon, 13 Nov 2017 12:25:02 -0500 Subject: [PATCH 2/4] gofmt and goimports --- api/resources.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/resources.go b/api/resources.go index be415a4a5c1..8c547ee7e7d 100644 --- a/api/resources.go +++ b/api/resources.go @@ -32,7 +32,7 @@ func (r *Resources) Canonicalize() { // DefaultResources is a small resources object that contains the // default resources requests that we will provide to an object. -// --- THIS FUNCTION IS REPLICATED IN nomad/structs/structs.go +// --- THIS FUNCTION IS REPLICATED IN nomad/structs/structs.go // and should be kept in sync. func DefaultResources() *Resources { return &Resources{ @@ -44,8 +44,8 @@ func DefaultResources() *Resources { // MinResources is a small resources object that contains the // absolute minimum resources that we will provide to an object. -// This should not be confused with the defaults which are -// provided in DefaultResources() --- THIS LOGIC IS REPLICATED +// This should not be confused with the defaults which are +// provided in DefaultResources() --- THIS LOGIC IS REPLICATED // IN nomad/structs/structs.go and should be kept in sync. func MinResources() *Resources { return &Resources{ From 6c362720cffdea1fff51afd743b46809e6028a84 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle Date: Mon, 13 Nov 2017 12:32:52 -0500 Subject: [PATCH 3/4] gofmt and goimports nomad/structs/structs.go --- nomad/structs/structs.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 4efe8e60a09..3389b40f725 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -24,8 +24,7 @@ import ( "github.com/gorhill/cronexpr" "github.com/hashicorp/consul/api" - "github.com/hashicorp/go-multierror" - "github.com/hashicorp/go-version" + multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/nomad/acl" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/args" @@ -1222,10 +1221,9 @@ const ( BytesInMegabyte = 1024 * 1024 ) - // DefaultResources is a small resources object that contains the // default resources requests that we will provide to an object. -// --- THIS FUNCTION IS REPLICATED IN api/resources.go and should +// --- THIS FUNCTION IS REPLICATED IN api/resources.go and should // be kept in sync. func DefaultResources() *Resources { return &Resources{ @@ -1237,8 +1235,8 @@ func DefaultResources() *Resources { // MinResources is a small resources object that contains the // absolute minimum resources that we will provide to an object. -// This should not be confused with the defaults which are -// provided in Canonicalize() --- THIS FUNCTION IS REPLICATED IN +// This should not be confused with the defaults which are +// provided in Canonicalize() --- THIS FUNCTION IS REPLICATED IN // api/resources.go and should be kept in sync. func MinResources() *Resources { return &Resources{ From c0f4f520b7b467e48c3547b3b527a8d53f0a12d4 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle Date: Mon, 13 Nov 2017 12:51:19 -0500 Subject: [PATCH 4/4] Review feedback + re-add dropped import --- nomad/structs/structs.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 3389b40f725..0bf908f86a3 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -25,6 +25,7 @@ import ( "github.com/gorhill/cronexpr" "github.com/hashicorp/consul/api" multierror "github.com/hashicorp/go-multierror" + "github.com/hashicorp/go-version" "github.com/hashicorp/nomad/acl" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/args" @@ -1289,13 +1290,13 @@ func (r *Resources) MeetsMinResources() error { var mErr multierror.Error minResources := MinResources() if r.CPU < minResources.CPU { - mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum CPU value is 20; got %d", r.CPU)) + mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum CPU value is %d; got %d", minResources.CPU, r.CPU)) } if r.MemoryMB < minResources.MemoryMB { - mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum MemoryMB value is 10; got %d", r.MemoryMB)) + mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum MemoryMB value is %d; got %d", minResources.CPU, r.MemoryMB)) } if r.IOPS < minResources.IOPS { - mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum IOPS value is 0; got %d", r.IOPS)) + mErr.Errors = append(mErr.Errors, fmt.Errorf("minimum IOPS value is %d; got %d", minResources.CPU, r.IOPS)) } for i, n := range r.Networks { if err := n.MeetsMinResources(); err != nil {