[parth-patel010]

Purpose of this PR

With resource quota enforcement enabled, the webhook computed a SparkApplication's CPU requests from cores only and ignored driver.coreRequest / executor.coreRequest. Spark sets the pod CPU request from spark.kubernetes.{driver,executor}.request.cores when it's given (that's what coreRequest maps to) and only falls back to spark.{driver,executor}.cores otherwise. So apps like cores: 4, coreRequest: 500m were rejected for needing 4 CPUs per pod, and apps with coreRequest above cores got past the check.

Fixes #3278

Proposed changes:

• getSparkPodCoresRequests now takes the pod's coreRequest and, when it is set, parses it as a quantity and uses it for cpu / requests.cpu. Otherwise it keeps using cores (or the default of 1), as before. This matches what internal/scheduler/yunikorn/resourceusage already does.
• An unparsable coreRequest returns an error (failed to parse coreRequest "one": ...), so the webhook rejects the app instead of silently falling back to cores. This is the same way an invalid coreLimit is already handled.
• limits.cpu is not changed. It is still based on coreLimit, then cores.

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

The quota check should count what the pods will actually request. Apps that don't set coreRequest get exactly the same numbers as before.

Checklist

• I have conducted a self-review of my own code.
• I have updated documentation accordingly. (not needed, no user-facing docs describe how the quota is computed)
• 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

New tests

• TestGetResourceListCoreRequest checks cpu, requests.cpu and limits.cpu in four cases: cores only, coreRequest below cores, coreRequest above cores, and a decimal coreRequest (0.5).
• TestGetResourceListInvalidCoreRequest checks that an invalid coreRequest returns an error.
• Two validator tests run against a requests.cpu: 3 quota. In the first, the app needs 2.5 CPUs of requests but has 12 cores, and it must be admitted. In the second, the app needs 6 CPUs of requests but has 3 cores, and it must be rejected.

Before (master d016731 + the new tests):

--- FAIL: TestGetResourceListCoreRequest (0.00s)
    --- FAIL: TestGetResourceListCoreRequest/coreRequest_lower_than_cores (0.00s)
        resourcequota_test.go:107: cpu: expected 2500m, got 12
        resourcequota_test.go:107: requests.cpu: expected 2500m, got 12
    --- FAIL: TestGetResourceListCoreRequest/coreRequest_higher_than_cores (0.00s)
        resourcequota_test.go:107: cpu: expected 6, got 3
        resourcequota_test.go:107: requests.cpu: expected 6, got 3
    --- FAIL: TestGetResourceListCoreRequest/decimal_coreRequest (0.00s)
        resourcequota_test.go:107: cpu: expected 2500m, got 4
        resourcequota_test.go:107: requests.cpu: expected 2500m, got 4
--- FAIL: TestGetResourceListInvalidCoreRequest (0.00s)
    resourcequota_test.go:125: expected an error for an invalid executor coreRequest
--- FAIL: TestSparkApplicationValidatorValidateCreate_ResourceQuotaUsesCoreRequest (0.07s)
    sparkapplication_validator_test.go:252: expected quota satisfied, got failed to validate resource quota "default/cpu-requests"
--- FAIL: TestSparkApplicationValidatorValidateCreate_ResourceQuotaExceededByCoreRequest (0.00s)
    sparkapplication_validator_test.go:266: expected resource quota validation error, got <nil>
FAIL	github.com/kubeflow/spark-operator/v2/internal/webhook	0.090s

After:

--- PASS: TestGetResourceListCoreRequest (0.00s)
    --- PASS: TestGetResourceListCoreRequest/cores_only (0.00s)
    --- PASS: TestGetResourceListCoreRequest/coreRequest_lower_than_cores (0.00s)
    --- PASS: TestGetResourceListCoreRequest/coreRequest_higher_than_cores (0.00s)
    --- PASS: TestGetResourceListCoreRequest/decimal_coreRequest (0.00s)
--- PASS: TestGetResourceListInvalidCoreRequest (0.00s)
--- PASS: TestSparkApplicationValidatorValidateCreate_ResourceQuotaUsesCoreRequest (0.05s)
--- PASS: TestSparkApplicationValidatorValidateCreate_ResourceQuotaExceededByCoreRequest (0.00s)
ok  	github.com/kubeflow/spark-operator/v2/internal/webhook	0.073s

Full unit test suite (same packages as make unit-test, envtest 1.35.0):

ok  	github.com/kubeflow/spark-operator/v2/api/v1beta2	0.020s
ok  	github.com/kubeflow/spark-operator/v2/internal/controller/scheduledsparkapplication	8.259s
ok  	github.com/kubeflow/spark-operator/v2/internal/controller/sparkapplication	10.653s
ok  	github.com/kubeflow/spark-operator/v2/internal/controller/sparkconnect	7.309s
ok  	github.com/kubeflow/spark-operator/v2/internal/scheduler	0.024s
ok  	github.com/kubeflow/spark-operator/v2/internal/scheduler/kubescheduler	0.097s
ok  	github.com/kubeflow/spark-operator/v2/internal/scheduler/volcano	0.016s
ok  	github.com/kubeflow/spark-operator/v2/internal/scheduler/yunikorn	0.016s
ok  	github.com/kubeflow/spark-operator/v2/internal/scheduler/yunikorn/resourceusage	0.009s
ok  	github.com/kubeflow/spark-operator/v2/internal/webhook	5.728s
ok  	github.com/kubeflow/spark-operator/v2/pkg/certificate	6.385s
ok  	github.com/kubeflow/spark-operator/v2/pkg/features	0.007s
ok  	github.com/kubeflow/spark-operator/v2/pkg/tls	0.004s
ok  	github.com/kubeflow/spark-operator/v2/pkg/util	0.131s
ok  	github.com/kubeflow/spark-operator/v2/test/kustomize	0.009s

golangci-lint run ./internal/webhook/... reports 0 issues, and go vet and gofmt are clean.

This touches the same file as #3258 and #3269, but different functions (the CPU path rather than the memory path), so they don't overlap.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andreyvelich 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

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.