You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/08/04 14:50:51 UTC
[GitHub] [trafficcontrol] ezelkow1 opened a new pull request, #7003: Update dockerfile to include various required packages
ezelkow1 opened a new pull request, #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #ISSUE
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
<!-- **^ Add meaningful description above** --><hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Automation (docker images) <!-- Please specify which (GitHub Actions, Docker images, Ansible Roles, etc.) -->
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
Currently some of the tests are broken because the docker image is lacking some requirements for the ats build. Adding those to the docker file
## If this is a bugfix, which Traffic Control versions contained the bug?
<!-- Delete this section if the PR is not a bugfix, or if the bug is only in the master branch.
Examples:
- 5.1.2
- 5.1.3 (RC1)
-->
## PR submission checklist
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman merged pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
zrhoffman merged PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938061786
##########
cache-config/testing/docker/trafficserver/Dockerfile:
##########
@@ -63,10 +63,12 @@ RUN if [[ ${OS_VERSION%%.*} -ge 8 ]]; then \
curl \
gcc-toolset-9 \
gcc-toolset-9-runtime \
+ geoip \
jansson \
jansson-devel \
libmaxminddb \
- libmaxminddb-devel); \
+ libmaxminddb-devel \
+ libuuid ); \
Review Comment:
`rockylinux:8`, `centos:7`, and `centos:8` all include the `libuuid` package. Is specifying it needed?
```shell
[zrhoffman@computer ~]$ for image in rockylinux:8 centos:{7,8}; do docker run --rm "$image" rpm -q libuuid; done
libuuid-2.32.1-35.el8.x86_64
libuuid-2.23.2-65.el7.x86_64
libuuid-2.32.1-27.el8.x86_64
```
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
`rockylinux:8`, `centos:7`, and `centos:8` all include the `libuuid` package. Is specifying it needed?
```shell
[zrhoffman@computer ~]$ for image in rockylinux:8 centos:{7,8}; do docker run --rm "$image" rpm -q libuuid; done
libuuid-2.32.1-35.el8.x86_64
libuuid-2.23.2-65.el7.x86_64
libuuid-2.32.1-27.el8.x86_64
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938088749
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
We're seeing failures with our internal build system with Rocky8, on the TO API Tests. We're not entirely sure why it's succeeding in Github, but failing on our internal build.
The error is
```
infrastructure/cdn-in-a-box/docker-compose -p ATCN-toapi-7 -f docker-compose.traffic-ops-test.yml -f docker-compose.yml --no-ansi build --parallel --no-cache edge enroller dns db smtp trafficops integration
rpm -Uvh /trafficserver.rpm
---> Running in 4eaac11a7b28
error: Failed dependencies:
xz is needed by trafficserver-13492:9.1.2-22.b965ea10a.el8.x86_64
```
It looks like the `rpm` call doesn't install dependencies. I'm not sure how it's succeeding in Github here.
Every PR run I look at is using a cached RPM. Is it possible a clean build in Github that didn't use the cache would also fail?
We're still investigating.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938160531
##########
cache-config/testing/docker/trafficserver/Dockerfile:
##########
@@ -63,10 +63,12 @@ RUN if [[ ${OS_VERSION%%.*} -ge 8 ]]; then \
curl \
gcc-toolset-9 \
gcc-toolset-9-runtime \
+ geoip \
Review Comment:
I see, the CiaB Cache file just capitalizes the dependency `GeoIP`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938069106
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
Works for me :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938107434
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
> It looks like the rpm call doesn't install dependencies. I'm not sure how it's succeeding in Github here.
It uses rpm to install the trafficserver RPM in order to assert that all of the dependencies are installed in a previous docker layer. This is preferable for users of CDN in a Box in order to reduce the build time when the trafficserver RPM changes, as
* `rpm` runs much faster than `yum`
* the same dependencies don't need to be repeatedly reinstalled.
> Every PR run I look at is using a cached RPM. Is it possible a clean build in Github that didn't use the cache would also fail?
While it is true that the ATS RPM is usually cached, the cache key includes the latest release tag and latest commit on the release branch used (currently `8.1.x`), so it does update whenever the `8.1.x` release branch does. For example, from GHA run [7676647738](https://github.com/apache/trafficcontrol/runs/7676647738) of the *trafficserver* job of the * TC Health Client Integration Tests* GHA workflow:
```
Cache restored from key: trafficserver-8.1.4-3.61478f21e.el8.x86_64.rpm-13333c2156bae36275075764d90aabd12b94c622db675d823b68cd3ca382cfb9
```
from
https://github.com/apache/trafficcontrol/blob/f08435a625fd007198b9e993f47d4cdb9c9821d5/.github/workflows/cache-config-tests.yml#L122 https://github.com/apache/trafficcontrol/blob/f08435a625fd007198b9e993f47d4cdb9c9821d5/.github/workflows/cache-config-tests.yml#L122 and https://github.com/apache/trafficcontrol/blob/f08435a625fd007198b9e993f47d4cdb9c9821d5/.github/workflows/health-client-tests.yml#L123
> ```
> xz is needed by trafficserver-13492:9.1.2-22.b965ea10a.el8.x86_64
> ```
That would be nice if the GHAs used an ATS 9 RPM instead. Since #7003 moves us in that direction, I'm all for merging it, but [the existing ATS RPM SPEC file](https://github.com/apache/trafficcontrol/blob/f08435a625/cache-config/testing/docker/trafficserver/trafficserver.spec) fails to build for ATS 9 (IIRC it said something.yml did not exist), so that will need to be updated, too. I'll open a proper issue for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938107434
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
> It looks like the rpm call doesn't install dependencies. I'm not sure how it's succeeding in Github here.
It uses rpm to install the trafficserver RPM in order to assert that all of the dependencies are installed in a previous docker layer. This is preferable for users of CDN in a Box in order to reduce the build time when the trafficserver RPM changes, as
* `rpm` runs much faster than `yum`
* the same dependencies don't need to be repeatedly reinstalled.
> Every PR run I look at is using a cached RPM. Is it possible a clean build in Github that didn't use the cache would also fail?
While it is true that the ATS RPM is usually cached, the cache key includes the latest release tag and latest commit on the release branch used (currently `8.1.x`), so it does update whenever the `8.1.x` release branch does. For example, from GHA run [7676647738](https://github.com/apache/trafficcontrol/runs/7676647738) of the *trafficserver* job of the *TC Health Client Integration Tests* GHA workflow:
```
Cache restored from key: trafficserver-8.1.4-3.61478f21e.el8.x86_64.rpm-13333c2156bae36275075764d90aabd12b94c622db675d823b68cd3ca382cfb9
```
from
https://github.com/apache/trafficcontrol/blob/f08435a625fd007198b9e993f47d4cdb9c9821d5/.github/workflows/cache-config-tests.yml#L122 https://github.com/apache/trafficcontrol/blob/f08435a625fd007198b9e993f47d4cdb9c9821d5/.github/workflows/cache-config-tests.yml#L122 and https://github.com/apache/trafficcontrol/blob/f08435a625fd007198b9e993f47d4cdb9c9821d5/.github/workflows/health-client-tests.yml#L123
> ```
> xz is needed by trafficserver-13492:9.1.2-22.b965ea10a.el8.x86_64
> ```
That would be nice if the GHAs used an ATS 9 RPM instead. Since #7003 moves us in that direction, I'm all for merging it, but [the existing ATS RPM SPEC file](https://github.com/apache/trafficcontrol/blob/f08435a625/cache-config/testing/docker/trafficserver/trafficserver.spec) fails to build for ATS 9 (IIRC it said something.yml did not exist), so that will need to be updated, too. I'll open a proper issue for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938160085
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
Opened #7006 for ATS 9.1.x failing to build
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938109883
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
> While it is true that the ATS RPM is usually cached, the cache key includes the latest release tag and latest commit on the release branch used (currently `8.1.x`), so it does update whenever the `8.1.x` release branch does.
The cache is also busted whenever any file in `cache-config/testing/docker/trafficserver/**` is updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938068183
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
Even if CentOS includes it by default, if ATS needs it, specifying it will make the Dockerfile more portable, easier to switch base images to other things that might not have it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938070464
##########
cache-config/testing/docker/trafficserver/Dockerfile:
##########
@@ -63,10 +63,12 @@ RUN if [[ ${OS_VERSION%%.*} -ge 8 ]]; then \
curl \
gcc-toolset-9 \
gcc-toolset-9-runtime \
+ geoip \
Review Comment:
Why add the `geoip` dependency to t3c integration test's trafficserver-building Dockerfile but not the CDN-in-a-Box cache Dockerfile?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938155402
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
I made a test PR to not use the cache, and it succeeded:
https://github.com/apache/trafficcontrol/runs/7678948197?check_suite_focus=true
```
Wrote: /root/rpmbuild/RPMS/x86_64/trafficserver-8.1.4-3.61478f21e.el8.x86_64.rpm
```
So, not sure what the difference is with our internal build. Will continue to investigate
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938013787
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
"not ... enabled" meaning they aren't running in GHAs or haven't even been written yet?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r937962760
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
please match indentation of the rest of the file. Currently, instead of real indentation this file uses spaces. If you'd prefer to instead convert this file to use real indentation, that's fine also.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ezelkow1 commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r937991143
##########
infrastructure/cdn-in-a-box/cache/Dockerfile:
##########
@@ -60,6 +60,7 @@ RUN dnf -y install epel-release && \
libmaxminddb \
libtool-ltdl \
libunwind \
+ libuuid \
Review Comment:
switched to spaces
wrt to the tests, we are seeing them internally but I believe they have not been enabled yet on open source, so this is in preparation of that. Also to provide future compatibility with ats 9.2 rpms
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7003: Update dockerfile to include various required packages
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on code in PR #7003:
URL: https://github.com/apache/trafficcontrol/pull/7003#discussion_r938070464
##########
cache-config/testing/docker/trafficserver/Dockerfile:
##########
@@ -63,10 +63,12 @@ RUN if [[ ${OS_VERSION%%.*} -ge 8 ]]; then \
curl \
gcc-toolset-9 \
gcc-toolset-9-runtime \
+ geoip \
Review Comment:
Why add the `geoip` dependency to t3c integration tests' trafficserver-building Dockerfile but not the CDN-in-a-Box cache Dockerfile?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org