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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

zrhoffman
Copy link
Member

@zrhoffman zrhoffman commented Jan 27, 2022 •
edited
Loading

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 zrhoffman added low impact affects only a small portion of a CDN, and cannot itself break one Traffic Server related to Apache Traffic Server cdn-in-a-box related to the Docker-based CDN-in-a-Box system build related to the build process improvement The functionality exists but it could be improved in some way. labels Jan 27, 2022
ericholguin
ericholguin approved these changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@ericholguin ericholguin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

  • Ran make
  • Made changes to cache-config/testing/docker/trafficserver/run.sh
  • Ran make again
  • Output: make: Nothing to be done for 'all'.

@ocket8888
Copy link
Contributor

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?

@zrhoffman
Copy link
Member Author

zrhoffman commented Apr 20, 2022 •
edited
Loading

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/trafficserver@8.1.x 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

@ocket8888
Copy link
Contributor

ocket8888 commented Apr 20, 2022 •
edited
Loading

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

@zrhoffman
Copy link
Member Author

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

@ocket8888
Copy link
Contributor

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.

@zrhoffman
Copy link
Member Author

zrhoffman commented Sep 21, 2022 •
edited
Loading

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
Copy link
Member Author

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~?

@ocket8888
Copy link
Contributor

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
Copy link
Contributor

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.

@zrhoffman
Copy link
Member Author

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?

@ocket8888 Yes, because that RPM is not being used for ATS development, it uses ATS release branches. ATS_SOURCES does not refer to the source code of ATS, just the sources for this particular build environment, which is pretty mundane.

If a developer wanted to test their own ATS dev work in CDN in a Box, they would supply their own RPM, in which case they wouldn't want to use the makefile for the ATS RPM anyway.

That's an edge case, though. Most people using CDN in a Box just wand an ATS RPM, any ATS RPM. Recall that, until #5920, we used an ancient ATS RPM from ATS's CI server, it wasn't a make goal at all.

trafficcontrol/infrastructure/cdn-in-a-box/cache/Dockerfile

Lines 76 to 77 in eff8f8f

ADD https://ci.trafficserver.apache.org/RPMS/CentOS7/trafficserver-7.1.4-2.el7.x86_64.rpm /trafficserver.rpm
ADD https://ci.trafficserver.apache.org/RPMS/CentOS7/trafficserver-devel-7.1.4-2.el7.x86_64.rpm /trafficserver-devel.rpm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericholguin ericholguin ericholguin approved these changes

Assignees
No one assigned
Labels
build related to the build process cdn-in-a-box related to the Docker-based CDN-in-a-Box system improvement The functionality exists but it could be improved in some way. low impact affects only a small portion of a CDN, and cannot itself break one Traffic Server related to Apache Traffic Server
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zrhoffman @ocket8888 @ericholguin