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

fix: Resource requests on init/wait containers. Fixes #6809 #6879

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

zorulo
Copy link
Contributor

@zorulo zorulo commented Oct 6, 2021

Signed-off-by: zorulo

Fixes #6809. I understand we probably want tests on this (?) but I'm unfamiliar with how to do so (sorry for newbieness).

I wasn't quite clear on how to run this locally; followed the running locally guide but ran into the issue mentioned here, amongst others, also trying to use minikube, v1.22.2 of both minikube/kubectl.

As such, submitting the test workflow as noted in #6809 did not run because of the issue, but I was able to look at the pod and check that the requests were being applied without needing to specify limits.

Signed-off-by: zorulo <artist.swimmer.cheng@gmail.com>
@zorulo zorulo force-pushed the bugfix/init-wait-resource-requests branch from 01ccefc to ca955e5 Compare October 6, 2021 21:16
@zorulo zorulo changed the title Fixes #6809 Resource requests on init/wait containers fix: Resource requests on init/wait containers. Fixes #6809 Oct 6, 2021
@tczhao
Copy link
Member

tczhao commented Oct 7, 2021

Hi @zorulo
You can add test to TestIsResourcesSpecified at workflow/controller/workflowpod_test.go
then make test to validate

@sarabala1979 sarabala1979 self-assigned this Oct 7, 2021
Signed-off-by: zorulo <artist.swimmer.cheng@gmail.com>
@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #6879 (718ac7e) into master (efa11e0) will decrease coverage by 0.13%.
The diff coverage is 22.22%.

❗ Current head 718ac7e differs from pull request most recent head 59c2703. Consider uploading reports for the commit 59c2703 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6879      +/-   ##
==========================================
- Coverage   48.65%   48.51%   -0.14%     
==========================================
  Files         265      265              
  Lines       19161    19258      +97     
==========================================
+ Hits         9322     9343      +21     
- Misses       8790     8863      +73     
- Partials     1049     1052       +3     
Impacted Files Coverage Δ
persist/sqldb/archived_workflow_labels.go 44.82% <0.00%> (-33.97%) ⬇️
persist/sqldb/migrate.go 0.00% <0.00%> (ø)
persist/sqldb/null_workflow_archive.go 0.00% <0.00%> (ø)
persist/sqldb/workflow_archive.go 0.00% <0.00%> (ø)
...iclient/http1/archived-workflows-service-client.go 0.00% <0.00%> (ø)
server/workflowarchive/archived_workflow_server.go 59.43% <53.84%> (-2.30%) ⬇️
...orkflow/controller/estimation/estimator_factory.go 70.00% <100.00%> (ø)
workflow/controller/workflowpod.go 74.41% <100.00%> (ø)
workflow/metrics/server.go 15.78% <0.00%> (-3.51%) ⬇️
pkg/apiclient/http1-client.go 0.00% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efa11e0...59c2703. Read the comment docs.

@alexec alexec enabled auto-merge (squash) October 7, 2021 20:06
@alexec alexec merged commit 4d38404 into argoproj:master Oct 7, 2021
This was referenced Oct 15, 2021
sarabala1979 pushed a commit that referenced this pull request Oct 19, 2021
Signed-off-by: zorulo <artist.swimmer.cheng@gmail.com>
kriti-sc pushed a commit to kriti-sc/argo-workflows that referenced this pull request Oct 24, 2021
…rgoproj#6879)

Signed-off-by: zorulo <artist.swimmer.cheng@gmail.com>
Signed-off-by: kriti-sc <kathuriakriti1@gmail.com>
@alexec alexec mentioned this pull request Nov 5, 2021
25 tasks
@sarabala1979 sarabala1979 mentioned this pull request Dec 15, 2021
73 tasks
@sarabala1979 sarabala1979 mentioned this pull request Mar 1, 2022
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.

Executor resource requests not applied to init/wait containers
4 participants