diff --git a/RUBY_VS_GO_BUILDPACK_COMPARISON.md b/RUBY_VS_GO_BUILDPACK_COMPARISON.md index aba4a4922..6fe683e55 100644 --- a/RUBY_VS_GO_BUILDPACK_COMPARISON.md +++ b/RUBY_VS_GO_BUILDPACK_COMPARISON.md @@ -2191,12 +2191,40 @@ dependencies: | **APM Agents** | ✅ 15 agents | ✅ 14 agents | Missing: Google Stackdriver Debugger (deprecated) | | **Security Providers** | ✅ 6 | ✅ 6 | Identical | | **Database JDBC Injection** | ✅ | ✅ | Identical | -| **Memory Calculator** | ✅ | ✅ | Identical | +| **Memory Calculator** | ✅ v3.13.0 | ✅ v4.2.0 | **Behaviour change** — see below | | **JVMKill Agent** | ✅ | ✅ | Identical | | **Custom JRE Repositories** | ✅ Runtime config | ❌ Requires fork | Breaking change | | **Multi-buildpack** | ⚠️ Via framework | ✅ Native V3 | Go improvement | | **Configuration Overrides** | ✅ | ✅ | Identical (JBP_CONFIG_*) | +#### Memory Calculator Behaviour Change (v3 → v4) + +The memory calculator was upgraded from **v3.13.0 to v4.2.0**. The difference only affects apps with an **explicit `-Xmx`** in `JAVA_OPTS` (setting `-Xmx` explicitly in containerised environments is generally considered bad practice — the calculator sizes heap better automatically). How a pinned `-Xmx` is handled: + +| | v3.13.0 (Ruby) | v4.2.0 (Go) | +|--|----------------|-------------| +| Memory check | `non-heap > total` | `non-heap + heap > total` | +| Non-heap when `-Xmx` set | Squeezed to `total − Xmx` | Calculated independently | +| `-Xmx512M`, container=750M | ✅ passes | ❌ fails | + +When `-Xmx` is not set, both v3 and v4 size heap and non-heap to fit within the container — no difference. When `-Xmx` is pinned, v4 requires the container to fit both heap and non-heap (thread stacks + metaspace + code cache). v3 squeezed non-heap into whatever remained after `-Xmx`, claiming less total memory — at the cost of potentially undersized thread stacks and metaspace at runtime. Apps that fit in smaller containers with v3 may fail at startup with v4: + +``` +required memory 1269289K is greater than 750M available for allocation +``` + +**Migration options**: + +1. **Lower `stack_threads`** *(only if your app uses fewer than 250 threads)*: 250 threads × ~1M = ~250M native memory. Reducing this alone is often enough to fit within the container: + ```yaml + env: + JBP_CONFIG_OPEN_JDK_JRE: '{ memory_calculator: { stack_threads: 50 } }' + ``` + +2. **Remove `-Xmx` from `JAVA_OPTS`** — let the calculator size heap automatically. Note: removing `-Xmx` avoids the fixed-heap check but does not reduce total memory need. You will likely still need to increase `memory:` in `manifest.yml` so the calculator has enough room to allocate adequate heap. + +3. **Increase manifest memory** — raise `memory:` to fit `Xmx + non-heap`. Based on the error above (`1269289K ≈ 1240M`), set at least **1300M** for a `-Xmx512M` app with default settings. + ### 10.3 Adoption Recommendations **✅ RECOMMENDED for**: diff --git a/src/java/jres/memory_calculator.go b/src/java/jres/memory_calculator.go index b1777ce62..b08b03cfc 100644 --- a/src/java/jres/memory_calculator.go +++ b/src/java/jres/memory_calculator.go @@ -21,9 +21,13 @@ type MemoryCalculator struct { javaMajorVersion int calculatorPath string version string - classCount int - stackThreads int - headroom int + classCount int + stackThreads int + headroom int + configLoaded bool + classCountUserSet bool + stackThreadsUserSet bool + headroomUserSet bool } // NewMemoryCalculator creates a new memory calculator @@ -54,6 +58,8 @@ func (m *MemoryCalculator) Supply() error { m.version = dep.Version m.ctx.Log.Info("Installing Memory Calculator (%s)", m.version) + m.LoadConfig() + // Create bin directory binDir := filepath.Join(m.jreDir, "bin") if err := os.MkdirAll(binDir, 0755); err != nil { @@ -107,14 +113,15 @@ func (m *MemoryCalculator) Supply() error { m.calculatorPath = finalPath - // Count classes in the application - if err := m.countClasses(); err != nil { - m.ctx.Log.Warning("Failed to count classes: %s (using default)", err.Error()) - m.classCount = 0 // Will be calculated as 35% of actual later + // Count classes in the application, unless overridden by config + if m.classCount == 0 { + if err := m.countClasses(); err != nil { + m.ctx.Log.Warning("Failed to count classes: %s (using default)", err.Error()) + } } - m.ctx.Log.Info("Memory Calculator installed: Loaded Classes: %d, Threads: %d", - m.classCount, m.stackThreads) + m.ctx.Log.Info("Memory Calculator installed: Loaded Classes: %s, Threads: %s, Headroom: %s", + m.classCountDisplay(), m.stackThreadsDisplay(), m.headroomDisplay()) // Clean up temp directory os.RemoveAll(tempDir) @@ -152,6 +159,8 @@ func (m *MemoryCalculator) detectInstalledCalculator() { // Finalize configures the memory calculator in the startup command func (m *MemoryCalculator) Finalize() error { + m.LoadConfig() + // If calculatorPath not set, try to detect it from previous installation if m.calculatorPath == "" { m.detectInstalledCalculator() @@ -364,24 +373,73 @@ func (m *MemoryCalculator) convertToRuntimePath(stagingPath string) string { return fmt.Sprintf("/home/vcap/deps/%s/jre/bin/%s", depsIdx, filename) } -// LoadConfig loads memory calculator configuration from environment/config +// openJDKJREConfig mirrors the memory_calculator section of JBP_CONFIG_OPEN_JDK_JRE. +type openJDKJREConfig struct { + MemoryCalculator memoryCalculatorConfig `yaml:"memory_calculator"` +} + +type memoryCalculatorConfig struct { + StackThreads int `yaml:"stack_threads"` + ClassCount int `yaml:"class_count"` + Headroom int `yaml:"headroom"` +} + +// LoadConfig loads memory calculator configuration from JBP_CONFIG_OPEN_JDK_JRE +// (standard CF format) and falls back to MEMORY_CALCULATOR_* env vars. +// Must be called at the start of Supply(), before countClasses(). func (m *MemoryCalculator) LoadConfig() { - // Check for environment overrides - // JBP_CONFIG_OPEN_JDK_JRE='{memory_calculator: {stack_threads: 300}}' + if m.configLoaded { + return + } + m.configLoaded = true + if config := os.Getenv("JBP_CONFIG_OPEN_JDK_JRE"); config != "" { + yamlHandler := common.YamlHandler{} + + // Extract raw memory_calculator sub-section to validate its fields separately, + // so unknown top-level keys (e.g. jre:) are silently ignored while typos + // inside memory_calculator: are warned about. + rawCfg := struct { + MC interface{} `yaml:"memory_calculator"` + }{} + if err := yamlHandler.Unmarshal([]byte(config), &rawCfg); err == nil && rawCfg.MC != nil { + if mcBytes, err := yamlHandler.Marshal(rawCfg.MC); err == nil { + if err := yamlHandler.ValidateFields(mcBytes, &memoryCalculatorConfig{}); err != nil { + m.ctx.Log.Warning("Unknown fields in JBP_CONFIG_OPEN_JDK_JRE memory_calculator: %s", err.Error()) + } + } + } - // For now, using defaults - // In production, we'd parse JSON from environment variables + cfg := openJDKJREConfig{} + if err := yamlHandler.Unmarshal([]byte(config), &cfg); err != nil { + m.ctx.Log.Warning("Failed to parse JBP_CONFIG_OPEN_JDK_JRE: %s", err.Error()) + } else { + mc := cfg.MemoryCalculator + if mc.StackThreads > 0 { + m.stackThreads = mc.StackThreads + m.stackThreadsUserSet = true + } + if mc.ClassCount > 0 { + m.classCount = mc.ClassCount + m.classCountUserSet = true + } + if mc.Headroom > 0 { + m.headroom = mc.Headroom + m.headroomUserSet = true + } + } + } - // Check specific environment variables if val := os.Getenv("MEMORY_CALCULATOR_STACK_THREADS"); val != "" { if threads, err := strconv.Atoi(val); err == nil { m.stackThreads = threads + m.stackThreadsUserSet = true } } if val := os.Getenv("MEMORY_CALCULATOR_HEADROOM"); val != "" { if headroom, err := strconv.Atoi(val); err == nil { m.headroom = headroom + m.headroomUserSet = true } } } @@ -395,6 +453,27 @@ func copyFile(src, dst string) error { return os.WriteFile(dst, data, 0755) } +func (m *MemoryCalculator) classCountDisplay() string { + if m.classCountUserSet { + return fmt.Sprintf("%d", m.classCount) + } + return fmt.Sprintf("%d (auto-detected)", m.classCount) +} + +func (m *MemoryCalculator) stackThreadsDisplay() string { + if m.stackThreadsUserSet { + return fmt.Sprintf("%d", m.stackThreads) + } + return fmt.Sprintf("%d (default)", m.stackThreads) +} + +func (m *MemoryCalculator) headroomDisplay() string { + if m.headroomUserSet { + return fmt.Sprintf("%d%%", m.headroom) + } + return fmt.Sprintf("%d%% (default)", m.headroom) +} + // RunMemoryCalculator runs the memory calculator and returns the calculated JAVA_OPTS // This is primarily for testing func (m *MemoryCalculator) RunMemoryCalculator(memoryLimit string) (string, error) { diff --git a/src/java/jres/memory_calculator_issues_test.go b/src/java/jres/memory_calculator_issues_test.go new file mode 100644 index 000000000..b4621f961 --- /dev/null +++ b/src/java/jres/memory_calculator_issues_test.go @@ -0,0 +1,233 @@ +package jres_test + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "github.com/cloudfoundry/java-buildpack/src/java/common" + "github.com/cloudfoundry/java-buildpack/src/java/jres" + "github.com/cloudfoundry/libbuildpack" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// These tests document and demonstrate known regressions introduced by the +// migration from the Ruby buildpack (memory calculator v3) to the Go buildpack +// (memory calculator v4). Each test is expected to FAIL until the issue is fixed. +// +// Tracked in: https://github.com/cloudfoundry/java-buildpack/issues/1257 + +var _ = Describe("Memory Calculator Issues", func() { + var ( + buildDir string + depsDir string + cacheDir string + ctx *common.Context + jreDir string + logBuffer *bytes.Buffer + ) + + BeforeEach(func() { + var err error + buildDir, err = os.MkdirTemp("", "mc-issues-build") + Expect(err).NotTo(HaveOccurred()) + + depsDir, err = os.MkdirTemp("", "mc-issues-deps") + Expect(err).NotTo(HaveOccurred()) + + cacheDir, err = os.MkdirTemp("", "mc-issues-cache") + Expect(err).NotTo(HaveOccurred()) + + Expect(os.MkdirAll(depsDir+"/0", 0755)).To(Succeed()) + + logBuffer = &bytes.Buffer{} + logger := libbuildpack.NewLogger(logBuffer) + manifest := &libbuildpack.Manifest{} + installer := &libbuildpack.Installer{} + stager := libbuildpack.NewStager([]string{buildDir, cacheDir, depsDir, "0"}, logger, manifest) + command := &libbuildpack.Command{} + + ctx = &common.Context{ + Stager: stager, + Manifest: manifest, + Installer: installer, + Log: logger, + Command: command, + } + + jreDir = filepath.Join(depsDir, "0", "jre") + Expect(os.MkdirAll(filepath.Join(jreDir, "bin"), 0755)).To(Succeed()) + }) + + AfterEach(func() { + os.RemoveAll(buildDir) + os.RemoveAll(depsDir) + os.RemoveAll(cacheDir) + }) + + // fakeBinary writes a placeholder file that detectInstalledCalculator() will find. + // Required for tests that go through Finalize() or GetCalculatorCommand(), + // since both return early if no calculator binary is detected. + fakeBinary := func(version string) { + name := fmt.Sprintf("java-buildpack-memory-calculator-%s", version) + path := filepath.Join(jreDir, "bin", name) + Expect(os.WriteFile(path, []byte("#!/bin/sh\n"), 0755)).To(Succeed()) + } + + // readGeneratedScript returns the content of the generated memory_calculator.sh. + readGeneratedScript := func() string { + path := filepath.Join(depsDir, "0", "bin", "memory_calculator.sh") + data, err := os.ReadFile(path) + Expect(err).NotTo(HaveOccurred()) + return string(data) + } + + // ------------------------------------------------------------------------- + // https://github.com/cloudfoundry/java-buildpack/issues/1257 + // LoadConfig() appears not to be called — MEMORY_CALCULATOR_* env vars silently ignored + // + // LoadConfig() reads MEMORY_CALCULATOR_STACK_THREADS and MEMORY_CALCULATOR_HEADROOM + // but appears not to be invoked during Supply() or Finalize(). Teams cannot + // tune stack_threads or headroom to work around the memory regression. + // + // Expected fix: call LoadConfig() at the start of Supply() (before countClasses()), + // so both stack_threads and headroom overrides are in effect before the script + // is built in Finalize(). + // ------------------------------------------------------------------------- + Describe("#1257: LoadConfig() never called, MEMORY_CALCULATOR_* env vars silently ignored", func() { + It("MEMORY_CALCULATOR_STACK_THREADS env var reduces thread count in generated script", func() { + DeferCleanup(os.Unsetenv, "MEMORY_CALCULATOR_STACK_THREADS") + Expect(os.Setenv("MEMORY_CALCULATOR_STACK_THREADS", "50")).To(Succeed()) + + fakeBinary("4.2.0") + mc := jres.NewMemoryCalculator(ctx, jreDir, "17.0.9", 17) + Expect(mc.Finalize()).To(Succeed()) + + script := readGeneratedScript() + + // This assertion FAILS until LoadConfig() is called at the start of Supply(): + // currently the script will contain --thread-count=250 (the default). + Expect(script).To(ContainSubstring("--thread-count=50"), + "expected --thread-count=50 from MEMORY_CALCULATOR_STACK_THREADS env var, "+ + "but LoadConfig() appears not to be called so the override is silently ignored.\nScript:\n%s", script) + }) + + It("MEMORY_CALCULATOR_HEADROOM env var applies headroom in generated script", func() { + DeferCleanup(os.Unsetenv, "MEMORY_CALCULATOR_HEADROOM") + Expect(os.Setenv("MEMORY_CALCULATOR_HEADROOM", "5")).To(Succeed()) + + fakeBinary("4.2.0") + mc := jres.NewMemoryCalculator(ctx, jreDir, "17.0.9", 17) + Expect(mc.Finalize()).To(Succeed()) + + script := readGeneratedScript() + + // This assertion FAILS until LoadConfig() is called at the start of Supply(): + // currently no --head-room flag appears because headroom defaults to 0. + Expect(script).To(ContainSubstring("--head-room=5"), + "expected --head-room=5 from MEMORY_CALCULATOR_HEADROOM env var, "+ + "but LoadConfig() appears not to be called.\nScript:\n%s", script) + }) + }) + + // ------------------------------------------------------------------------- + // https://github.com/cloudfoundry/java-buildpack/issues/1257 + // JBP_CONFIG_OPEN_JDK_JRE env var is not parsed + // + // The CF convention for tuning the memory calculator is: + // JBP_CONFIG_OPEN_JDK_JRE: '{ memory_calculator: { stack_threads: 50 } }' + // + // Two bugs appear to prevent this from working: + // a) LoadConfig() appears not to be called during Supply() or Finalize() + // b) LoadConfig() reads MEMORY_CALCULATOR_STACK_THREADS instead of + // parsing JBP_CONFIG_OPEN_JDK_JRE + // + // Note: class_count override must be loaded before countClasses() in Supply(); + // stack_threads must be loaded before buildCalculatorCommand() in Finalize(). + // Therefore LoadConfig() should be called at the start of Supply(). + // + // Reducing stack_threads from 250 → 50 saves 200M of stack, which is the + // primary mitigation available to teams hitting the 750M regression. + // ------------------------------------------------------------------------- + Describe("#1257: JBP_CONFIG_OPEN_JDK_JRE is not parsed, stack_threads override silently ignored", func() { + It("stack_threads set via JBP_CONFIG_OPEN_JDK_JRE is reflected in generated script", func() { + DeferCleanup(os.Unsetenv, "JBP_CONFIG_OPEN_JDK_JRE") + Expect(os.Setenv("JBP_CONFIG_OPEN_JDK_JRE", + "{ memory_calculator: { stack_threads: 50 } }")).To(Succeed()) + + fakeBinary("4.2.0") + mc := jres.NewMemoryCalculator(ctx, jreDir, "17.0.9", 17) + Expect(mc.Finalize()).To(Succeed()) + + script := readGeneratedScript() + + // This assertion FAILS until JBP_CONFIG_OPEN_JDK_JRE is parsed: + // currently the script will contain --thread-count=250 (the default). + // Fix: parse JBP_CONFIG_OPEN_JDK_JRE in LoadConfig() and call it at + // the start of Supply(). With 50 threads: stack = 50M instead of 250M + // → saves 200M, making 750M containers viable again. + Expect(script).To(ContainSubstring("--thread-count=50"), + "expected --thread-count=50 from JBP_CONFIG_OPEN_JDK_JRE but got default 250.\n"+ + "Fix: parse JBP_CONFIG_OPEN_JDK_JRE in LoadConfig() and call it at start of Supply().\nScript:\n%s", + script) + }) + + It("class_count set via JBP_CONFIG_OPEN_JDK_JRE overrides calculated count", func() { + DeferCleanup(os.Unsetenv, "JBP_CONFIG_OPEN_JDK_JRE") + Expect(os.Setenv("JBP_CONFIG_OPEN_JDK_JRE", + "{ memory_calculator: { class_count: 3500 } }")).To(Succeed()) + + fakeBinary("4.2.0") + mc := jres.NewMemoryCalculator(ctx, jreDir, "17.0.9", 17) + Expect(mc.Finalize()).To(Succeed()) + + script := readGeneratedScript() + + // This assertion FAILS until JBP_CONFIG_OPEN_JDK_JRE is parsed. + // class_count must be loaded before countClasses() runs in Supply(). + // class_count=3500 keeps metaspace at ~34M instead of ~233M. + Expect(script).To(ContainSubstring("--loaded-class-count=3500"), + "expected --loaded-class-count=3500 from JBP_CONFIG_OPEN_JDK_JRE.\nScript:\n%s", + script) + }) + }) + + // ------------------------------------------------------------------------- + // JBP_CONFIG_OPEN_JDK_JRE field validation: + // - Unknown top-level fields (e.g. jre:) must be silently ignored + // - Unknown fields inside memory_calculator: must produce a WARNING (typo guard) + // ------------------------------------------------------------------------- + Describe("JBP_CONFIG_OPEN_JDK_JRE field validation", func() { + It("applies memory_calculator settings and logs no WARNING when jre: is also present", func() { + DeferCleanup(os.Unsetenv, "JBP_CONFIG_OPEN_JDK_JRE") + Expect(os.Setenv("JBP_CONFIG_OPEN_JDK_JRE", + `{ jre: { version: "25.+" }, memory_calculator: { headroom: 5, stack_threads: 50 } }`)).To(Succeed()) + + fakeBinary("4.2.0") + mc := jres.NewMemoryCalculator(ctx, jreDir, "17.0.9", 17) + Expect(mc.Finalize()).To(Succeed()) + + script := readGeneratedScript() + Expect(script).To(ContainSubstring("--thread-count=50")) + Expect(script).To(ContainSubstring("--head-room=5")) + Expect(logBuffer.String()).NotTo(ContainSubstring("WARNING"), + "unexpected WARNING in log output:\n%s", logBuffer.String()) + }) + + It("logs a WARNING when memory_calculator contains an unknown field (e.g. typo)", func() { + DeferCleanup(os.Unsetenv, "JBP_CONFIG_OPEN_JDK_JRE") + Expect(os.Setenv("JBP_CONFIG_OPEN_JDK_JRE", + `{ memory_calculator: { stack_thread: 50 } }`)).To(Succeed()) // typo: missing 's' + + fakeBinary("4.2.0") + mc := jres.NewMemoryCalculator(ctx, jreDir, "17.0.9", 17) + Expect(mc.Finalize()).To(Succeed()) + + Expect(logBuffer.String()).To(ContainSubstring("WARNING"), + "expected WARNING for unknown field 'stack_thread' in memory_calculator") + Expect(logBuffer.String()).To(ContainSubstring("stack_thread"), + "WARNING should mention the unknown field name") + }) + }) +})