-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Enable the override of MemoryLimit through webhook #2478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: danielrs <[email protected]>
Signed-off-by: danielrs <[email protected]>
Signed-off-by: danielrs <[email protected]>
Signed-off-by: danielrs <[email protected]>
Signed-off-by: danielrs <[email protected]>
Hi @ChenYi015 , I am tagging you here as you were the reviewer of #2383 and I am addressing your comments in here. Hope you like it! |
@danielrsfreitas Thanks for your contribution, I will review it later today. |
Signed-off-by: danielrs <[email protected]>
/lgtm |
Hi @ChenYi015 , thanks for approving the code. I see it is not ready for merge yet. Is there anything left on my side? What would be the next steps? |
Let us wait for approve from another maintainer and we will release this feature in the next minor version (i.e. |
/assign @vara-bonthu @jacobsalway @ImpSy |
Any updates? Was anybody else able to check this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for your contribution @danielrsfreitas !
@danielrsfreitas just saw this, thanks for taking the effort and make this ready for merge, much appreciated 🙏 |
/approve |
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Did anyone test this? These new parameters are not working when I am using a kubectl manifest of such on v2.2.0: Error from server (BadRequest): error when creating "xxxxx.yaml": SparkApplication in version "v1beta2" cannot be handled as a SparkApplication: strict decoding error: unknown field "spec.executor.memoryLimit" apiVersion: sparkoperator.k8s.io/v1beta2 |
Purpose of this PR
Proposed changes:
Add the ability to setup
memoryLimits
definition through webhook.The changes were originally part of the development of #2383 and #2389.
Change Category
Rationale
This changes addresses the community most reacted issue #1489.
A few users have created their own custom version of either SparkOperator or Kubernetes Admission Controller to enable memoryLimits with a different value than memory requested.
It was first raised here #1990. The pull request 1990 contained two features: memoryLimits and queue boost. Since the second part was covered in another PR, the 1990 was closed and the memoryLimits was not merged in.
Checklist
Additional Notes