[vikas-saxena02]

Purpose of this PR

internal/scheduler/yunikorn/resourceusage had 16.5% statement coverage. Only byteStringAsBytes, bytesToMi and cpuRequest had direct tests. The entire memory calculation chain — the part that decides what CPU and memory YuniKorn actually reserves for driver and executor pods — was exercised only indirectly, through TestSchedule in the parent package.

This adds direct table tests for that chain. Coverage goes from 16.5% to 97.8%. No production code is touched.

Part of #3202.

Proposed changes:

• Add tests to memory_test.go for isJavaApp, getMemoryOverheadFactor, memoryRequestBytes, executorPysparkMemoryBytes, sparkOffHeapMemoryBytes, driverMemoryRequest and executorMemoryRequest, including their error paths.
• Add tests to resource_usage_test.go for DriverPodRequests and ExecutorPodRequests, covering defaults, coreRequest precedence over cores, and error propagation.

Change Category

• Bugfix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that could affect existing functionality)
• Documentation update

Rationale

These functions encode Spark's memory-overhead semantics, which are easy to get wrong and currently unpinned by any test. The cases added lock down:

• the 384Mi minimum overhead floor, and the boundary where the proportional factor overtakes it;
• the JVM (0.1) versus non-JVM (0.4) default overhead factors, and the fact that Java and Scala are the JVM types;
• precedence of an explicit memoryOverhead and memoryOverheadFactor over the type-derived defaults;
• the mebibyte default applied to a suffixless spark.executor.pyspark.memory, matching Spark's documented behaviour;
• addition of PySpark and off-heap memory to the executor request;
• the floor-to-MiB conversion that mirrors Spark's BasicDriverFeatureStep / BasicExecutorFeatureStep.

Expected values were derived from the Spark semantics rather than from current output, so these are regression tests, not a snapshot of existing behaviour.

Checklist

• I have conducted a self-review of my own code.
• I have updated documentation accordingly.
• I have added tests that prove my changes are effective or that my feature works.
• Existing unit tests pass locally with my changes.

Additional Notes

Local run:

go test ./internal/scheduler/yunikorn/resourceusage/... -cover
ok  github.com/kubeflow/spark-operator/v2/internal/scheduler/yunikorn/resourceusage  0.548s  coverage: 97.8% of statements

The residual ~2% is the strconv.ParseInt error branch in byteStringAsBytes, which is unreachable because the preceding regex already guarantees the capture group is digits.

@nabuskey @vjanelle @tariq-hasan One observation found while writing these, not addressed here: sparkOffHeapMemoryBytes compares spark.memory.offHeap.enabled against the exact string "false", so "False" or "FALSE" would be treated as enabled. Spark parses this case-insensitively. I deliberately did not add a test for it, since that would pin the current behaviour rather than flag it. Happy to open a separate issue if maintainers agree it's worth fixing.

[vjanelle]

In general we should match the behaviour Java has?

[vikas-saxena02]

@nabuskey @vjanelle @tariq-hasan While reviewing this PR with Claude, it flagged that internal/webhook/resourcequota.go re-implements the same memory-overhead maths this PR covers, and that the two copies have diverged. I've checked each point against current master and they hold up. Raising them here rather than fixing, since it's production code and out of scope for a test-only PR.

1. Scala applications get a different overhead factor.
resourcequota.go:165 selects the factor with app.Spec.Type == v1beta2.SparkApplicationTypeJava, whereas resourceusage/memory.go:39 uses isJavaApp(), which is Java || Scala. A Scala application therefore gets the non-JVM factor (0.4) when the webhook validates against a ResourceQuota, and the JVM factor (0.1) on the YuniKorn path.

2. The webhook's byte parser is unanchored.
resourcequota.go:48 uses ([0-9]+)([a-z]+)?; resourceusage/java.go:41 uses ^([0-9]+)([a-z]+)?$. Inputs such as 600gb This breaks are rejected by the latter and silently accepted by the former. The existing TestByteStringAsMbInvalid already asserts that these must be rejected.

3. Fractional memory values are unreachable.
parseJavaMemoryString has a javaFractionStringPattern branch at line 234, and its error message at line 244 advertises 1.5mb as valid. But the unanchored integer pattern matches first on 1.5g — capturing 1 with an empty suffix, which isn't in javaStringSuffixes — so the function falls through and returns the error before the fraction branch is ever reached.

4. None of this is covered. resourcequota_test.go currently tests only parseJavaMemoryString. getSparkPodMemoryRequests, getMemoryRequests, validateResourceQuota and the cores helpers have no direct tests.

@vjanelle this may be the same question you raised about matching Java's behaviour — the off-heap string comparison looks like one instance of a broader pattern rather than a one-off.

Happy to open an issue for the behaviour differences, and to follow up with tests for internal/webhook/resourcequota.go under #3202 — given the overlap it's the natural next package. Let me know which you'd prefer.

[vikas-saxena02]

@nabuskey can you plz review/approve this as well. @vjanelle already approved it

[vjanelle]

/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vjanelle
Once this PR has been reviewed and has the lgtm label, please assign nabuskey for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[google-oss-prow]

New changes are detected. LGTM label has been removed.

[vikas-saxena02]

@nabuskey @vjanelle can you please review this again