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 bug in processing jobs on platforms without Docker #1834

Merged
merged 4 commits into from
Jun 6, 2023

Conversation

psa
Copy link
Contributor

@psa psa commented Jun 1, 2023

The container.GetHostInfo() struct is returned empty on hosts without docker and this results in passing zero parallelism to the parallel executor.

Use Golang's inbuilt runtime.NumCPU() instead. This should be safe on Docker too.

I've also added a fair bit of debug logging to assist with debugging the struct passed to the runner. This is leftover from my original line of inquiry, but it seemed like useful code so I kept it in the change.

@psa psa requested a review from a team as a code owner June 1, 2023 21:55
@psa psa changed the title Numcpu fix Fix bug in processing jobs on platforms without Docker Jun 1, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 1, 2023

@psa this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Jun 1, 2023
Paul Armstrong added 3 commits June 3, 2023 11:01
Log the full contents of the job protobuf to make debugging jobs easier
The caller may mis-calculate the number of CPUs as zero, in which case
ensure that at least one thread is spawned.
For hosts without docker, GetHostInfo() returns a blank struct which
has zero CPUs and causes downstream trouble.
Copy link
Member

@wolfogre wolfogre left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #1834 (2b6ac7b) into master (4989f44) will increase coverage by 1.39%.
The diff coverage is 65.24%.

@@            Coverage Diff             @@
##           master    #1834      +/-   ##
==========================================
+ Coverage   61.22%   62.62%   +1.39%     
==========================================
  Files          46       51       +5     
  Lines        7141     8187    +1046     
==========================================
+ Hits         4372     5127     +755     
- Misses       2462     2673     +211     
- Partials      307      387      +80     
Impacted Files Coverage Δ
pkg/common/outbound_ip.go 0.00% <0.00%> (ø)
pkg/container/docker_cli.go 82.23% <ø> (ø)
pkg/container/docker_logger.go 52.08% <ø> (ø)
pkg/container/docker_run.go 13.58% <0.00%> (-0.01%) ⬇️
pkg/container/docker_volume.go 0.00% <ø> (ø)
pkg/container/file_collector.go 37.30% <0.00%> (ø)
pkg/container/host_environment.go 0.00% <0.00%> (ø)
...ontainer/linux_container_environment_extensions.go 23.07% <0.00%> (-1.25%) ⬇️
pkg/exprparser/functions.go 66.32% <0.00%> (-1.04%) ⬇️
pkg/model/workflow.go 42.33% <8.88%> (+0.21%) ⬆️
... and 26 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mergify mergify bot removed the needs-work Extra attention is needed label Jun 5, 2023
@mergify mergify bot merged commit 3ac2b72 into nektos:master Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants