Fix: should not add emptyDir sizeLimit conf if it is nil

kubeflow / spark-operator

Kubernetes operator for managing the lifecycle of Apache Spark applications on Kubernetes.

Apache License 2.0

2.8k stars 1.38k forks source link

Fix: should not add emptyDir sizeLimit conf if it is nil #2305

Closed ChenYi015 closed 2 weeks ago

ChenYi015 commented 2 weeks ago

Purpose of this PR

Close #2199

Proposed changes:

When running spark-submit, the emptyDir sizeLimit conf should not be added if sizeLimit is nil.

Change Category

[x] 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

Checklist

[x] 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

ChenYi015 commented 2 weeks ago

/assign @jacobsalway @ImpSy

jacobsalway commented 2 weeks ago

/lgtm

Tested locally on Kind to reproduce bug on master and confirmed the fix works

ChenYi015 commented 2 weeks ago

/approve

google-oss-prow[bot] commented 2 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChenYi015, Cian911

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubeflow/spark-operator/blob/master/OWNERS)~~ [ChenYi015] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

jacobsalway commented 2 weeks ago

On a side note: I added the original change to support sizeLimit on emptyDir volumes so this looks like it's a bug introduced in the controller-runtime rewrite.

https://github.com/kubeflow/spark-operator/pull/1993

Cian911 commented 2 weeks ago

Amazing, thanks @ChenYi015 & @jacobsalway for this.

Is it possible you can do a new rc- release so I can better test this internally?