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 configurable MAX_FORKS for jobs #5604

Merged
merged 1 commit into from
Jan 15, 2020

Conversation

jakemcdermott
Copy link
Contributor

@jakemcdermott jakemcdermott commented Jan 7, 2020

Add configurable MAX_FORKS for jobs

SUMMARY

Add a configurable limit for job forks
see #5142

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • API
  • UI
ADDITIONAL INFORMATION

settings
Screenshot from 2020-01-06 18-42-58

save
max_forks_01

launch
Screenshot from 2020-01-15 13-15-26

@jakemcdermott jakemcdermott changed the title Add a configurable limit for job forks Add configurable MAX_FORKS for jobs Jan 7, 2020
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Contributor

@ryanpetrello ryanpetrello left a comment

Choose a reason for hiding this comment

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

Should we add something that enforces this after-the-fact (maybe in the tasks.py code that looks up the actual forks value)? i.e.,

  • User makes a JT w/ 5000 forks
  • Admin notices and sets the max to 20
  • User runs the JT

In this scenario, I think the job should still probably run, but maybe we enforce 20, and print a loud warning in the logs.

@jakemcdermott
Copy link
Contributor Author

In this scenario, I think the job should still probably run, but maybe we enforce 20, and print a loud warning in the logs.

Sounds good to me. We can log a warning instead of completely preventing the launch.

By enforce, do you mean just log the warning or log the warning and stop the job before the playbook gets executed?

@ryanpetrello
Copy link
Contributor

ryanpetrello commented Jan 8, 2020

I'm of the opinion that we shouldn't stop the job from running - just cap the forks with min(job.forks, max_allowed_forks) and print a warning in the logs.

args.append('--forks=%d' % job.forks)
if job.forks:
if settings.MAX_FORKS > 0 and job.forks > settings.MAX_FORKS:
logger.warning(f'Maximum number of forks ({settings.MAX_FORKS}) exceeded.')
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@ryanpetrello ryanpetrello left a comment

Choose a reason for hiding this comment

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

Looks good to me - thanks!

args.append('--forks=%d' % job.forks)
if job.forks:
if settings.MAX_FORKS > 0 and job.forks > settings.MAX_FORKS:
logger.warning(f'Maximum number of forks ({settings.MAX_FORKS}) exceeded.')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot from 2020-01-15 13-15-26

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member

made one small test comment

otherwise no objections from me

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 3040a25 into ansible:devel Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants