refactoring to improve clarity

Changes

@@ -21,7 +21,7 @@
 
   [NEW FEATURES]
 
-  * Add Port Control (admin table) features
+  * Add Port Control (admin table) features to web
   * Implement DBIx::Class::Schema::Versioned support - DB schema is now versioned
   * Navbar query for device with single hit redirects to Device details page
   * Show device name on device details views (closes #30)

Netdisco/bin/netdisco-daemon

@@ -1,13 +1,13 @@
 #!/usr/bin/env perl
 
-use Dancer ':syntax :script';
+use Dancer ':script';
 use Dancer::Plugin::DBIC 'schema';
 
 # add dispatch methods for each port control action
 use base 'Netdisco::PortControl';
 
 use Daemon::Generic::While1;
-use Netdisco::Util 'load_nd_config';
+use Netdisco::Util qw/load_nd_config is_discoverable/;
 use Try::Tiny;
 
 newdaemon(
@@ -32,24 +32,8 @@ sub gd_preconfig {
   return (); # important
 }
 
-sub dispatch {
-  my ($self, $job) = @_;
-
-  # do update
-  my %dispatch = (
-    location => 'set_location',
-  );
-
-  my $target = $dispatch{$job->action}
-    or return ();
-
-  # return results
-  return $self->$target($job);
-}
-
 sub gd_run_body {
   my $self = shift;
-  my $nd_config = var('nd_config');
 
   # get all pending jobs
   my $rs = schema('netdisco')->resultset('Admin')->search({
@@ -57,27 +41,56 @@ sub gd_run_body {
     status => 'queued',
   });
 
-  JOB: while (my $job = $rs->next) {
+  while (my $job = $rs->next) {
+      my $target = 'set_'. $job->action;
+      next unless $self->can($target);
+
       # filter for discover_*
-      my $device = NetAddr::IP::Lite->new($job->device) or next JOB;
+      next unless is_discoverable($job->device);
 
-      if (length $nd_config->{_}->{discover_no}) {
+      # mark job as running
-          my @d_no = split /,\s*/, $nd_config->{_}->{discover_no};
+      next unless $self->lock_job($job);
-          foreach my $item (@d_no) {
+
-              my $ip = NetAddr::IP::Lite->new($item) or next JOB;
+      # do job
-              next JOB if $ip->contains($device);
+      my ($status, $log) = $self->$target($job);
+
+      # revert to queued status if we failed to action the job
+      if (not $status) {
+          $self->revert_job($job->job);
+      }
+      else {
+          # update job state to done/error with log
+          $self->close_job($job->job, $status, $log);
       }
   }
 
-      if (length $nd_config->{_}->{discover_only}) {
+  $self->gd_sleep( var('nd_config')->{loc}->{sleep_time} );
-          my $okay = 0;
+}
-          my @d_only = split /,\s*/, $nd_config->{_}->{discover_only};
+
-          foreach my $item (@d_only) {
+sub revert_job {
-              my $ip = NetAddr::IP::Lite->new($item) or next JOB;
+  my ($self, $id) = @_;
-              ++$okay if $ip->contains($device);
+
+  try {
+      schema('netdisco')->resultset('Admin')
+        ->find($id)
+        ->update({status => 'queued', started => undef});
   }
-          next JOB if not $okay;
+  catch {  warn "error reverting job: $_\n" };
+}
+
+sub close_job {
+  my ($self, $id, $status, $log) = @_;
+
+  try {
+      schema('netdisco')->resultset('Admin')
+        ->find($id)
+        ->update({status => $status, log => $log, finished => \'now()'});
   }
+  catch {  warn "error closing job: $_\n" };
+}
+
+sub lock_job {
+  my ($self, $job) = @_;
 
   # lock db table, check job state is still queued, update to running
   try {
@@ -92,36 +105,13 @@ sub gd_run_body {
           return 1;
       });
 
-          next JOB if not $status_updated;
+      return 0 if not $status_updated;
   }
   catch {
-          warn "error updating job status: $_\n";
+      warn "error locking job: $_\n";
-          next JOB;
+      return 0;
   };
 
-      # do job
+  return 1;
-      my ($status, $log) = $self->dispatch($job);
-
-      # revert to queued status if we failed to connect to device
-      if (not $status) {
-          try {
-              schema('netdisco')->resultset('Admin')
-                ->find($job->job)
-                ->update({status => 'queued', started => undef});
-          }
-          catch {  warn "error updating job: $_\n" };
-      }
-      else {
-          # update job state to done/error with log
-          try {
-              schema('netdisco')->resultset('Admin')
-                ->find($job->job)
-                ->update({status => $status, log => $log, finished => \'now()'});
-          }
-          catch {  warn "error updating job: $_\n" };
-      }
-  }
-
-  $self->gd_sleep( $nd_config->{loc}->{sleep_time} );
 }
 

Netdisco/lib/Netdisco/Util.pm

@@ -3,6 +3,7 @@ package Netdisco::Util;
 use Dancer qw/:syntax :script/;
 use Dancer::Plugin::DBIC 'schema';
 
+use NetAddr::IP::Lite;
 use SNMP::Info;
 use Config::Tiny;
 use File::Slurp;
@@ -10,9 +11,43 @@ use Try::Tiny;
 
 use base 'Exporter';
 our @EXPORT = ();
-our @EXPORT_OK = qw/load_nd_config get_device snmp_connect sort_port/;
+our @EXPORT_OK = qw/
+  is_discoverable
+  load_nd_config
+  get_device
+  snmp_connect
+  sort_port
+/;
 our %EXPORT_TAGS = (port_control => [qw/get_device snmp_connect/]);
 
+sub is_discoverable {
+  my $ip = shift;
+
+  my $device = NetAddr::IP::Lite->new($ip) or return 0;
+  my $discover_no   = var('nd_config')->{_}->{discover_no};
+  my $discover_only = var('nd_config')->{_}->{discover_only};
+
+  if (length $discover_no) {
+      my @d_no = split /,\s*/, $discover_no;
+      foreach my $item (@d_no) {
+          my $ip = NetAddr::IP::Lite->new($item) or return 0;
+          return 0 if $ip->contains($device);
+      }
+  }
+
+  if (length $discover_only) {
+      my $okay = 0;
+      my @d_only = split /,\s*/, $discover_only;
+      foreach my $item (@d_only) {
+          my $ip = NetAddr::IP::Lite->new($item) or return 0;
+          ++$okay if $ip->contains($device);
+      }
+      return 0 if not $okay;
+  }
+
+  return 1;
+}
+
 sub load_nd_config {
   my $file = shift or die "missing netdisco config file name.\n";
   my $config = {};
@@ -45,11 +80,8 @@ sub get_device {
 }
 
 sub build_mibdirs {
-  my $nd_config = var('nd_config')
+  my $mibhome  = var('nd_config')->{_}->{mibhome};
-    or die "Cannot call build_mibdirs without Dancer and nd_config.\n";
+  (my $mibdirs = var('nd_config')->{_}->{mibdirs}) =~ s/\s+//g;
-
-  my $mibhome  = $nd_config->{_}->{mibhome};
-  (my $mibdirs = $nd_config->{_}->{mibdirs}) =~ s/\s+//g;
 
   $mibdirs =~ s/\$mibhome/$mibhome/g;
   return [ split /,/, $mibdirs ];
@@ -57,8 +89,7 @@ sub build_mibdirs {
 
 sub snmp_connect {
   my $ip = shift;
-  my $nd_config = var('nd_config')
+  my $nd_config = var('nd_config')->{_};
-    or die "Cannot call snmp_connect without Dancer and nd_config.\n";
 
   # get device details from db
   my $device = get_device($ip)
@@ -67,15 +98,15 @@ sub snmp_connect {
   # TODO: really only supporing v2c at the moment
   my %snmp_args = (
     DestHost => $device->ip,
-    Version => ($device->snmp_ver || $nd_config->{_}->{snmpver} || 2),
+    Version => ($device->snmp_ver || $nd_config->{snmpver} || 2),
-    Retries => ($nd_config->{_}->{snmpretries} || 2),
+    Retries => ($nd_config->{snmpretries} || 2),
-    Timeout => ($nd_config->{_}->{snmptimeout} || 1000000),
+    Timeout => ($nd_config->{snmptimeout} || 1000000),
     MibDirs => build_mibdirs(),
     AutoSpecify => 1,
     Debug => ($ENV{INFO_TRACE} || 0),
   );
 
-  (my $comm = $nd_config->{_}->{community_rw}) =~ s/\s+//g;
+  (my $comm = $nd_config->{community_rw}) =~ s/\s+//g;
   my @communities = split /,/, $comm;
 
   my $info = undef;