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 2021/06/07 13:07:48 UTC
[GitHub] [trafficcontrol] zrhoffman opened a new pull request #5920: Use ATS 8.1 in CDN in a Box
zrhoffman opened a new pull request #5920:
URL: https://github.com/apache/trafficcontrol/pull/5920
<!--
************ STOP!! ************
If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
-->
## What does this PR (Pull Request) do?
<!-- Explain the changes you made here. If this fixes an Issue, identify it by
replacing the text in the checkbox item with the Issue number e.g.
- [x] This PR fixes #9001 OR is not related to any Issue
^ This will automatically close Issue number 9001 when the Pull Request is
merged (The '#' is important).
Be sure you check the box properly, see the "The following criteria are ALL
met by this PR" section for details.
-->
- [x] This PR fixes #5918 by using a Traffic Server RPM built for the latest version of CentOS 8 and getting the *CDN-in-a-Box CI* GitHub Actions workflow to pass.<!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
- [x] This PR fixes #5919 by making CDN in a Box use a `trafficserver` RPM built from the 8.1.x ATS release branch.<!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this
Pull Request. Also, feel free to add the name of a tool or script that is
affected but not on the list.
Additionally, if this Pull Request does NOT affect documentation, please
explain why documentation is not required. -->
- CDN in a Box
- Documentation
- GitHub Actions
- T3C integration tests
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your Pull Request. If
it includes tests (and most should), outline here the steps needed to run the
tests. If not, lay out the manual testing procedure and please explain why
tests are unnecessary for this Pull Request. -->
1. Build and run CDN in a Box, ensure caches are reported as healthy on http://trafficmonitor.infra.ciab.test/
```shell
make -j6
docker-compose build --parallel
docker-compose kill && docker-compose down -v && docker-compose up -d
docker-compose logs -f trafficmonitor
```
2. Build and run CDN in a Box for CentOS 7, ensure caches are reported as healthy on http://trafficmonitor.infra.ciab.test/
```shell
export RHEL_VERSION=7
make -j6
docker-compose build --parallel
docker-compose kill && docker-compose down -v && docker-compose up -d
docker-compose logs -f trafficmonitor
```
3. Check the result of the *CDN-in-a-Box CI* GitHub Actions workflow
## If this is a bug fix, what versions of Traffic Control are affected?
<!-- If this PR fixes a bug, please list here all of the affected versions - to
the best of your knowledge. It's also pretty helpful to include a commit hash
of where 'master' is at the time this PR is opened (if it affects master),
because what 'master' means will change over time. For example, if this PR
fixes a bug that's present in master (at commit hash '1df853c8'), in v4.0.0,
and in the current 4.0.1 Release candidate (e.g. RC1), then this list would
look like:
- master (1df853c8)
- 4.0.0
- 4.0.1 (RC1)
If you don't know what other versions might have this bug, AND don't know how
to find the commit hash of 'master', then feel free to leave this section
blank (or, preferably, delete it entirely).
-->
- master (5ad059f0af)
- 5.1.2
## The following criteria are ALL met by this PR
<!-- Check the boxes to signify that the associated statement is true. To
"check a box", replace the space inside of the square brackets with an 'x'.
e.g.
- [ x] <- Wrong
- [x ] <- Wrong
- [] <- Wrong
- [*] <- Wrong
- [x] <- Correct!
-->
- [x] This PR includes tests
- [x] This PR includes documentation
- [x] This PR includes an update to CHANGELOG.md
- [x] This PR includes any and all required license headers
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5920: Use ATS 8.1 in CDN in a Box
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5920:
URL: https://github.com/apache/trafficcontrol/pull/5920#discussion_r647790159
##########
File path: infrastructure/cdn-in-a-box/cache/run.sh
##########
@@ -89,6 +126,6 @@ chmod "0644" "/etc/cron.d/traffic_ops_ort-cron" && crontab "/etc/cron.d/traffic_
crond -im off
-touch /var/log/trafficserver/diags.log
+touch /opt/trafficserver/var/log/trafficserver/diags.log
# Leaves the container hanging open in the event of a failure for debugging purposes
-tail -Fn +1 /var/log/trafficserver/diags.log /var/log/ort.log
+tail -Fn+1 /opt/trafficserver/var/log/trafficserver/diags.log /var/log/ort.log
Review comment:
Should these also be determined from the rpm? Not the ORT log file, that's probably fine, just the ATS stuff
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5920: Use ATS 8.1 in CDN in a Box
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5920:
URL: https://github.com/apache/trafficcontrol/pull/5920#discussion_r647676999
##########
File path: infrastructure/cdn-in-a-box/traffic_ops_data/profiles/010-ATS_EDGE_TIER_CACHE.json
##########
@@ -87,7 +87,7 @@
"configFile": "records.config",
"name": "CONFIG proxy.config.log.logfile_dir",
"secure": false,
- "value": "STRING /var/log/trafficserver"
+ "value": "STRING /opt/trafficserver/var/log/trafficserver"
Review comment:
Getting logfile_dir from the RPM in 4486105741
##########
File path: infrastructure/cdn-in-a-box/bin/ats-version.sh
##########
@@ -0,0 +1,52 @@
+#!/usr/bin/env bash
+# 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.
+
+trap 'echo "Error on line ${LINENO} of ${0}" >/dev/stderr; exit 1' ERR;
+set -o errexit -o nounset -o pipefail
+
+project=trafficserver
+script_dir="$(dirname "$0")"
+ats_version_file="${script_dir}/../cache/ATS_VERSION"
+
+remote_ats_version() {
+ local gitbox_url=https://gitbox.apache.org/repos/asf
+ local repo="${project}.git"
+ local branch refs commit last_tag release
Review comment:
Using good-person spacing in a2a18f830b
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -164,6 +180,9 @@ $(ORT_RPM_ABSOLUTE): $(ORT_DIST_RPM)
cp -f "$?" "$@"
# Dist rpms
+$(ATS_DIST_RPM): $(ATS_SOURCE)
+ 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
Review comment:
Expanding from some variable in fdb6f38212.
##########
File path: .github/workflows/ciab.yaml
##########
@@ -215,16 +215,52 @@ jobs:
name: ${{ github.job }}
path: ${{ github.workspace }}/dist/${{ github.job }}-*.rpm
+ trafficserver:
+ if: github.event.pull_request.draft == false
+ runs-on: ubuntu-latest
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v2
+ - name: Get commit sha
+ uses: ./.github/actions/fetch-github-branch-sha
+ with:
+ owner: apache
+ repo: trafficserver
+ branch: 8.1.x
+ id: git-repo-sha
+ - name: Display SHA
+ run: echo "${{ steps.git-repo-sha.outputs.sha }}"
+ - name: Check Cache
+ id: ats-rpm-cache
+ uses: actions/cache@v2
+ with:
+ path: ${{ github.workspace }}/dist
+ key: ${{ steps.git-repo-sha.outputs.sha }}-el${{ env.RHEL_VERSION }}-${{ hashFiles('cache-config/testing/docker/trafficserver/**') }}
+ - name: Build ATS RPM
+ if: steps.ats-rpm-cache.outputs.cache-hit != 'true'
+ uses: ./.github/actions/build-ats-test-rpm
+ env:
+ ATC_COMPONENT: ${{ github.job }}
+ - name: Display structure of dist directory
+ run: ls -lR
+ working-directory: ${{ github.workspace }}/dist
Review comment:
I just copy/pasted the `trafficserver` job from the [*Traffic Control Cache Config integration tests*](https://github.com/apache/trafficcontrol/blob/6f71cfa2dcd6f4684d423c22187d5e2662916a25/.github/workflows/cache-config-tests.yml#L93-L126) workflow. Removed the *Display SHA* step from both workflows in b4d175c802.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5920: Use ATS 8.1 in CDN in a Box
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5920:
URL: https://github.com/apache/trafficcontrol/pull/5920#discussion_r647538312
##########
File path: infrastructure/cdn-in-a-box/traffic_ops_data/profiles/010-ATS_EDGE_TIER_CACHE.json
##########
@@ -87,7 +87,7 @@
"configFile": "records.config",
"name": "CONFIG proxy.config.log.logfile_dir",
"secure": false,
- "value": "STRING /var/log/trafficserver"
+ "value": "STRING /opt/trafficserver/var/log/trafficserver"
Review comment:
Shouldn't this be determined from the RPM the same way the configuration files are for the `chown` step?
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -164,6 +180,9 @@ $(ORT_RPM_ABSOLUTE): $(ORT_DIST_RPM)
cp -f "$?" "$@"
# Dist rpms
+$(ATS_DIST_RPM): $(ATS_SOURCE)
+ 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
Review comment:
Don't these `../../`s need to be expanded from some variable?
##########
File path: infrastructure/cdn-in-a-box/bin/ats-version.sh
##########
@@ -0,0 +1,52 @@
+#!/usr/bin/env bash
+# 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.
+
+trap 'echo "Error on line ${LINENO} of ${0}" >/dev/stderr; exit 1' ERR;
+set -o errexit -o nounset -o pipefail
+
+project=trafficserver
+script_dir="$(dirname "$0")"
+ats_version_file="${script_dir}/../cache/ATS_VERSION"
+
+remote_ats_version() {
+ local gitbox_url=https://gitbox.apache.org/repos/asf
+ local repo="${project}.git"
+ local branch refs commit last_tag release
Review comment:
Inconsistent indentation. You can use whatever you want as long as it's consistent, but just remember that tabs are inherently superior and if you use spaces you're a bad person.
##########
File path: .github/workflows/ciab.yaml
##########
@@ -215,16 +215,52 @@ jobs:
name: ${{ github.job }}
path: ${{ github.workspace }}/dist/${{ github.job }}-*.rpm
+ trafficserver:
+ if: github.event.pull_request.draft == false
+ runs-on: ubuntu-latest
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v2
+ - name: Get commit sha
+ uses: ./.github/actions/fetch-github-branch-sha
+ with:
+ owner: apache
+ repo: trafficserver
+ branch: 8.1.x
+ id: git-repo-sha
+ - name: Display SHA
+ run: echo "${{ steps.git-repo-sha.outputs.sha }}"
+ - name: Check Cache
+ id: ats-rpm-cache
+ uses: actions/cache@v2
+ with:
+ path: ${{ github.workspace }}/dist
+ key: ${{ steps.git-repo-sha.outputs.sha }}-el${{ env.RHEL_VERSION }}-${{ hashFiles('cache-config/testing/docker/trafficserver/**') }}
+ - name: Build ATS RPM
+ if: steps.ats-rpm-cache.outputs.cache-hit != 'true'
+ uses: ./.github/actions/build-ats-test-rpm
+ env:
+ ATC_COMPONENT: ${{ github.job }}
+ - name: Display structure of dist directory
+ run: ls -lR
+ working-directory: ${{ github.workspace }}/dist
Review comment:
This step and the "Display SHA" seem like the GHA equivalent of `console.debug("myVar is:", myVar)` to me - do you think those add continued value now that the workflow is written?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5920: Use ATS 8.1 in CDN in a Box
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5920:
URL: https://github.com/apache/trafficcontrol/pull/5920#discussion_r647809916
##########
File path: infrastructure/cdn-in-a-box/cache/run.sh
##########
@@ -89,6 +126,6 @@ chmod "0644" "/etc/cron.d/traffic_ops_ort-cron" && crontab "/etc/cron.d/traffic_
crond -im off
-touch /var/log/trafficserver/diags.log
+touch /opt/trafficserver/var/log/trafficserver/diags.log
# Leaves the container hanging open in the event of a failure for debugging purposes
-tail -Fn +1 /var/log/trafficserver/diags.log /var/log/ort.log
+tail -Fn+1 /opt/trafficserver/var/log/trafficserver/diags.log /var/log/ort.log
Review comment:
Using the gotten logfile_dir value here has well in b1ef527c1e.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 merged pull request #5920: Use ATS 8.1 in CDN in a Box
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #5920:
URL: https://github.com/apache/trafficcontrol/pull/5920
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5920: Use ATS 8.1 in CDN in a Box
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5920:
URL: https://github.com/apache/trafficcontrol/pull/5920#discussion_r647809916
##########
File path: infrastructure/cdn-in-a-box/cache/run.sh
##########
@@ -89,6 +126,6 @@ chmod "0644" "/etc/cron.d/traffic_ops_ort-cron" && crontab "/etc/cron.d/traffic_
crond -im off
-touch /var/log/trafficserver/diags.log
+touch /opt/trafficserver/var/log/trafficserver/diags.log
# Leaves the container hanging open in the event of a failure for debugging purposes
-tail -Fn +1 /var/log/trafficserver/diags.log /var/log/ort.log
+tail -Fn+1 /opt/trafficserver/var/log/trafficserver/diags.log /var/log/ort.log
Review comment:
Using the gotten logfile_dir value here as well in b1ef527c1e.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org