trafficcontrol icon indicating copy to clipboard operation
trafficcontrol copied to clipboard

CiaB: Do not rebuild Traffic Server when the ATS RPM spec changes

Open zrhoffman opened this issue 3 years ago • 10 comments

This PR makes the ATS RPM no longer rebuild when Make detects that the files in $(TC_DIR)/cache-config/testing/docker/trafficserver/ have changed, which saves 6-8 minutes of local build time for CDN in a Box. The ATS RPM will still rebuild whenever a new commit is pushed to apache/trafficserver:8.1.x


Which Traffic Control components are affected by this PR?

  • CDN in a Box

What is the best way to verify this PR?

  1. In the CDN in a Box directory, build the targets using make
    make -j$(nproc)
    
  2. Update a file in the cd cache-config/testing/docker/trafficserver directory
    echo echo An update >> ../../cache-config/testing/docker/trafficserver/run.sh
    
  3. Run make again, verify that no targets are rebuilt
    make
    
    Expected output:
    make: Nothing to be done for 'all'.
    

PR submission checklist

zrhoffman avatar Jan 27 '22 14:01 zrhoffman

But... doesn't this mean that if the process for generating an ATS RPM for CiaB changes, it won't correctly detect that it needs to rebuild the RPM and those changes won't be propagated to CiaB?

ocket8888 avatar Apr 20 '22 20:04 ocket8888

But... doesn't this mean that if the process for generating an ATS RPM for CiaB changes, it won't correctly detect that it needs to rebuild the RPM and those changes won't be propagated to CiaB?

That is an unlikely edge case that can be fixed if the user runs make very-clean, which should be early on in their process for troubleshooting ATS not working.

90% of CDN in a Box (including t3c) works without ATS functioning properly, but if it does ever break (e.g., if we ever change to a /-prefixed RPM instead of /opt/trafficserver/-prefixed), the ATS RPM gets rebuilt whenever apache/[email protected] is updated (about once a month), so that issue would go away with time anyway.

IMO not having to rebuild the ATS RPM when switching Git branches is well worth having this edge case. But if you have another idea to avoid unnecessarily rebuilding the ATS RPM, I'm all ears

zrhoffman avatar Apr 20 '22 21:04 zrhoffman

not having to rebuild the ATS RPM when switching Git branches is well worth having this edge case.

Why do you have to rebuild the ATS RPM when you switch git branches? You should only have to rebuild if the sources in that directory change. The specfile you reference in the title has only changed seven times ~this year~ in the past year.

I just verified that switching git branches doesn't make you rebuild that:

$ time make cache/trafficserver.rpm
make: 'cache/trafficserver.rpm' is up to date.
make cache/trafficserver.rpm  0.02s user 0.04s system 141% cpu 0.046 total
$ git checkout 1f264bf1624246616a62ffe93c6ddb7d7a095ff0
Note: switching to '1f264bf1624246616a62ffe93c6ddb7d7a095ff0'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 1f264bf16 Fixes the cache-config integration tests. (#6755)
$ time make cache/trafficserver.rpm
make: 'cache/trafficserver.rpm' is up to date.
make cache/trafficserver.rpm  0.02s user 0.04s system 137% cpu 0.044 total

ocket8888 avatar Apr 20 '22 22:04 ocket8888

Why do you have to rebuild the ATS RPM when you switch git branches? You should only have to rebuild if the sources in that directory change.

It's not only about the sources, it's also about the age of the Traffic Server RPM in the dist/ directory. If make detects that the sources are newer than the already-build Traffic Server RPM, it will try to build the RPM again. For example, if dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm already exists, but you touch the sources to make them newer:

[zrhoffman@computer cdn-in-a-box]$ touch ../../cache-config/testing/docker/trafficserver/*
[zrhoffman@computer cdn-in-a-box]$ make --debug cache/trafficserver.rpm
GNU Make 4.3
Built for x86_64-pc-linux-gnu
Copyright (C) 1988-2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Reading makefiles...
Updating makefiles....
Updating goal targets....
   Prerequisite '../../cache-config/testing/docker/trafficserver/cjose.pic.patch' is newer than target '../../dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm'.
   Prerequisite '../../cache-config/testing/docker/trafficserver/Dockerfile' is newer than target '../../dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm'.
   Prerequisite '../../cache-config/testing/docker/trafficserver/jansson.pic.patch' is newer than target '../../dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm'.
   Prerequisite '../../cache-config/testing/docker/trafficserver/run.sh' is newer than target '../../dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm'.
   Prerequisite '../../cache-config/testing/docker/trafficserver/traffic_server_jemalloc' is newer than target '../../dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm'.
   Prerequisite '../../cache-config/testing/docker/trafficserver/trafficserver.spec' is newer than target '../../dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm'.
  Must remake target '../../dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm'.
docker-compose -f ./../../cache-config/testing/docker/docker-compose-ats-build.yml build --parallel trafficserver_build && docker-compose -f ./../../cache-config/testing/docker/docker-compose-ats-build.yml run --rm trafficserver_build
[+] Building 0.9s (5/10)
 => [internal] load build definition from Dockerfile                                                              0.4s
[and so on]

but if you cancel that, then touch the RPM to make it newer:

[zrhoffman@computer cdn-in-a-box]$ touch ../../dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm
[zrhoffman@computer cdn-in-a-box]$ make --debug cache/trafficserver.rpm
GNU Make 4.3
Built for x86_64-pc-linux-gnu
Copyright (C) 1988-2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Reading makefiles...
Updating makefiles....
Updating goal targets....
 Prerequisite '../../dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm' is newer than target 'cache/trafficserver.rpm'.
Must remake target 'cache/trafficserver.rpm'.
cp -f "../../dist/trafficserver-8.1.5-1.c16631ab0.el8.x86_64.rpm" "cache/trafficserver.rpm" || (rm ".//cache/ATS_VERSION"; false)
Successfully remade target file 'cache/trafficserver.rpm'.

#6525 fixes this problem

zrhoffman avatar Sep 21 '22 18:09 zrhoffman

Oh, so you mean like you switch to a branch where the source is changed and then switch back it still thinks the source is newer?

That's true, but it seems extremely rare since that hardly ever changes.

ocket8888 avatar Sep 21 '22 21:09 ocket8888

It happens frequently to me. Just running this triggers an ATS RPM rebuild the next time I run make cache/trafficserver.rpm.

git checkout 1f264bf162~ && git checkout apache/master

zrhoffman avatar Sep 21 '22 21:09 zrhoffman

I just verified that switching git branches doesn't make you rebuild that:

That is a different result than I got. You used 1f264bf162, which is most recent time the ATS RPM directory was changed. How about 1f264bf162~?

zrhoffman avatar Sep 21 '22 21:09 zrhoffman

I didn't check for a commit where any specific file was modified I just did ~HEAD, because I thought you meant it would happen any time you switch branches.

ocket8888 avatar Sep 21 '22 22:09 ocket8888

This still feels like a step backwards, though. Make is doing its level-best to tell if something needs to be rebuilt, but it's just not quite smart enough to do it - so instead we just give up any kind of change detection? I, for one, would absolutely checkout the master branch and/or rebase my PR branches without reading through each new commit's diff to see if it changed this file, so for people like me this means our RPMs just stay out-of-date forever, as an alternative to rebuilding sometimes when we don't really need to. I feel like a better solution would be to simply make the ATS build system smarter so that rebuilding when there are no real changes isn't a total repeat of work that's already been done, perhaps by putting more of the process in the Docker caching layers rather than on every run.

ocket8888 avatar Sep 22 '22 15:09 ocket8888