Skip to content

Commit 21ede2a

Browse files
author
Steve Powell
committed
Report an error if -totMemory omitted, or specifies less than 1K.
Previously, if the -totMemory flag was omitted the memory calculator would allocate the minimum of the size ranges supplied in -memorySizes. However, if some memory types had a zero lower-bound (the default), the allocation would be zero. This is never a valid memory option. This change makes an omitted -totMemory flag, or a flag that specifies less than one kilobyte, an error, resulting in a non-zero exit code (and a message on stderr). Some code in the memory calculator is removed. [#94311618]
1 parent 1ac380f commit 21ede2a

File tree

5 files changed

+59
-77
lines changed

5 files changed

+59
-77
lines changed

‎flags/validate_flags.go‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ var (
5454
)
5555

5656
// Validate flags passed on command line; exit(1) if invalid; exit(2) if help printed
57-
func ValidateFlags() (memSize memory.MemSize, weights map[string]float64, sizes map[string]memory.Range, memIsSpecified bool) {
57+
func ValidateFlags() (memSize memory.MemSize, weights map[string]float64, sizes map[string]memory.Range) {
5858

5959
flag.Parse() // exit on error
6060

@@ -63,24 +63,29 @@ func ValidateFlags() (memSize memory.MemSize, weights map[string]float64, sizes
6363
os.Exit(2)
6464
}
6565

66-
// validation routines exit on error
67-
memSize, memIsSpecified = validateTotMemory(*totMemory)
66+
// validation routines will not return on error
67+
memSize = validateTotMemory(*totMemory)
6868
weights = validateWeights(*memoryWeights)
6969
sizes = validateSizes(*memorySizes)
7070

71-
return memSize, weights, sizes, memIsSpecified
71+
return memSize, weights, sizes
7272
}
7373

74-
func validateTotMemory(mem string) (value memory.MemSize, isSpecified bool) {
74+
func validateTotMemory(mem string) memory.MemSize {
7575
if mem == "" {
76-
return memory.MEMSIZE_ZERO, false
76+
fmt.Fprintf(os.Stderr, "-%s must be specified", totalFlag)
77+
os.Exit(1)
7778
}
7879
ms, err := memory.NewMemSizeFromString(mem)
7980
if err != nil {
8081
fmt.Fprintf(os.Stderr, "Error in -%s flag: %s", totalFlag, err)
8182
os.Exit(1)
8283
}
83-
return ms, true
84+
if ms.LessThan(memory.MemSize(1024)) {
85+
fmt.Fprintf(os.Stderr, "Total memory (-%s flag) is less than 1K", totalFlag)
86+
os.Exit(1)
87+
}
88+
return ms
8489
}
8590

8691
func validateWeights(weights string) map[string]float64 {

‎integration/main_test.go‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,33 @@ var _ = Describe("java-buildpack-memory-calculator executable", func() {
6161
Ω(string(se)).Should(ContainSubstring("Weight must be positive in -memoryWeights flag; clause 'heap:-2'"), "stderr incorrect for "+badFlags[0])
6262
})
6363

64+
It("executes with error when no total memory is supplied", func() {
65+
badFlags :=
66+
[]string{
67+
"-memoryWeights=heap:5,stack:1,permgen:3,native:1",
68+
"-memorySizes=stack:2m..,heap:30m..400m,permgen:10m..12m",
69+
}
70+
so, se, err := runOutAndErr(badFlags...)
71+
Ω(err).Should(HaveOccurred(), "no -totMemory flag")
72+
73+
Ω(string(so)).Should(BeEmpty(), "stdout not empty when no -totMemory flag")
74+
Ω(string(se)).Should(ContainSubstring("-totMemory must be specified"), "stderr incorrect when no -totMemory flag")
75+
})
76+
77+
It("executes with error when too little total memory is supplied", func() {
78+
badFlags :=
79+
[]string{
80+
"-totMemory=1023b",
81+
"-memoryWeights=heap:5,stack:1,permgen:3,native:1",
82+
"-memorySizes=stack:2m..,heap:30m..400m,permgen:10m..12m",
83+
}
84+
so, se, err := runOutAndErr(badFlags...)
85+
Ω(err).Should(HaveOccurred(), badFlags[0])
86+
87+
Ω(string(so)).Should(BeEmpty(), "stdout not empty for "+badFlags[0])
88+
Ω(string(se)).Should(ContainSubstring("Total memory (-totMemory flag) is less than 1K"), "stderr incorrect for "+badFlags[0])
89+
})
90+
6491
Context("with valid parameters", func() {
6592
var (
6693
totMemFlag, weightsFlag, sizesFlag string
@@ -79,25 +106,6 @@ var _ = Describe("java-buildpack-memory-calculator executable", func() {
79106
sOut, sErr, cmdErr = runOutAndErr(goodFlags...)
80107
})
81108

82-
Context("when no total memory is supplied", func() {
83-
BeforeEach(func() {
84-
totMemFlag = ""
85-
sizesFlag = "-memorySizes=stack:2m..,heap:30m..400m,permgen:10m..12m"
86-
})
87-
88-
It("allocates the minima", func() {
89-
Ω(cmdErr).ShouldNot(HaveOccurred(), "exit status")
90-
Ω(string(sErr)).Should(Equal(""), "stderr")
91-
Ω(strings.Split(string(sOut), " ")).Should(ConsistOf(
92-
"-Xmx30M",
93-
"-Xms30M",
94-
"-Xss2M",
95-
"-XX:MaxPermSize=10M",
96-
"-XX:PermSize=10M",
97-
), "stdout")
98-
})
99-
})
100-
101109
Context("using nothing but total memory parameter", func() {
102110
BeforeEach(func() {
103111
totMemFlag = "-totMemory=4g"

‎main.go‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ const (
3333
func main() {
3434

3535
// validateFlags() will exit on error
36-
memSize, weights, sizes, memSizeSpecified := flags.ValidateFlags()
36+
memSize, weights, sizes := flags.ValidateFlags()
3737

3838
switchFuns := switches.AllJreSwitchFuns
3939

@@ -43,16 +43,12 @@ func main() {
4343
os.Exit(1)
4444
}
4545

46-
if memSizeSpecified {
47-
if err = allocator.Balance(memSize); err != nil {
48-
fmt.Fprintf(os.Stderr, "Cannot balance memory: %s", err)
49-
os.Exit(1)
50-
}
51-
if warnings := allocator.GetWarnings(); len(warnings) != 0 {
52-
fmt.Fprintln(os.Stderr, strings.Join(warnings, "\n"))
53-
}
54-
} else {
55-
allocator.SetLowerBounds()
46+
if err = allocator.Balance(memSize); err != nil {
47+
fmt.Fprintf(os.Stderr, "Cannot balance memory: %s", err)
48+
os.Exit(1)
49+
}
50+
if warnings := allocator.GetWarnings(); len(warnings) != 0 {
51+
fmt.Fprintln(os.Stderr, strings.Join(warnings, "\n"))
5652
}
5753

5854
switches := allocator.Switches(switchFuns)

‎memory/allocator.go‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525

2626
type Allocator interface {
2727
Balance(memLimit MemSize) error // Balance allocations to buckets within memory limit
28-
SetLowerBounds() // Allocate smallest memories allowed
2928
Switches(switches.Funs) []string // Get selected memory switches from current allocations
3029
GetWarnings() []string // Get warnings (if balancing succeeded)
3130
}
@@ -56,6 +55,10 @@ const (
5655
// Balance memory between buckets, adjusting stack units, observing
5756
// constraints, and detecting memory wastage and default proximity.
5857
func (a *allocator) Balance(memLimit MemSize) error {
58+
if memLimit.LessThan(MemSize(kILO)) {
59+
return fmt.Errorf("Too little memory to allocate: %s", memLimit)
60+
}
61+
5962
// adjust stack bucket, if it exists
6063
stackBucket, estNumThreads := a.normaliseStack(memLimit)
6164

@@ -73,12 +76,6 @@ func (a *allocator) Balance(memLimit MemSize) error {
7376
return nil
7477
}
7578

76-
func (a *allocator) SetLowerBounds() {
77-
for _, b := range a.buckets {
78-
b.SetSize(b.Range().Floor())
79-
}
80-
}
81-
8279
func (a *allocator) Switches(sfs switches.Funs) []string {
8380
var strs = make([]string, 0, 10)
8481
for s, b := range a.buckets {

‎memory/allocator_test.go‎

Lines changed: 9 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ package memory_test
1717

1818
import (
1919
"github.com/cloudfoundry/java-buildpack-memory-calculator/memory"
20-
"github.com/cloudfoundry/java-buildpack-memory-calculator/memory/switches"
2120

2221
. "github.com/onsi/ginkgo"
2322
. "github.com/onsi/gomega"
@@ -59,7 +58,6 @@ var _ = Describe("Allocator", func() {
5958
Context("constructor", func() {
6059

6160
Context("with good parameters", func() {
62-
6361
BeforeEach(func() {
6462
sizes = strmap{
6563
"stack": "2m",
@@ -87,26 +85,6 @@ var _ = Describe("Allocator", func() {
8785
"Bucket{name: native, size: <nil>, range: 0.., weight: 1}",
8886
))
8987
})
90-
91-
It("sets lower bounds and reports switches correctly", func() {
92-
a.SetLowerBounds()
93-
94-
Ω(memory.GetBuckets(a)).Should(ConsistOf(
95-
"Bucket{name: stack, size: 2M, range: 2M..2M, weight: 1}",
96-
"Bucket{name: heap, size: 30M, range: 30M.., weight: 5}",
97-
"Bucket{name: permgen, size: 10M, range: 10M..10M, weight: 3}",
98-
"Bucket{name: native, size: 0, range: 0.., weight: 1}",
99-
))
100-
101-
sws := a.Switches(switches.AllJreSwitchFuns)
102-
Ω(sws).Should(ConsistOf(
103-
"-Xmx30M",
104-
"-Xms30M",
105-
"-XX:MaxPermSize=10M",
106-
"-XX:PermSize=10M",
107-
"-Xss2M",
108-
)) // heap, permgen, stack
109-
})
11088
})
11189
})
11290

@@ -136,6 +114,15 @@ var _ = Describe("Allocator", func() {
136114
It("fails", func() {})
137115
})
138116

117+
Context("with no memory and no buckets", func() {
118+
BeforeEach(func() {
119+
sizes = strmap{"heap": "0.."}
120+
weights = floatmap{}
121+
memLimit = memory.MEMSIZE_ZERO
122+
})
123+
It("fails", func() {})
124+
})
125+
139126
Context("with not enough memory and one bucket", func() {
140127
BeforeEach(func() {
141128
sizes = strmap{"heap": "64m.."}
@@ -182,17 +169,6 @@ var _ = Describe("Allocator", func() {
182169
})
183170
})
184171

185-
Context("with no memory and no buckets", func() {
186-
BeforeEach(func() {
187-
sizes = strmap{"heap": "0.."}
188-
weights = floatmap{}
189-
memLimit = memory.MEMSIZE_ZERO
190-
})
191-
It("results in no buckets", func() {
192-
Ω(memory.GetBuckets(a)).Should(BeEmpty())
193-
})
194-
})
195-
196172
Context("with single bucket to 'balance'", func() {
197173

198174
BeforeEach(func() {

0 commit comments

Comments
 (0)