Skip to content
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

Add resource limits for spark dependencies cronjob #620

Merged
merged 1 commit into from
Aug 30, 2019

Conversation

rubenvp8510
Copy link
Collaborator

Fixes #598

@rubenvp8510 rubenvp8510 force-pushed the Issue-598 branch 2 times, most recently from 9397b27 to 76143e9 Compare August 27, 2019 01:01
@jpkrohling
Copy link
Contributor

One of the e2e tests for Kubernetes failed, so I just restarted it. Almost immediately, I saw this in the logs:

git checkout --progress --force 76143e9bd1c40fc0b71fd45a987c80b9db0a9096
##[error]fatal: reference is not a tree: 76143e9bd1c40fc0b71fd45a987c80b9db0a9096
Removed matchers: 'checkout-git'
##[remove-matcher owner=checkout-git]
##[error]Git checkout failed with exit code: 128
##[error]Exit code 1 returned from process: file name '/home/runner/runners/2.157.0/bin/Runner.PluginHost', arguments 'action "GitHub.Runner.Plugins.Repository.CheckoutTask, Runner.Plugins"'.

Not sure what's going on, but could you try to rebase and update this PR?

@jpkrohling
Copy link
Contributor

Opened actions/checkout#23 to track this failure.

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a test?

@@ -68,7 +68,8 @@ func CreateSparkDependencies(jaeger *v1.Jaeger) *batchv1beta1.CronJob {
Image: jaeger.Spec.Storage.Dependencies.Image,
Name: name,
// let spark job use its default values
Env: removeEmptyVars(envVars),
Env: removeEmptyVars(envVars),
Resources: jaeger.Spec.Storage.Dependencies.Resources,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think other components by default use the resources specified at the root level of the spec. We should consider doing the same here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@rubenvp8510 rubenvp8510 force-pushed the Issue-598 branch 2 times, most recently from 3d81896 to c41c003 Compare August 27, 2019 18:06
@rubenvp8510
Copy link
Collaborator Author

@pavolloffay Tests added.

@pavolloffay pavolloffay merged commit d2d327d into jaegertracing:master Aug 30, 2019
@palmerabollo
Copy link

palmerabollo commented Nov 1, 2019

@rubenvp8510 could you please share an example of how a Jaeger resource would look like (yaml)? I am not sure about how the golang ResourceRequirements maps to the yaml (I see the TestSparkDependenciesResources in the code but I can't see the mapping to yaml either).

I am particularly interested in increasing the request/limits of the jaeger-spark-dependencies pods. The official docs omit the info about it in the "Configuring the Custom Resource" section.

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource limits for spark dependencies cronjob
4 participants