actions in device_skip table are now an array/set

lib/App/Netdisco/DB/Result/Admin.pm

@@ -56,6 +56,28 @@ __PACKAGE__->set_primary_key("job");
 
 # You can replace this text with custom code or comments, and it will be preserved on regeneration
 
+=head1 RELATIONSHIPS
+
+=head2 skipped
+
+Retuns the set of C<device_skip> entries which may apply to this job. They
+match the device IP and job action, and may refer to one or more backends.
+
+=cut
+
+__PACKAGE__->has_many( skipped => 'App::Netdisco::DB::Result::DeviceSkip',
+  sub {
+    my $args = shift;
+    return {
+      "$args->{foreign_alias}.device"
+        => { -ident => "$args->{self_alias}.device" },
+      "$args->{foreign_alias}.actionset"
+        => { '@>' => \"string_to_array($args->{self_alias}.action,'')" },
+    };
+  },
+  { cascade_copy => 0, cascade_update => 0, cascade_delete => 0 }
+);
+
 =head1 METHODS
 
 =head2 summary
@@ -114,17 +136,4 @@ between the date stamp and time stamp. That is:
 
 sub finished_stamp  { return (shift)->get_column('finished_stamp')  }
 
-=head1 RELATIONSHIPS
-
-=head2 skipped
-
-Retuns the set of C<device_skip> entries which may apply to this job. They
-match the device IP and job action, and may refer to one or more backends.
-
-=cut
-
-__PACKAGE__->has_many( skipped => 'App::Netdisco::DB::Result::DeviceSkip',
-   { 'foreign.device' => 'self.device', 'foreign.action' => 'self.action' },
-);
-
 1;

lib/App/Netdisco/DB/Result/DeviceSkip.pm

@@ -4,6 +4,8 @@ package App::Netdisco::DB::Result::DeviceSkip;
 use strict;
 use warnings;
 
+use List::MoreUtils ();
+
 use base 'DBIx::Class::Core';
 __PACKAGE__->table("device_skip");
 __PACKAGE__->add_columns(
@@ -11,18 +13,18 @@ __PACKAGE__->add_columns(
   { data_type => "text", is_nullable => 0 },
   "device",
   { data_type => "inet", is_nullable => 0 },
-  "action",
+  "actionset",
-  { data_type => "text", is_nullable => 0 },
+  { data_type => "text[]", is_nullable => 0 },
   "deferrals",
   { data_type => "integer", is_nullable => 1, default_value => '0' },
   "skipover",
   { data_type => "boolean", is_nullable => 1, default_value => \'false' },
 );
 
-__PACKAGE__->set_primary_key("backend", "device", "action");
+__PACKAGE__->set_primary_key("backend", "device");
 
 __PACKAGE__->add_unique_constraint(
-  device_skip_pkey => [qw/backend device action/]);
+  device_skip_pkey => [qw/backend device/]);
 
 =head1 METHODS
 
@@ -39,4 +41,17 @@ sub increment_deferrals {
   return $row->update({ deferrals => ($row->deferrals + 1) });
 }
 
+=head2 add_to_actionset
+
+=cut
+
+sub add_to_actionset {
+  my ($row, @badactions) = @_;
+  return unless $row->in_storage;
+  return unless scalar @badactions;
+  return $row->update({ actionset =>
+    [ sort (List::MoreUtils::uniq( @{ $row->actionset }, @badactions )) ]
+  });
+}
+
 1;

lib/App/Netdisco/JobQueue/PostgreSQL.pm

@@ -20,11 +20,11 @@ our @EXPORT_OK = qw/
   jq_locked
   jq_queued
   jq_prime_skiplist
-  jq_log
-  jq_userlog
   jq_lock
   jq_defer
   jq_complete
+  jq_log
+  jq_userlog
   jq_insert
   jq_delete
 /;
@@ -94,68 +94,68 @@ sub jq_queued {
   })->get_column('device')->all;
 }
 
+# given a device, tests if any of the primary acls applies
+# returns a list of job actions to be denied/skipped on this host.
+sub _get_denied_actions {
+  my $device = shift;
+  my @badactions = ();
+
+  push @badactions, ('discover', @{ setting('job_prio')->{high} })
+    if not is_discoverable($device);
+
+  push @badactions, (qw/macsuck nbtstat/)
+    if not is_macsuckable($device);
+
+  push @badactions, 'arpnip'
+    if not is_arpnipable($device);
+
+  return @badactions;
+}
+
 sub jq_prime_skiplist {
   my $fqdn = hostfqdn || 'localhost';
+  my @devices = schema('netdisco')->resultset('Device')->all;
   my $rs = schema('netdisco')->resultset('DeviceSkip');
-  my @d_actions = ('discover', @{ setting('job_prio')->{high} });
+  my %actionset = ();
+
+  foreach my $d (@devices) {
+    my @badactions = _get_denied_actions($d);
+    $actionset{$d->ip} = \@badactions if scalar @badactions;
+  }
 
   schema('netdisco')->txn_do(sub {
-    my @devices = schema('netdisco')->resultset('Device')->all;
     $rs->search({ backend => $fqdn })->delete;
-
-    foreach my $action (@d_actions) {
-      $rs->populate([
-        map {{
-          backend => $fqdn,
-          device  => $_->ip,
-          action  => $action,
-          skipover => \'true',
-        }} grep { not is_discoverable($_) } @devices
-      ]);
-    }
-
-    foreach my $action (qw/macsuck nbtstat/) {
-      $rs->populate([
-        map {{
-          backend => $fqdn,
-          device  => $_->ip,
-          action  => $action,
-          skipover => \'true',
-        }} grep { not is_macsuckable($_) } @devices
-      ]);
-    }
-
     $rs->populate([
       map {{
         backend => $fqdn,
-        device  => $_->ip,
+        device  => $_,
-        action  => 'arpnip',
+        actionset => $actionset{$_},
         skipover => \'true',
-      }} grep { not is_arpnipable($_) } @devices
+      }} keys %actionset
     ]);
   });
 }
 
-sub jq_log {
-  return schema('netdisco')->resultset('Admin')->search({}, {
-    order_by => { -desc => [qw/entered device action/] },
-    rows => 50,
-  })->with_times->hri->all;
-}
-
-sub jq_userlog {
-  my $user = shift;
-  return schema('netdisco')->resultset('Admin')->search({
-    username => $user,
-    finished => { '>' => \"(now() - interval '5 seconds')" },
-  })->with_times->all;
-}
-
 sub jq_lock {
   my $job = shift;
   my $fqdn = hostfqdn || 'localhost';
   my $happy = false;
 
+  # need to handle device discovered since backend daemon started
+  # and the skiplist was primed. these should be checked against
+  # the various acls and have device_skip entry added if needed,
+  # and return false if it should have been skipped.
+  # this is also why it's probably unnecessary to check is_* within
+  # jobs, but we do that as well, just to be sure.
+  my @badactions = _get_denied_actions($job->device);
+  if (scalar @badactions) {
+    schema('netdisco')->resultset('DeviceSkip')->find_or_create({
+      backend => $fqdn, device => $job->device,
+    },{ key => 'device_skip_pkey' })->add_to_actionset(@badactions);
+
+    return false;
+  }
+
   # lock db row and update to show job has been picked
   try {
     schema('netdisco')->txn_do(sub {
@@ -192,10 +192,17 @@ sub jq_defer {
   my $fqdn = hostfqdn || 'localhost';
   my $happy = false;
 
+  # note this taints all actions on the device. for example if both
+  # macsuck and arpnip are allowed, but macsuck fails 10 times, then
+  # arpnip (and every other action) will be prevented on the device.
+
+  # seeing as defer is only triggered by an SNMP connect failure, this
+  # behaviour seems reasonable, to me (or desirable, perhaps).
+
   try {
     schema('netdisco')->txn_do(sub {
       schema('netdisco')->resultset('DeviceSkip')->find_or_create({
-        backend => $fqdn, device => $job->device, action => $job->action,
+        backend => $fqdn, device => $job->device,
       },{ key => 'device_skip_pkey' })->increment_deferrals;
 
       # lock db row and update to show job is available
@@ -236,6 +243,21 @@ sub jq_complete {
   return $happy;
 }
 
+sub jq_log {
+  return schema('netdisco')->resultset('Admin')->search({}, {
+    order_by => { -desc => [qw/entered device action/] },
+    rows => 50,
+  })->with_times->hri->all;
+}
+
+sub jq_userlog {
+  my $user = shift;
+  return schema('netdisco')->resultset('Admin')->search({
+    username => $user,
+    finished => { '>' => \"(now() - interval '5 seconds')" },
+  })->with_times->all;
+}
+
 sub jq_insert {
   my $jobs = shift;
   $jobs = [$jobs] if ref [] ne ref $jobs;

share/schema_versions/App-Netdisco-DB-41-42-PostgreSQL.sql

@@ -3,10 +3,10 @@ BEGIN;
 CREATE TABLE "device_skip" (
   "backend" text NOT NULL,
   "device" inet NOT NULL,
-  "action" text NOT NULL,
+  "actionset" text[] DEFAULT '{}',
   "deferrals" integer DEFAULT 0,
-  "skipover"  boolean DEFAULT false,
+  "skipover" boolean DEFAULT false,
-  PRIMARY KEY ("backend", "device", "action")
+  PRIMARY KEY ("backend", "device")
 );
 
 COMMIT;