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

Speed up builds by caching /tmp/wheelhouse directory, speed up EL 8 build step by not running slow Requires / Provides Python generation step #697

Merged
merged 22 commits into from
Mar 31, 2021

Conversation

Kami
Copy link
Member

@Kami Kami commented Mar 28, 2021

This pull request is an attempt to speed up builds by caching /tmp/wheelhouse directory.

This way we can avoid re-building wheels for all the dependencies for each run which is quite slow.

TODO

  • Not necessarily as part of this PR, but figure out why RHEL 8 build is so much slower than other distro builds (almost x2).

@Kami
Copy link
Member Author

Kami commented Mar 28, 2021

@armab I didn't dig into TODO yet since I'm trying to get caching to work, but do you have any ideas why RHEL 8 build is so much slower compared to other distros?

EDIT: Actual build step is around 3-4 slower on CentOS 8 which is somewhat unacceptable. Improving that would help a lot.

@Kami
Copy link
Member Author

Kami commented Mar 28, 2021

Also, one of the "bottlenecks" is re-building wheels for all the git repo dependencies on each run (orquesta, ldap backend, rbac, etc.).

We should also figure out a way to cache those since there is no need to re-build it if the actual repo / git sha doesn't changes.

@arm4b
Copy link
Member

arm4b commented Mar 28, 2021

Yeah, EL8 takes the longest time to build since the beginning when it was added, we didn't figure out why.

@Kami
Copy link
Member Author

Kami commented Mar 28, 2021

I added some additional timing instrumentation and it seems one part of rpmbuild step just takes a very long time (~6 minutes).

Screenshot from 2021-03-28 23-12-17

Need to figure out to see if we can bump rpmbuild verbosity since it may offer more insight.

@Kami
Copy link
Member Author

Kami commented Mar 28, 2021

Here is the verbose debug output if someone wants to dig in - https://circleci.com/api/v1.1/project/github/StackStorm/st2-packages/3815/output/110/3?file=true&allocation-id=6060f421bf121a40f4490233-3-build%2F203B6FE6.

I quickly looked at it and it appears the issue is related to pythondistdeps.py being called a tons (probably for every Python dep) and it's slow.

EDIT: So it looks like this step find and generates "Requires" section for all the Python dependencies which I don't think is what we need. If you look at CentOS 8 debug output, that step doesn't run it all.

So if we can some how disable that Python deps requires generation for CentOS 8 as well, we should vastly speed up the build.

@Kami
Copy link
Member Author

Kami commented Mar 28, 2021

From slack (https://stackstorm-community.slack.com/archives/CSBELJ78A/p1616971134338600).

EL7 package:

rpm -q -i --provides ~/Downloads/st2-3.5dev-28.x86_64*1*.rpm
Name        : st2
Version     : 3.5dev
Release     : 28
Architecture: x86_64
Install Date: (not installed)
Group       : System/Management
Size        : 141683575
License     : ASL 2.0
Signature   : (none)
Source RPM  : st2-3.5dev-28.src.rpm
Build Date  : ned 28 mar 2021 23:30:57
Build Host  : f7917c6af893
URL         : https://github.com/StackStorm/st2
Summary     : StackStorm all components bundle
Description :
  Package is full standalone stackstorm installation including
  all components
config(st2) = 3.5dev-28
libffi-806b1a9d.so.6.0.4()(64bit)
st2 = 3.5dev-28
st2(x86-64) = 3.5dev-28

EL8 package:

kami ~/w/stackstorm/st2-packages (git:cache_tmp_wheelhouse)$ rpm -q -i --provides ~/Downloads/st2-3.5dev-28.x86_64.rpm
Name        : st2
Version     : 3.5dev
Release     : 28
Architecture: x86_64
Install Date: (not installed)
Group       : System/Management
Size        : 141991176
License     : ASL 2.0
Signature   : (none)
Source RPM  : st2-3.5dev-28.src.rpm
Build Date  : ned 28 mar 2021 23:37:42
Build Host  : ccdd67b8b527
URL         : https://github.com/StackStorm/st2
Summary     : StackStorm all components bundle
Description :
  Package is full standalone stackstorm installation including
  all components
config(st2) = 3.5dev-28
libffi-806b1a9d.so.6.0.4()(64bit)
python3.6dist(argcomplete) = 1.12.2
python3.6dist(click) = 7.1.2
python3.6dist(decorator) = 4.4.2
python3.6dist(filelock) = 3.0.12
python3.6dist(msgpack) = 1.0.2
python3.6dist(orjson) = 3.5.1
python3.6dist(pika) = 1.2.0
python3.6dist(prettytable) = 2.1.0
python3.6dist(prompt-toolkit) = 1.0.15
python3.6dist(python-json-logger) = 2.0.1
python3.6dist(python-statsd) = 2.1.0
python3.6dist(retrying) = 1.3.3
python3.6dist(sseclient-py) = 1.7
python3.6dist(webob) = 1.8.5
python3.6dist(wrapt) = 1.12.1
python3dist(argcomplete) = 1.12.2
python3dist(click) = 7.1.2
python3dist(decorator) = 4.4.2
python3dist(filelock) = 3.0.12
python3dist(msgpack) = 1.0.2
python3dist(orjson) = 3.5.1
python3dist(pika) = 1.2.0
python3dist(prettytable) = 2.1.0
python3dist(prompt-toolkit) = 1.0.15
python3dist(python-json-logger) = 2.0.1
python3dist(python-statsd) = 2.1.0
python3dist(retrying) = 1.3.3
python3dist(sseclient-py) = 1.7
python3dist(webob) = 1.8.5
python3dist(wrapt) = 1.12.1
st2 = 3.5dev-28
st2(x86-64) = 3.5dev-28

In short, I don't think we need all that "requires" Python cruft for EL 8 packages since we bundle virtualenv and that requires section looks wrong anyway (it's missing a bunch of dependencies).

@Kami
Copy link
Member Author

Kami commented Mar 28, 2021

Alright, looks like a victory - from ~10-13 minutes for EL8 build step down to ~4 minutes. The whole build should now take around 10 minutes with a primed cache vs ~17-18 minutes before.

It appears my workaround / fix worked. The "meat" of it is here https://github.com/StackStorm/st2-packages/pull/697/files#diff-f0d938587e6d1cdabf2257a39f1070bac432a2b3242ee1c95cdd84454549a53cR28. Basically, I re-defined those macros since we don't want to run those steps.

The build is much faster now and more in line with EL7 where all that requires and provides cruft which we don't depend on is not there since we are not a traditional RPM for a Python library and we bundle virtual environment with all the deps.


Technically that Requires and Provides section was also incorrect / wrong because our package doesn't provide those libraries since they are only used internally in a package specific virtual environment.

Somewhat surprised it didn't cause issues yet - I guess that's probably because most people install it on a dedicated server / VM where they don't install a bunch of other RPM packages which could uncover this issue.

In short, if you installed old EL 8 RPM on a server, then installed RPM for some Python library which depends on another Python library which st2 package claims it Provides, it would cause havoc / not work / break things.

@Kami Kami changed the title [WIP] Speed up builds by caching /tmp/wheelhouse directory Speed up builds by caching /tmp/wheelhouse directory, speed up EL 8 build step by not running slow Requires / Provides Python generation step Mar 28, 2021
difference as well (unlikely since those macros don't appear to run
there by default).
@Kami
Copy link
Member Author

Kami commented Mar 29, 2021

Do we still need to build cryptography from scratch in EL build?

I removed that change and it appears to be working fine (it uses correct pre-built wheel).

I assume it may have something to do with old version of pip in the past and perhaps that version not supporting newer pre-built wheel formats used by cryptography or similar.

EDIT: @punkrokk It appears you originally added that change (38fe69f) so perhaps you remember the context / reasoning behind it.

# ls -la ~/st2-packages/wheelhouse/
# du -hs ~/st2-packages/wheelhouse/
- save_cache:
name: Store Wheelhouse Cache
Copy link
Member

Choose a reason for hiding this comment

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

👍

docker cp st2-packages-vol:/root/build/. ~/st2-packages/build/${DISTRO}
# # TODO: It works! (~0.5-1min speed-up) Enable CircleCI2.0 cache for pip and wheelhouse later
Copy link
Member

Choose a reason for hiding this comment

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

Good to find this TODO comment. This takes us back in time, 3 years ago :)

packages/st2/rpm/st2.spec Show resolved Hide resolved
packages/st2/rpm/st2.spec Show resolved Hide resolved
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

I'm totally fine with the Provides changes.

Looks like Requires change removes ~2mins of the build overhead for EL8.
I agree that's a reasonable price to pay for the controversial to me %undefine __pythondist_requires and %undefine __python_requires addition.

Thanks for the deep research and significant optimization here 👍

@Kami
Copy link
Member Author

Kami commented Mar 31, 2021

@armab Thanks for the review.

@Kami Kami merged commit dc3985f into master Mar 31, 2021
@Kami Kami deleted the cache_tmp_wheelhouse branch March 31, 2021 19:26
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.

2 participants