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

Prevent slowdown in yum due to high file descriptor count #1960

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

vepadulano
Copy link
Contributor

Checklist

  • Added a news entry

I am one of the maintainers of the ROOT project conda-forge feedstock at https://github.com/conda-forge/root-feedstock . We make use of the yum_requirements.txt feature which triggers a yum install step in our build with the necessary package. I always noticed that this takes a huge amount of time due to this bug and I always manually set the number of file descriptors when developing locally. I thought it would be a good idea to upstream this to conda-smithy. Let me know if it makes sense and if I'm missing anything relevant in the changes. Thank you so much for the great project!

@vepadulano vepadulano requested a review from a team as a code owner June 24, 2024 07:46
Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Can we restrict this change to only when yum requirements are actually used? IDK what side effects it might have on builds otherwise and so that seems safer. Maybe I am worrying too much though.

Set the maximum number of file descriptors to 1024 before going through the `yum
install` step of the build, only in case `yum_requirements.txt` is present in
the recipe. This is done to mitigate a bug with old versions of rpm such as the
one shipped with the Centos7 container. See https://bugzilla.redhat.com/show_bug.cgi?id=1537564
@vepadulano
Copy link
Contributor Author

Can we restrict this change to only when yum requirements are actually used?

Seems like a reasonable request! I modified the commit, let me know if it looks good 👍

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

We should ensure not to cause new lines.

conda_smithy/templates/build_steps.sh.tmpl Outdated Show resolved Hide resolved
Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Thank you!

@vepadulano
Copy link
Contributor Author

Thank you so much @beckermr for your prompt reply and help!

Comment on lines 65 to 69
# Due to https://bugzilla.redhat.com/show_bug.cgi?id=1537564 old versions of rpm
# are drastically slowed down when the number of file descriptors is very high.
# This can be visible during a `yum install` step of a feedstock build.
ulimit -n 1024
{{ yum_build_setup }}{% endif -%}
Copy link
Member

Choose a reason for hiding this comment

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

Let's put this in a subshell to make this as locally as possible.

Suggested change
# Due to https://bugzilla.redhat.com/show_bug.cgi?id=1537564 old versions of rpm
# are drastically slowed down when the number of file descriptors is very high.
# This can be visible during a `yum install` step of a feedstock build.
ulimit -n 1024
{{ yum_build_setup }}{% endif -%}
(
# Due to https://bugzilla.redhat.com/show_bug.cgi?id=1537564 old versions of rpm
# are drastically slowed down when the number of file descriptors is very high.
# This can be visible during a `yum install` step of a feedstock build.
# => Set a lower limit in a subshell for the `yum install`s only.
ulimit -n 1024
{{ yum_build_setup }}
){% endif -%}

(Alternatively, we could also put this is in yum_build_setup directly; IDK what the preference would be.)

Copy link
Contributor

Choose a reason for hiding this comment

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

(unresolved for visibility)

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could also put this is in yum_build_setup directly

Handling in PR: #1961

This also fixed a formatting issue Jinja blank space cleanup otherwise introduces

Co-authored-by: Marcel Bargull <[email protected]>
Copy link
Member

@mbargull mbargull left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for digging deep into this stuff!

@beckermr beckermr merged commit 9e4720f into conda-forge:main Jun 24, 2024
2 checks passed
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.

5 participants