Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Updates ATS builds for cache-config integration tests.#5938

Merged
zrhoffman merged 1 commit intoapache:masterfrom
jrushford:ats_openssl_fix
Jun 18, 2021
Merged

Updates ATS builds for cache-config integration tests.#5938
zrhoffman merged 1 commit intoapache:masterfrom
jrushford:ats_openssl_fix

Conversation

@jrushford
Copy link
Copy Markdown
Contributor

@jrushford jrushford commented Jun 15, 2021

Modifies building ATS for cache-config integration proper versions of openssl, cjose, and jansson are linked and
provided by the ATS rpm.

What does this PR (Pull Request) do?

  • Insures the ATS build used in integration tests is linked with openssl 1.1.1, close, and Jansson
  • Changes minimum Centos/Red Had version to 8
  • All Containers now run CentOS 8.

Which Traffic Control components are affected by this PR?

  • Traffic Ops ORT

What is the best way to verify this PR?

The GHA cache-config integration tests should pass. You may
run the tests manually also, see the cache-config/testing/README.md

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

@mitchell852 mitchell852 added cache-config Cache config generation tests related to tests and/or testing infrastructure labels Jun 15, 2021
Copy link
Copy Markdown
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

This is an oversight of #5920, not #5938, but changes to cache-config/testing/docker/trafficserver/** should trigger the CDN-in-a-Box CI GHA workflow. It is currently ignored, with the rest of cache-config/testing/**:

- 'cache-config/testing/**'

- 'cache-config/testing/**'

Would you please add exceptions for cache-config/testing/docker/trafficserver/** so that PRs like these trigger the CDN-in-a-Box CI workflow?

@jrushford jrushford force-pushed the ats_openssl_fix branch 2 times, most recently from b18be54 to 722f8cd Compare June 15, 2021 20:37
@jrushford jrushford force-pushed the ats_openssl_fix branch 4 times, most recently from 1e7ddb7 to 510911d Compare June 16, 2021 03:33
@ocket8888
Copy link
Copy Markdown
Contributor

I was about to restart the CiaB job, but it's a real failure:

Executing(%clean): /bin/sh -e /var/tmp/rpm-tmp.ThI6Uy
+ umask 022
+ cd /root/rpmbuild/BUILD
+ cd trafficserver-8.1.1
+ rm -rf /root/rpmbuild/BUILDROOT/trafficserver-8.1.1-45.55bbe4a64.el8.x86_64
+ exit 0
Build completed
trafficserver RPM has been copied
cp -f "../../dist/trafficserver-.el8.x86_64.rpm" "cache/trafficserver.rpm" || (rm ".//cache/ATS_VERSION"; false)
cp: cannot stat '../../dist/trafficserver-.el8.x86_64.rpm': No such file or directory
rm: cannot remove './/cache/ATS_VERSION': No such file or directory
make: *** [Makefile:147: cache/trafficserver.rpm] Error 1
Child process make exited with status code 2 !

Thought it was worth calling out explicitly, since my first instinct was to just restart it.

@jrushford
Copy link
Copy Markdown
Contributor Author

I was about to restart the CiaB job, but it's a real failure:

Executing(%clean): /bin/sh -e /var/tmp/rpm-tmp.ThI6Uy
+ umask 022
+ cd /root/rpmbuild/BUILD
+ cd trafficserver-8.1.1
+ rm -rf /root/rpmbuild/BUILDROOT/trafficserver-8.1.1-45.55bbe4a64.el8.x86_64
+ exit 0
Build completed
trafficserver RPM has been copied
cp -f "../../dist/trafficserver-.el8.x86_64.rpm" "cache/trafficserver.rpm" || (rm ".//cache/ATS_VERSION"; false)
cp: cannot stat '../../dist/trafficserver-.el8.x86_64.rpm': No such file or directory
rm: cannot remove './/cache/ATS_VERSION': No such file or directory
make: *** [Makefile:147: cache/trafficserver.rpm] Error 1
Child process make exited with status code 2 !

Thought it was worth calling out explicitly, since my first instinct was to just restart it.

I reverted the ciab.yml changes as they are not really part of this PR to begin with. I didn't look at the error in detail.

@jrushford jrushford force-pushed the ats_openssl_fix branch 2 times, most recently from 1e3116f to a3f3e20 Compare June 16, 2021 14:21
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why no longer define the ATS_VERSION environment variable in the compose file or an env file? I see that run.sh sets a default value for ATS_VERSION if ATS_VERSION is undefined, but if ATS_VERSION is meant to be passed to run.sh, a user should be able to see a sane default for ATS_VERSION without running run.sh, by running something like

docker-compose -f docker-compose-ats-build.yml run --rm trafficserver_build sh -c 'echo $ATS_VERSION'

Copy link
Copy Markdown
Contributor Author

@jrushford jrushford Jun 16, 2021

Choose a reason for hiding this comment

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

I moved it to the environment section of the GHA workflow/cache-config-tests.yml thinking that it was made available to all runners.

Copy link
Copy Markdown
Member

@zrhoffman zrhoffman Jun 16, 2021

Choose a reason for hiding this comment

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

I moved it to the environment section of the GHA workflow/cache-config-tests.yml thinking that it was made available to all runners.

Sure, it will be made available to all GitHub Actions runners, but the T3C integration tests should continue to be usable outside of GitHub Actions.

I see that there is a default set in run.sh itself:
https://github.com/apache/trafficcontrol/blob/a3f3e2045b86e734f52c043c59d9dfd73641b29e/cache-config/testing/docker/trafficserver/run.sh#L28
but now that means that users of the T3C integration tests would need to poke around in the scripts themselves to see what environment variables it uses. The compose file or an env file should exist to serve as a reference for users wanting to know what environment variables they can set, especially since the user can already learn about CJOSE_URL, CJOSE_TAG, JANSSON_URL, JANSSON_TAG, OPENSSL_URL, and OPENSSL_TAG from the compose file.

I do see the value in being able to set that environment variable from GitHub Actions. In order to do that:

  1. The hard-coded 8.1.x provided as the branch input for the fetch-github-branch-sha action should instead reference the ATS_VERSION environment variable:https://github.com/apache/trafficcontrol/blob/a3f3e2045b86e734f52c043c59d9dfd73641b29e/.github/workflows/cache-config-tests.yml#L103-L109
  2. The ATS_VERSION environment variable should be referenced used for the fetch-github-branch-sha action's branch input in the CDN-in-a-Box CI workflow, as well:
    https://github.com/apache/trafficcontrol/blob/a3f3e2045b86e734f52c043c59d9dfd73641b29e/.github/workflows/ciab.yaml#L224-L230
  3. The ATS_VERSION environment variable needs to be defined for the CDN in a Box CI workflow:
    https://github.com/apache/trafficcontrol/blob/a3f3e2045b86e734f52c043c59d9dfd73641b29e/.github/workflows/ciab.yaml#L20-L21
  4. (Already covered:) Removing a definition of ATS_VERSION from docker-compose-ats-build.yml breaks bin/ats-version.sh, used by the CDN in a Box ATS_DIST_RPM makefile target:
    https://github.com/apache/trafficcontrol/blob/a3f3e2045b86e734f52c043c59d9dfd73641b29e/infrastructure/cdn-in-a-box/bin/ats-version.sh#L30
    So that you don't need to touch the ats-version.sh, I have opened Search both trafficserver/run.sh or docker-compose-ats-build.yml the ATS release branch/ATS_VERSION #5945 so that CDN in a Box does not break when Updates ATS builds for cache-config integration tests. #5938 is merged.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So a lot of this is me trying and testing different things in the GHA workflow. So don't get too excited with me pushing up things to test in the workflow. I'll let you know when I'm done ok.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds good, I'll wait to hear from you.

@zrhoffman
Copy link
Copy Markdown
Member

The Traffic Control CI should not need to run ATS tests. I'm not opposed to keeping make check in the RPM SPEC file, but once it's moved to a separate %check section, the build-ats-test-rpm GitHub Action should have a way of having the rpmbuild command include --nocheck.

@jrushford
Copy link
Copy Markdown
Contributor Author

The Traffic Control CI should not need to run ATS tests. I'm not opposed to keeping make check in the RPM SPEC file, but once it's moved to a separate %check section, the build-ats-test-rpm GitHub Action should have a way of having the rpmbuild command include --nocheck.

@zrhoffman You're referring to the Internal Jenkins CI?

@zrhoffman
Copy link
Copy Markdown
Member

zrhoffman commented Jun 16, 2021

The Traffic Control CI should not need to run ATS tests. I'm not opposed to keeping make check in the RPM SPEC file, but once it's moved to a separate %check section, the build-ats-test-rpm GitHub Action should have a way of having the rpmbuild command include --nocheck.

@zrhoffman You're referring to the Internal Jenkins CI?

I am referring to .github/actions/build-ats-test-rpm.

Edit: By "The Traffic Control CI" I just mean the ATC GitHub Actions in general.

@jrushford jrushford force-pushed the ats_openssl_fix branch 5 times, most recently from 5902872 to c47fc96 Compare June 17, 2021 16:33

trafficserver_build:
environment:
- RHEL_VERSION=8
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Building the image using build arg RHEL_VERSION=7 and running using that image and environment variable RHEL_VERSION=8 (and vice versa) causes the ATS RPM to fail to build. So, either

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Made the changes to the ort_test/Dockerfile.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

RHEL_VERSION is still defined in the environment in the compose file, which overrides RHEL_VERSION as set by an ENV instruction in the Dockerfile, so it should still be removed here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

proper versions of openssl, cjose, and jansson  are linked and
provided by the ATS rpm.
@zrhoffman zrhoffman added the improvement The functionality exists but it could be improved in some way. label Jun 17, 2021
Copy link
Copy Markdown
Member

@zrhoffman zrhoffman 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!

  • Builds trafficserver successfully for CentOS 8 or CentOS 7
  • t3c integration tests succeed on both CentOS 8 and CentOS 7
  • The newer OpenSSL version is used. When running the tests on CentOS 7, ssl_server_name.yaml loads successfully (improvement from #5906 (review)):
    [Jun 18 00:12:08.135] traffic_server NOTE: loading /opt/trafficserver/etc/trafficserver/ssl_server_name.yaml
    [Jun 18 00:12:08.136] traffic_server NOTE: ssl_server_name.yaml done reloading!
    
  • GitHub Actions does not run the trafficserver unit tests

Although the t3c integration tests GHA sometimes fails, that was the case before this PR as well, and this PR does not make it succeed less frequently.

@zrhoffman zrhoffman merged commit 9944e7b into apache:master Jun 18, 2021
@jrushford jrushford deleted the ats_openssl_fix branch August 26, 2021 18:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cache-config Cache config generation improvement The functionality exists but it could be improved in some way. tests related to tests and/or testing infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants