Skip to content

Fix: should not add emptyDir sizeLimit conf on executor pods if it is nil#2316

Merged
google-oss-prow[bot] merged 1 commit intokubeflow:masterfrom
Cian911:cian911-emptydir-executor-bug
Nov 11, 2024
Merged

Fix: should not add emptyDir sizeLimit conf on executor pods if it is nil#2316
google-oss-prow[bot] merged 1 commit intokubeflow:masterfrom
Cian911:cian911-emptydir-executor-bug

Conversation

@Cian911
Copy link
Contributor

@Cian911 Cian911 commented Nov 10, 2024

Purpose of this PR

Proposed changes:

This is a follow up fix from work already done in #2305 by @ChenYi015

This PR adds a check at submission on executors pods if an emptyDir sizeLimit is nil.

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

Following some further testing of an older SparkApplication template wherein the volumeName (see issue #2199 for more details) had not been changed, I noticed behaviour wherein only the driver pod would get created and no executor pods would spawn. I believe this is due to the same issue outlined in #2305 which would make sense, as the check was only added on the driver pod submission, and not the executors.

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

@Cian911 Cian911 force-pushed the cian911-emptydir-executor-bug branch from 1f613c7 to 700c4e2 Compare November 10, 2024 22:12
@Cian911
fix: should not add emptyDir szeLimit conf on executors pods if nil
700c4e2 
Signed-off-by: Cian Gallagher <cian@ciangallagher.net>
@google-oss-prow
Copy link
Contributor

google-oss-prow bot commented Nov 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChenYi015

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@ChenYi015
Copy link
Member

ChenYi015 commented Nov 11, 2024

@Cian911 Thanks for fixing this.
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Nov 11, 2024
@google-oss-prow google-oss-prow bot merged commit 2999546 into kubeflow:master Nov 11, 2024
ChenYi015 pushed a commit to ChenYi015/spark-operator that referenced this pull request Dec 10, 2024
@Cian911 @ChenYi015
Fix: should not add emptyDir sizeLimit conf on executor pods if it is…
2dbb0ff 
… nil (kubeflow#2316)

Signed-off-by: Cian Gallagher <cian@ciangallagher.net>
(cherry picked from commit 2999546)
@ChenYi015 ChenYi015 mentioned this pull request Dec 10, 2024
Merged
google-oss-prow bot pushed a commit that referenced this pull request Dec 11, 2024
@ChenYi015 @Aransh
@npgretz @Cian911 @jacobsalway @dependabot @c-h-afzal @Tom-Newton @Aakcht
Release v2.1.0 (#2354)
664b9d0 
* Allow setting automountServiceAccountToken (#2298)

* Allow setting automountServiceAccountToken on workloads and serviceAccounts

Signed-off-by: Aran Shavit <Aranshavit@gmail.com>

* update helm docs

Signed-off-by: Aran Shavit <Aranshavit@gmail.com>

---------

Signed-off-by: Aran Shavit <Aranshavit@gmail.com>
(cherry picked from commit 515d805)

* Fix: executor container security context does not work (#2306)

Signed-off-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit 171e429)

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

Signed-off-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit 763682d)

* Allow the Controller and Webhook Containers to run with the securityContext: readOnlyRootfilesystem: true (#2282)

* create a tmp dir for the controller to write Spark artifacts to and set the controller to readOnlyRootFilesystem

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* mount a dir for the webhook container to generate its certificates in and set readOnlyRootFilesystem: true for the webhook pod

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* update the securityContext in the controller deployment test

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* update securityContext of the webhook container in the deployment_test

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* update README

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* remove -- so comments are not rendered in the README.md

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* recreate README.md after removal of comments for volumes and volumeMounts

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* make indentation for volumes and volumeMounts consistent with rest of values.yaml

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* Revert "make indentation for volumes and volumeMounts consistent with rest of values.yaml"

This reverts commit dba97fc.

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* fix indentation in webhook and controller deployment templates for volumes and volumeMounts

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/values.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/controller/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/controller/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/webhook/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* Update charts/spark-operator-chart/templates/webhook/deployment.yaml

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>

* add additional securityContext to the controller deployment_test.yaml

Signed-off-by: Nick Gretzon <npgretz@gmail.com>

---------

Signed-off-by: Nick Gretzon <npgretz@gmail.com>
Signed-off-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit 72107fd)

* Fix: should not add emptyDir sizeLimit conf on executor pods if it is nil (#2316)

Signed-off-by: Cian Gallagher <cian@ciangallagher.net>
(cherry picked from commit 2999546)

* Bump `volcano.sh/apis` to 1.10.0 (#2320)

Signed-off-by: Jacob Salway <jacob.salway@gmail.com>
(cherry picked from commit 22e4fb8)

* Truncate UI service name if over 63 characters (#2311)

* Truncate UI service name if over 63 characters

Signed-off-by: Jacob Salway <jacob.salway@gmail.com>

* Also truncate ingress name

Signed-off-by: Jacob Salway <jacob.salway@gmail.com>

---------

Signed-off-by: Jacob Salway <jacob.salway@gmail.com>
(cherry picked from commit 43c1888)

* Bump aquasecurity/trivy-action from 0.28.0 to 0.29.0 (#2332)

Bumps [aquasecurity/trivy-action](https://github.com/aquasecurity/trivy-action) from 0.28.0 to 0.29.0.
- [Release notes](https://github.com/aquasecurity/trivy-action/releases)
- [Commits](aquasecurity/trivy-action@0.28.0...0.29.0)

---
updated-dependencies:
- dependency-name: aquasecurity/trivy-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit 270b09e)

* Bump github.com/onsi/ginkgo/v2 from 2.20.2 to 2.22.0 (#2335)

Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.20.2 to 2.22.0.
- [Release notes](https://github.com/onsi/ginkgo/releases)
- [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md)
- [Commits](onsi/ginkgo@v2.20.2...v2.22.0)

---
updated-dependencies:
- dependency-name: github.com/onsi/ginkgo/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit 40423d5)

* The webhook-key-name command-line param isn't taking effect (#2344)

Signed-off-by: C. H. Afzal <c-h-afzal@outlook.com>
(cherry picked from commit a261523)

* Robustness to driver pod taking time to create (#2315)

* Retry after driver pod now found if recent submission

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Add a test

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Make grace period configurable

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Update test

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Add an extra test with the driver pod

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Separate context to create and delete the driver pod

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Tidy

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Autoformat

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Update error message

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Add helm paramater

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Update internal/controller/sparkapplication/controller.go

Co-authored-by: Yi Chen <github@chenyicn.net>
Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

* Newlines between helm tests

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

---------

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>
Co-authored-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit d815e78)

* Use NSS_WRAPPER_PASSWD instead of /etc/passwd as in spark-operator image entrypoint.sh (#2312)

Signed-off-by: Aakcht <aakcht@gmail.com>
(cherry picked from commit 5dd91c4)

* Move sparkctl to cmd directory (#2347)

* Move spark-operator

Signed-off-by: Yi Chen <github@chenyicn.net>

* Move sparkctl to cmd directory

Signed-off-by: Yi Chen <github@chenyicn.net>

* Remove unnecessary app package/directory

Signed-off-by: Yi Chen <github@chenyicn.net>

---------

Signed-off-by: Yi Chen <github@chenyicn.net>
(cherry picked from commit 2375a30)

* Spark Operator Official Release v2.1.0

Signed-off-by: Yi Chen <github@chenyicn.net>

---------

Signed-off-by: Yi Chen <github@chenyicn.net>
Co-authored-by: Aran Shavit <Aranshavit@gmail.com>
Co-authored-by: Nicholas Gretzon <50811947+npgretz@users.noreply.github.com>
Co-authored-by: Cian (Keen) Gallagher <cg1830@gmail.com>
Co-authored-by: Jacob Salway <jacob.salway@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: C. H. Afzal <41106172+c-h-afzal@users.noreply.github.com>
Co-authored-by: Thomas Newton <thomas.w.newton@gmail.com>
Co-authored-by: Aakcht <aakcht@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChenYi015 ChenYi015 ChenYi015 approved these changes

@ImpSy ImpSy Awaiting requested review from ImpSy

@jacobsalway jacobsalway Awaiting requested review from jacobsalway

Assignees

@ChenYi015 ChenYi015

Labels

approved lgtm size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Cian911 @ChenYi015