Finish writing and testing service scheduling code.

bin/pt-agent

@@ -4421,6 +4421,7 @@ sub main {
    # TODO: only the main proc needs write access
    # ########################################################################
    my $config_file = get_config_file();
+   _info("Config file: $config_file");
    if ( -f $config_file ) {
       die "$config_file is not writable.\n"
          unless -w $config_file;
@@ -4723,6 +4724,7 @@ sub run_agent {
                );
                schedule_services(
                   services => $new_services,
+                  lib_dir  => $lib_dir,
                );
                $services = $new_services;
             }
@@ -4838,21 +4840,30 @@ sub schedule_services {
 
    have_required_args(\%args, qw(
       services
+      lib_dir
    )) or die;
    my $services = $args{services};
+   my $lib_dir  = $args{lib_dir};
 
    _info("Scheduling services");
 
    my $new_crontab = make_new_crontab(%args);
-   _info("New crontab:\n", $new_crontab);
+   _info("New crontab:\n" . $new_crontab || '');
 
-   my ($fh, $file) = tempfile();
+   my $crontab_file = "$lib_dir/crontab";
-   print { $fh } $new_crontab;
+   open my $fh, '>', $crontab_file
-   close $fh;
+      or die "Error opening $crontab_file: $OS_ERROR";
+   print { $fh } $new_crontab
+      or die "Error writing to $crontab_file: $OS_ERROR";
+   close $fh
+      or die "Error closing $crontab_file: $OS_ERROR";
 
-   system("crontab $file");
+   my $err_file = "$lib_dir/crontab.err";
-
+   system("crontab $crontab_file > $err_file 2>&1");
-   unlink $file;
+   if ( $CHILD_ERROR ) {
+      my $error = `cat $err_file`;
+      die "Error setting new crontab: $error\n";
+   }
 
    return;
 }
@@ -4866,7 +4877,8 @@ sub make_new_crontab {
    my $services = $args{services};
    
    # Optional args
-   my $crontab_list = $args{crontab_list} || `crontab -l`;
+   my $crontab_list = defined $args{crontab_list} ? $args{crontab_list}
+                    :                               `crontab -l 2>/dev/null`;
 
    my @other_lines
       = grep { $_ !~ m/pt-agent --run-service/ }
@@ -4878,7 +4890,7 @@ sub make_new_crontab {
          $service->schedule . " pt-agent --run-service " . $service->name;
    }
 
-   my $new_crontab = join("\n", @other_lines, @pt_agent_lines, "\n");
+   my $new_crontab = join("\n", @other_lines, @pt_agent_lines) . "\n";
 
    return $new_crontab;
 }
@@ -4907,7 +4919,6 @@ sub send_data {
 sub get_config_file {
    my $home_dir    = $ENV{HOME} || $ENV{HOMEPATH} || $ENV{USERPROFILE} || '.';
    my $config_file = "$home_dir/.pt-agent.conf";
-   _info("Config file: $config_file");
    return $config_file;
 }
 

t/pt-agent/make_new_crontab.t

@@ -11,7 +11,7 @@ use warnings FATAL => 'all';
 use English qw(-no_match_vars);
 use Test::More;
 use JSON;
-use File::Temp qw(tempdir);
+use File::Temp qw(tempfile);
 
 use Percona::Test;
 require "$trunk/bin/pt-agent";
@@ -24,14 +24,12 @@ sub test_make_new_crontab {
    my (%args) = @_;
    have_required_args(\%args, qw(
       file
-      name
       services
    )) or die;
    my $file     = $args{file};
-   my $name     = $args{name};
    my $services = $args{services};
 
-   my $crontab_list   = slurp_file("$trunk/$sample/$file.in");
+   my $crontab_list = slurp_file("$trunk/$sample/$file.in");
 
    my $new_crontab = pt_agent::make_new_crontab(
       services     => $services,
@@ -44,14 +42,10 @@ sub test_make_new_crontab {
          "$sample/$file.out",
          cmd_output => 1,
       ),
-      "$name"
+      $args{name} || $file,
    );
 } 
 
-# #############################################################################
-# Empty crontab, new service.
-# #############################################################################
-
 my $run0 = Percona::WebAPI::Resource::Run->new(
    number  => '0',
    program => 'pt-query-digest',
@@ -66,12 +60,75 @@ my $svc0 = Percona::WebAPI::Resource::Service->new(
    runs     => [ $run0 ],
 );
 
+# Empty crontab, add the service.
 test_make_new_crontab(
-   name => "crontab001",
    file => "crontab001",
    services => [ $svc0 ],
 );
 
+# Crontab has another line, add the service to it.
+test_make_new_crontab(
+   file => "crontab002",
+   services => [ $svc0 ],
+);
+
+# Crontab has another line and an old service, remove the old service
+# and add the current service.
+test_make_new_crontab(
+   file => "crontab003",
+   services => [ $svc0 ],
+);
+
+# #############################################################################
+# Use real crontab.
+# #############################################################################
+
+# The previous tests pass in a crontab file to make testing easier.
+# Now test that make_new_crontab() will run `crontab -l' if not given
+# input.  To test this, we add a fake line to our crontab.  If
+# make_new_crontab() really runs `crontab -l', then this fake line
+# will be in the new crontab it returns.
+
+my $crontab = `crontab -l 2>/dev/null`;
+SKIP: {
+   skip 'Crontab is not empty', 3 if $crontab;
+
+   my ($fh, $file) = tempfile();
+   print {$fh} "* 0  *  *  *  date > /dev/null";
+   close $fh or warn "Cannot close $file: $OS_ERROR";
+   my $output = `crontab $file 2>&1`;
+
+   $crontab = `crontab -l 2>&1`;
+
+   is(
+      $crontab,
+      "* 0  *  *  *  date > /dev/null",
+      "Set other crontab line"
+   ) or diag($output);
+
+   unlink $file or warn "Cannot remove $file: $OS_ERROR";
+
+   my $new_crontab = pt_agent::make_new_crontab(
+      services => [ $svc0 ],
+   );
+
+   is(
+      $new_crontab,
+      "* 0  *  *  *  date > /dev/null
+* 8 * * 1,2,3,4,5 pt-agent --run-service query-monitor
+",
+      "Runs crontab -l by default"
+   );
+
+   system("crontab -r 2>/dev/null");
+   $crontab = `crontab -l 2>/dev/null`;
+   is(
+      $crontab,
+      "",
+      "Removed crontab"
+   );
+};
+
 # #############################################################################
 # Done.
 # #############################################################################

t/pt-agent/run_agent.t

@@ -20,6 +20,15 @@ require "$trunk/bin/pt-agent";
 Percona::Toolkit->import(qw(Dumper));
 Percona::WebAPI::Representation->import(qw(as_hashref));
 
+# Running the agent is going to cause it to schedule the services,
+# i.e. write a real crontab.  The test box/user shouldn't have a
+# crontab, so we'll warn and clobber it if there is one.
+my $crontab = `crontab -l 2>/dev/null`;
+if ( $crontab ) {
+   warn "Removing crontab: $crontab\n";
+   `crontab -r`;
+}
+
 # #############################################################################
 # Create mock client and Agent
 # #############################################################################
@@ -136,7 +145,7 @@ my $run0 = Percona::WebAPI::Resource::Run->new(
 my $svc0 = Percona::WebAPI::Resource::Service->new(
    name     => 'query-monitor',
    alias    => 'Query Monitor',
-   schedule => '...',
+   schedule => '* * * * *',
    runs     => [ $run0 ],
 );
 
@@ -232,7 +241,16 @@ ok(
    "query-monitor service file"
 );
 
+$crontab = `crontab -l 2>/dev/null`;
+like(
+   $crontab,
+   qr/pt-agent --run-service query-monitor$/m,
+   "Scheduled service with crontab"
+);
+
+# #############################################################################
 # Run run_agent() again, like the agent had been stopped and restarted.
+# #############################################################################
 
 $ua->{responses}->{get} = [
    # First check, fail
@@ -308,9 +326,21 @@ ok(
    "No Service diff, no service file changes"
 );
 
+my $new_crontab = `crontab -l 2>/dev/null`;
+is(
+   $new_crontab,
+   $crontab,
+   "Crontab is the same"
+);
+
 # #############################################################################
 # Done.
 # #############################################################################
+
+# This shouldn't cause an error, but if it does, let it show up
+# in the results as an error.
+`crontab -r`;
+
 if ( -f $config_file ) {
    unlink $config_file 
       or warn "Error removing $config_file: $OS_ERROR";

t/pt-agent/samples/crontab001.out

@@ -1,2 +1 @@
 * 8 * * 1,2,3,4,5 pt-agent --run-service query-monitor
-

t/pt-agent/samples/crontab002.in

@@ -0,0 +1 @@
+17 3 * * 1 cmd

t/pt-agent/samples/crontab002.out

@@ -0,0 +1,2 @@
+17 3 * * 1 cmd
+* 8 * * 1,2,3,4,5 pt-agent --run-service query-monitor

t/pt-agent/samples/crontab003.in

@@ -0,0 +1,3 @@
+17 3 * * 1 cmd
+* * * * 1 pt-agent --run-service old-service
+

t/pt-agent/samples/crontab003.out

@@ -0,0 +1,2 @@
+17 3 * * 1 cmd
+* 8 * * 1,2,3,4,5 pt-agent --run-service query-monitor

t/pt-agent/samples/service001

@@ -1 +1 @@
-{"runs":[{"number":"0","options":"--output json","output":"spool","program":"pt-query-digest"}],"name":"query-monitor","alias":"Query Monitor","schedule":"..."}
+{"runs":[{"number":"0","options":"--output json","output":"spool","program":"pt-query-digest"}],"name":"query-monitor","alias":"Query Monitor","schedule":"* * * * *"}

t/pt-agent/schedule_services.t

@@ -0,0 +1,182 @@
+#!/usr/bin/env perl
+
+BEGIN {
+   die "The PERCONA_TOOLKIT_BRANCH environment variable is not set.\n"
+      unless $ENV{PERCONA_TOOLKIT_BRANCH} && -d $ENV{PERCONA_TOOLKIT_BRANCH};
+   unshift @INC, "$ENV{PERCONA_TOOLKIT_BRANCH}/lib";
+};
+
+use strict;
+use warnings FATAL => 'all';
+use English qw(-no_match_vars);
+use Test::More;
+use JSON;
+use File::Temp qw(tempfile tempdir);
+
+use Percona::Test;
+require "$trunk/bin/pt-agent";
+
+my $crontab = `crontab -l 2>/dev/null`;
+if ( $crontab ) {
+   plan skip_all => 'Crontab is not empty';
+}
+
+Percona::Toolkit->import(qw(have_required_args Dumper));
+
+my $sample = "t/pt-agent/samples";
+my $tmpdir = tempdir("/tmp/pt-agent.$PID.XXXXXX", CLEANUP => 1);
+
+# #############################################################################
+# Schedule a good crontab.
+# #############################################################################
+
+my $run0 = Percona::WebAPI::Resource::Run->new(
+   number  => '0',
+   program => 'pt-query-digest',
+   options => '--output json',
+   output  => 'spool',
+);
+
+my $svc0 = Percona::WebAPI::Resource::Service->new(
+   name     => 'query-monitor',
+   alias    => 'Query Monitor',
+   schedule => '* 8 * * 1,2,3,4,5',
+   runs     => [ $run0 ],
+);
+
+# First add a fake line so we can know that the real, existing
+# crontab is used and not clobbered.
+my ($fh, $file) = tempfile();
+print {$fh} "* 0  *  *  *  date > /dev/null";
+close $fh or warn "Cannot close $file: $OS_ERROR";
+my $output = `crontab $file 2>&1`;
+
+$crontab = `crontab -l 2>&1`;
+
+is(
+   $crontab,
+   "* 0  *  *  *  date > /dev/null",
+   "Set other crontab line"
+) or diag($output);
+
+unlink $file or warn "Cannot remove $file: $OS_ERROR";
+
+eval {
+   $output = output(
+      sub {
+         pt_agent::schedule_services(
+            services => [ $svc0 ],
+            lib_dir  => $tmpdir,
+         )
+      },
+      stderr => 1,
+   );
+};
+
+is(
+   $EVAL_ERROR,
+   "",
+   "No error"
+) or diag($output);
+
+$crontab = `crontab -l 2>/dev/null`;
+is(
+   $crontab,
+   "* 0  *  *  *  date > /dev/null
+* 8 * * 1,2,3,4,5 pt-agent --run-service query-monitor
+",
+   "schedule_services()"
+);
+
+ok(
+   -f "$tmpdir/crontab",
+   "Wrote crontab to --lib/crontab"
+) or diag(`ls -l $tmpdir`);
+
+ok(
+   -f "$tmpdir/crontab.err",
+   "Write --lib/crontab.err",
+) or diag(`ls -l $tmpdir`);
+
+my $err = -f "$tmpdir/crontab.err" ? `cat $tmpdir/crontab.err` : '';
+is(
+   $err,
+   "",
+   "No crontab error"
+);
+
+system("crontab -r 2>/dev/null");
+$crontab = `crontab -l 2>/dev/null`;
+is(
+   $crontab,
+   "",
+   "Removed crontab"
+);
+
+# #############################################################################
+# Handle bad crontab lines.
+# #############################################################################
+
+$svc0 = Percona::WebAPI::Resource::Service->new(
+   name     => 'query-monitor',
+   alias    => 'Query Monitor',
+   schedule => '* * * * Foo',  # "foo":0: bad day-of-week
+   runs     => [ $run0 ],
+);
+
+eval {
+   $output = output(
+      sub {
+         pt_agent::schedule_services(
+            services => [ $svc0 ],
+            lib_dir  => $tmpdir,
+         ),
+      },
+      stderr => 1,
+      die    => 1,
+   );
+};
+
+like(
+   $EVAL_ERROR,
+   qr/Error setting new crontab/,
+   "Throws errors"
+) or diag($output);
+
+$crontab = `crontab -l 2>/dev/null`;
+is(
+   $crontab,
+   "",
+   "Bad schedule_services()"
+);
+
+ok(
+   -f "$tmpdir/crontab",
+   "Wrote crontab to --lib/crontab"
+) or diag(`ls -l $tmpdir`);
+
+ok(
+   -f "$tmpdir/crontab.err",
+   "Write --lib/crontab.err",
+) or diag(`ls -l $tmpdir`);
+
+$err = -f "$tmpdir/crontab.err" ? `cat $tmpdir/crontab.err` : '';
+like(
+   $err,
+   qr/bad/,
+   "Crontab error"
+);
+
+system("crontab -r 2>/dev/null");
+$crontab = `crontab -l 2>/dev/null`;
+is(
+   $crontab,
+   "",
+   "Removed crontab"
+);
+
+
+# #############################################################################
+# Done.
+# #############################################################################
+done_testing;