mirrors stats script by abizer · Pull Request #169 · ocf/puppet

abizer · 2017-06-17T23:25:08Z

No description provided.

matthew-mcallister · 2017-06-17T23:30:59Z

+
+    '/usr/local/sbin/record-mirrors-stats':
+      content => template('ocf_mirrors/record-mirrors-stats.py.erb'),
+      mode    => '0644';


I don't think this should be world-readable to be consistent with our use of secrets elsewhere.

Shouldn't this be executable?

Yes, one sec will fix

You should probably store config separately as @chriskuehl says and as we do for all of our other programs.

chriskuehl · 2017-06-17T23:27:25Z

+
+    '/usr/local/sbin/record-mirrors-stats':
+      content => template('ocf_mirrors/record-mirrors-stats.py.erb'),
+      mode    => '0644';


if it's in sbin it should be executable. additionally, if it has secrets, it shouldn't be world-readable and should have show_diff => false

ideally don't template a python file though. store the config separately.

yeah, my mistake

I modeled this after how it's done in the labstats script, I guess I'll fix this and then fix that as well

it now uses environment variables, seeded by cron. The password is stored in the root crontab so it should be secure enough + no need to add another config file. Does that seem reasonable?

chriskuehl · 2017-06-17T23:27:33Z

@@ -0,0 +1,54 @@
+#!/usr/bin/python


chriskuehl · 2017-06-17T23:29:14Z

+
+import os
+import pymysql
+from datetime import date


group imports into stdlib / 3rd party / application per PEP8: https://www.python.org/dev/peps/pep-0008/#imports

pre-commit should enforce this for you?

pre-commit didn't say anything about this, I'll re-install it I guess

chriskuehl · 2017-06-17T23:29:49Z

+sources = [mirrored for mirrored in os.listdir(MIRRORS_PATH)
+           if os.path.isdir(os.path.join(MIRRORS_PATH, mirrored)) and not mirrored.startswith('.')]
+
+sources.append('other') # catchall


2 spaces before inline comments per PEP8: https://www.python.org/dev/peps/pep-0008/#inline-comments

pre-commit isn't enforcing this?

chriskuehl · 2017-06-17T23:30:16Z

+def build_dists():
+    return { mirrored: { 'up': 0, 'down': 0 } for mirrored in sources }
+
+def process_file(fn):


2 blank lines before/after functions per PEP8 (going to stop commenting these, pre-commit should do this for you)

chriskuehl · 2017-06-17T23:31:08Z

+        cursorclass=pymysql.cursors.DictCursor,
+    )
+
+    c = conn.cursor()


use a context manager

with conn as cursor: ...

chriskuehl · 2017-06-17T23:32:19Z

+        for line in f:
+            stats = line.split()
+            dist = stats[6]
+            dist = dist.split('/')[1] if '/' in dist else dist


can you include a comment with an example line so people reading the script can see what is being parsed?

chriskuehl · 2017-06-17T23:32:36Z

+
+  cron {
+    'mirrors-stats':
+      command => '/usr/local/sbin/record-mirrors-stats',


don't know how this works if it's not executable?

seems to work fine for the last two weeks I let it run, but yeah I need to make it +x

Maybe cron runs the crunchbang without checking if it's +x? Sounds fishy to me.

matthew-mcallister · 2017-06-17T23:34:22Z

+            dist = stats[6]
+            dist = dist.split('/')[1] if '/' in dist else dist
+
+            if stats[8][0] in ('2', '3') and dist in dists: # http status code is 2xx/3xx


Comment this as well as the above lines, please.

EDIT: I'm referring to the stats[8][0] part of course.

chriskuehl · 2017-06-20T16:58:44Z

@@ -0,0 +1,63 @@
+#!/usr/bin/python


python files should either drop the .py if they're not intended to be imported (and only executed), or use underscores so they can be imported

our tradition is typically to drop the extension

jvperrin · 2017-06-28T04:17:22Z

+OCFSTATS_PWD = os.environ['OCFSTATS_PWD']
+MIRRORS_PATH = '/opt/mirrors/ftp'
+LOG_PATH = '/var/log/apache2'
+LOG_NAME = 'mirrors.ocf.berkeley.edu_access.log'


I think I forgot to mention this before, but the reason why I was using mirrors.ocf.berkeley.edu_access.log.1 instead of this was because the logs get rotated at around 6 AM, so if you check them once a day, unless you check very close to when they are rotated, you lose some data. For example, this is currently checked at midnight, so all the data from midnight to around 6 AM is currently lost since it is rotated before the next midnight.

Another option I just thought of is to scan both log files and check if the date is within the current day, that might be the best if you truly want the numbers to be representative of the actual day boundaries instead of 6 AM - 6 AM the next day. (honestly I don't think it matters, but it wouldn't be too bad to scan both log files either)

abizer · 2017-06-28T04:23:13Z

I changed the cron.daily timing to be at 12:05am for precisely this reason. Was trying to figure out how to puppet that for logrotate only. On Jun 27, 2017 9:17 PM, "Jason Perrin" <notifications@github.com> wrote: *@jvperrin* commented on this pull request. ------------------------------ In modules/ocf_mirrors/files/process-mirrors-logs <#169 (comment)>:

@@ -0,0 +1,82 @@

+#!/usr/bin/python +import argparse +import os +from datetime import date + +import pymysql + +OCFSTATS_PWD = os.environ['OCFSTATS_PWD'] +MIRRORS_PATH = '/opt/mirrors/ftp' +LOG_PATH = '/var/log/apache2' +LOG_NAME = 'mirrors.ocf.berkeley.edu_access.log' I think I forgot to mention this before, but the reason why I was using mirrors.ocf.berkeley.edu_access.log.1 instead of this was because the logs get rotated at around 6 AM, so if you check them once a day, unless you check very close to when they are rotated, you lose some data. For example, this is currently checked at midnight, so all the data from midnight to around 6 AM is currently lost since it is rotated before the next midnight. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#169 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB0_lijtg8NWpmarbiqLEFFpJSwLbLNbks5sIdPTgaJpZM4N9W6j> .

jvperrin · 2017-06-28T04:25:53Z

+
+            # extract dist name from request url
+            # '/debian/pool/main/h/hwdata/...' -> 'debian'
+            dist = stats[6]


I wrote this script as a bit of a hack to quickly see what traffic levels we had across dists, but ideally there would be a comment here explaining why these indices were chosen with maybe an example line:

The format comes from here:

%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-Agent}i\" %I %O

which translates to something like

8.8.8.8 - - [26/Jun/2017:00:05:13 -0700] "GET /centos/7.3.1611/path/here/blah HTTP/1.1" 200 3735 "-" "urlgrabber/3.10 yum/3.4.3" 147 3971

so that's why the 6th index is needed (%r corresponds to the path because there is an extra space in the timestamp), and the 8th index below gives the status code so that can be filtered for only successes. -1 (%O) and -2 (%I) below give the bytes downloaded and uploaded as seen by Apache respectively.

jvperrin · 2017-06-28T05:03:36Z

Meh, I think that's a bit much and would be surprising behavior honestly that one system has a different cron.daily time than the others for a script to collect mirror metrics. I think just scraping both log files and discarding those in the wrong date would be reasonably easy. The script only runs once a day, so if it takes a few more seconds it's not a big deal at all.

chriskuehl · 2017-06-28T18:49:27Z

+1 for jvperrin's approach

@jvperrin

add mirrors stats script and cronjob thank based @jvperrin

Some history: the variable was introduced in #169 when stat tracking was added, but #925 changed the implementation and didn't use the variable any more (probably because it was so far away from where it was used). This commit moves it next to where it is used now.

* fix(mirrors): reload nginx when certs change Previously, nginx was not picking up renewed certs. * refactor(mirrors): move+use variable for ocfstats password Some history: the variable was introduced in #169 when stat tracking was added, but #925 changed the implementation and didn't use the variable any more (probably because it was so far away from where it was used). This commit moves it next to where it is used now.

matthew-mcallister reviewed Jun 17, 2017

View reviewed changes

chriskuehl reviewed Jun 17, 2017

View reviewed changes

matthew-mcallister reviewed Jun 17, 2017

View reviewed changes

chriskuehl reviewed Jun 20, 2017

View reviewed changes

jvperrin reviewed Jun 28, 2017

View reviewed changes

abizer added 4 commits August 2, 2017 02:07

add mirrors stats script and cronjob

ad4bae2

templatize mirrors stats script

2f31f1c

secrets in cron env instead of templates

9063da6

rename mirrors stats script, add argparse

2846a04

abizer force-pushed the mirrors-script branch from c997436 to 2846a04 Compare August 2, 2017 09:27

abizer added 2 commits August 2, 2017 02:38

messed up the rebase

0fc9eb5

messed up more than the rebase

1a00549

abizer merged commit b1a9353 into ocf:master Aug 2, 2017

matthew-mcallister pushed a commit to matthew-mcallister/puppet that referenced this pull request Oct 29, 2017

mirrors stats script (ocf#169)

7cd2ab3

add mirrors stats script and cronjob thank based @jvperrin

Conversation

abizer commented Jun 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriskuehl Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-mcallister Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abizer commented Jun 28, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvperrin commented Jun 28, 2017

Uh oh!

chriskuehl commented Jun 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chriskuehl Jun 17, 2017 •

edited

Loading

matthew-mcallister Jun 17, 2017 •

edited

Loading