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 2020/12/04 23:34:07 UTC

[GitHub] [trafficcontrol] zrhoffman opened a new pull request #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

zrhoffman opened a new pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354


   <!--
   ************ 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 is not related to any Issue <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
     
   This PR makes it so you can look at specific Traffic Ops logs instead having to sort through the output of all of them combined.
   
   For example, if you only want to see errors and requests:
   
   ```shell
   docker-compose exec trafficops tail -qf /var/log/traffic_ops/event.log /var/log/traffic_ops/error.log
   ```
   
   In the future, we might want to look at limiting which logs are shown for the `trafficops` to make filtering by specific log like this unnecessary. The debug log in particular is unnecessarily verbose.
   
   ## 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
   
   ## 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. -->
   - Run CDN in a Box, check that
     ```shell
     docker-compose logs -f trafficops
     ```
     shows the same logs as before
   
   - Check that
     ```shell
     docker-compose exec trafficops tail -f /var/log/traffic_ops/event.log
     ```
     
     shows requests made to Traffic Ops
   
   ## 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] Only CDN in a Box is changed, no additional tests necessary
   - [x] Existing functionality is preserved, no documentation necessary
   - [x] An update to CHANGELOG.md is not necessary
   - [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] zrhoffman commented on a change in pull request #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354#discussion_r541213861



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run-go.sh
##########
@@ -80,7 +80,8 @@ CDNCONF=/opt/traffic_ops/app/conf/cdn.conf
 DBCONF=/opt/traffic_ops/app/conf/production/database.conf
 RIAKCONF=/opt/traffic_ops/app/conf/production/riak.conf
 mkdir -p /var/log/traffic_ops
-touch /var/log/traffic_ops/traffic_ops.log
+touch "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT"
+tail -qf "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT" &

Review comment:
       Agreed on all points.
   * Adjusted various scripts so we can use `nounset` when `errexit` is set in [`fd8854ace5~...906f79b27f`](/apache/trafficcontrol/compare/fd8854ace5~...906f79b27f)
   * Setting `errexit`, `nounset`, and `pipefail` in 48ab9744ed




----------------------------------------------------------------
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] rimashah25 edited a comment on pull request #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
rimashah25 edited a comment on pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354#issuecomment-740036503


   LGTM. 
   Tested it on localhost and saw the logs for curl cmds under /var/log/traffic_ops/event.log without additional information that we normally see in trafficops logs.


----------------------------------------------------------------
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] rimashah25 commented on pull request #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354#issuecomment-740036503


   LGTM. 
   Tested it on localhost and saw the logs for curl cmds under /var/log/traffic_ops/event.log


----------------------------------------------------------------
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] TaylorCFrey commented on a change in pull request #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
TaylorCFrey commented on a change in pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354#discussion_r541313418



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run.sh
##########
@@ -143,4 +146,4 @@ export TO_PASSWORD=$TO_ADMIN_PASSWORD
 
 to-enroll "to" ALL || (while true; do echo "enroll failed."; sleep 3 ; done)
 
-exec tail -f /var/log/traffic_ops/traffic_ops.log
+exec tail -f /dev/null

Review comment:
       Whoa super cool. Thanx!




----------------------------------------------------------------
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] TaylorCFrey commented on a change in pull request #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
TaylorCFrey commented on a change in pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354#discussion_r541125780



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run.sh
##########
@@ -143,4 +146,4 @@ export TO_PASSWORD=$TO_ADMIN_PASSWORD
 
 to-enroll "to" ALL || (while true; do echo "enroll failed."; sleep 3 ; done)
 
-exec tail -f /var/log/traffic_ops/traffic_ops.log
+exec tail -f /dev/null

Review comment:
       What is this line doing? Anything? Is it necessary?

##########
File path: infrastructure/cdn-in-a-box/variables.env
##########
@@ -89,11 +89,12 @@ TO_PERL_HOST=trafficops-perl
 TO_PERL_PORT=443
 TO_PERL_SCHEME=https
 TO_SECRET=blahblah
-TO_LOG_ERROR=stdout
-TO_LOG_WARNING=stdout
-TO_LOG_INFO=stdout
-TO_LOG_DEBUG=stdout
-TO_LOG_EVENT=stdout
+TO_LOG_ERROR=/var/log/traffic_ops/error.log
+TO_LOG_WARNING=/var/log/traffic_ops/warning.log
+TO_LOG_INFO=/var/log/traffic_ops/info.log
+#TO_LOG_DEBUG=/var/log/traffic_ops/debug.log
+TO_LOG_DEBUG=/dev/null

Review comment:
       For **DEBUG**, do we want a debug.log here or should it be piped to `/dev/null`?

##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run-go.sh
##########
@@ -80,7 +80,8 @@ CDNCONF=/opt/traffic_ops/app/conf/cdn.conf
 DBCONF=/opt/traffic_ops/app/conf/production/database.conf
 RIAKCONF=/opt/traffic_ops/app/conf/production/riak.conf
 mkdir -p /var/log/traffic_ops
-touch /var/log/traffic_ops/traffic_ops.log
+touch "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT"
+tail -qf "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT" &

Review comment:
       I almost always want to start a bash script with [Unofficial Strict Mode](http://redsymbol.net/articles/unofficial-bash-strict-mode/). But specifically here, the _nounset_ flag:
   
   `set -o nounset` (or `set -u` but I always prefer the longer, descriptive flags in scripts)
   
   It will help ensure the variables we are relying on are set, and will fail if that isn't the case. Definitely helped me in the past. `pipefail` and `errexit` are also critical I feel, but I'm not sure what all paths we expect from this script.




----------------------------------------------------------------
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] TaylorCFrey commented on a change in pull request #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
TaylorCFrey commented on a change in pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354#discussion_r541326002



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/to-access.sh
##########
@@ -171,26 +171,10 @@ to-enroll() {
 
 	local serverType="$1"
 
-	if [[ ! -z "$2" ]]; then
-		export MY_CDN="$2"
-	else
-		export MY_CDN="$CDN_NAME"
-	fi
-	if [[ ! -z "$3" ]]; then
-		export MY_CACHE_GROUP="$3"
-	else
-		export MY_CACHE_GROUP="CDN_in_a_Box_Edge"
-	fi
-	if [[ ! -z "$4" ]]; then
-		export MY_TCP_PORT="$4"
-	else
-		export MY_TCP_PORT="80"
-	fi
-	if [[ ! -z "$5" ]]; then
-		export MY_HTTPS_PORT="$5"
-	else
-		export MY_HTTPS_PORT="443"
-	fi
+	export MY_CDN="${2:-$CDN_NAME}"
+	export MY_CACHE_GROUP="${3:-CDN_in_a_Box_Edge}"
+	export MY_TCP_PORT="${4:-80}"
+	export MY_HTTPS_PORT="${5:-443}"

Review comment:
       I like this shorthand! I'll need to remember this the next time I chain if/elses




----------------------------------------------------------------
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 #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354


   


----------------------------------------------------------------
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] TaylorCFrey commented on pull request #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
TaylorCFrey commented on pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354#issuecomment-743443216


   Code and scripts look good to me. The distinct and verbose logging will definitely be beneficial.


----------------------------------------------------------------
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 #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354#discussion_r541144258



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run.sh
##########
@@ -143,4 +146,4 @@ export TO_PASSWORD=$TO_ADMIN_PASSWORD
 
 to-enroll "to" ALL || (while true; do echo "enroll failed."; sleep 3 ; done)
 
-exec tail -f /var/log/traffic_ops/traffic_ops.log
+exec tail -f /dev/null

Review comment:
       Starting at #4375, the `trafficops` service would only run `tail -f /dev/null` at the end if `TO_DEBUG_ENABLE` was set to `true` (to allow you to copy in a new `traffic_ops_golang` binary and start it without restarting the service).
   
   #5055 re-added `tail -f /dev/null` for all cases and added a health check to the `trafficops` service so that if someone runs `docker-compose ps` but `traffic_ops_golang` itself is not running, they will see that the container is unhealthy.




----------------------------------------------------------------
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 #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354#discussion_r541131853



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run.sh
##########
@@ -143,4 +146,4 @@ export TO_PASSWORD=$TO_ADMIN_PASSWORD
 
 to-enroll "to" ALL || (while true; do echo "enroll failed."; sleep 3 ; done)
 
-exec tail -f /var/log/traffic_ops/traffic_ops.log
+exec tail -f /dev/null

Review comment:
       it's necessary to keep a running process otherwise the container exits, destroying the service. Ideally the service itself would be that process, but because it needs to enroll itself and it can't do that until the service is started, we needed to run the service in the background.




----------------------------------------------------------------
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 #5354: Tail Traffic Ops and Traffic Ops Perl logs from log files

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5354:
URL: https://github.com/apache/trafficcontrol/pull/5354#discussion_r541136536



##########
File path: infrastructure/cdn-in-a-box/variables.env
##########
@@ -89,11 +89,12 @@ TO_PERL_HOST=trafficops-perl
 TO_PERL_PORT=443
 TO_PERL_SCHEME=https
 TO_SECRET=blahblah
-TO_LOG_ERROR=stdout
-TO_LOG_WARNING=stdout
-TO_LOG_INFO=stdout
-TO_LOG_DEBUG=stdout
-TO_LOG_EVENT=stdout
+TO_LOG_ERROR=/var/log/traffic_ops/error.log
+TO_LOG_WARNING=/var/log/traffic_ops/warning.log
+TO_LOG_INFO=/var/log/traffic_ops/info.log
+#TO_LOG_DEBUG=/var/log/traffic_ops/debug.log
+TO_LOG_DEBUG=/dev/null

Review comment:
       `/dev/null` is intentional. Debug-level logs are excessively verbose and include the SQL for queries. I kept that `TO_LOG_DEBUG=/var/log/traffic_ops/debug.log` line there in case anyone *does* want to see the debug output.




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