Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve ncd status logging #13

Merged
merged 7 commits into from Sep 4, 2014
Merged

Improve ncd status logging #13

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
32f6945
ncm-cdispd: log ncd status as a warning if some components failed
jouvin Sep 3, 2014
df625a3
ncm-cdispd: restore original formatting of ncd status message
jouvin Sep 3, 2014
d8790c2
ncm-cdispd launch_ncd(): use CAF::Process is_executable()
jouvin Sep 3, 2014
1bbdf9d
ncm-cdispd launch_ncd(): cleanup output
jouvin Sep 3, 2014
94421f2
ncm-cdispd: log a summary line per failed component
jouvin Sep 3, 2014
a3f6a8c
ncm-cdispd: define ncm-ncd executable path as a constant
jouvin Sep 3, 2014
0fe244c
Set an explicit message if component state file is empty
jouvin Sep 4, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 17 additions & 19 deletions src/main/scripts/ncm-cdispd
View file
Open in desktop
Expand Up @@ -563,7 +563,7 @@ sub launch_ncd {

my $result = 0; # Assume success

my @cmd = ( '/usr/sbin/ncm-ncd', '--configure' );
my $p = CAF::Process->new(['/usr/sbin/ncm-ncd', '--configure'] , log => $this_app );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mayeb make /usr/sbin/ncm-ncd a conatsnat or readonly and use it here and below?

if ( defined( $this_app->{ICLIST} ) && scalar(@{$this_app->{ICLIST}}) ) {
# At this point, ICLIST should contain only components present in the last profile received.
# The only case where a component may be in the list without being part of the configuration
Expand All @@ -574,62 +574,60 @@ sub launch_ncd {
# X failed with profile N+1) and thus X removal is not detected.
# As a result, X remains on the list of component to run. This should be harmless as ncm-ncd will ignore it.
# This is probably rare enough to avoid complex processing to handle this in ncm-dispd.
push @cmd, @{ $this_app->{ICLIST} };
$p->pushargs(@{$this_app->{ICLIST}});
} else {
$this_app->info("no components to be run by NCM - ncm-ncd won't be called");
return (0);
}

# ncd options
if ( $this_app->option('state') ) {
push @cmd, "--state", $this_app->option('state');
$p->pushargs("--state", $this_app->option('state'));
}
if ( $this_app->option('ncd-retries') ) {
push @cmd, "--retries", $this_app->option('ncd-retries');
$p->pushargs("--retries", $this_app->option('ncd-retries'));
}
if ( $this_app->option('ncd-timeout') ) {
push @cmd, "--timeout", $this_app->option('ncd-timeout');
$p->pushargs("--timeout", $this_app->option('ncd-timeout'));
}
if ( $this_app->option('ncd-useprofile') ) {
push @cmd, "--useprofile", $this_app->option('ncd-useprofile');
$p->pushargs("--useprofile", $this_app->option('ncd-useprofile'));
}

my $cmd_line = join( " ", @cmd );
if ( $this_app->option('noaction') ) {
$this_app->info( "would run (noaction mode): " . $cmd_line );
$this_app->info( "would run (noaction mode): $p");
} else {
$this_app->info( "about to run: " . $cmd_line );
my $verb = $cmd[0];
if ( -x $verb ) {
$this_app->info( "about to run: $p");
if ( $p->is_executable() ) {
# Delay processing of some signals
delay_signals();

# Execute ncm-ncd and report exit status
my $p = CAF::Process->new( \@cmd, log => $this_app );
sub act {
my ($logger, $message) = @_;
$logger->debug(1, $message);
$logger->verbose($message);
}
my $errormsg = $p->stream_output(\&act, mode => 'line', arguments => [$this_app]);

my $ec = $?;
$this_app->debug(3, "ncm-ncd finished with full message $errormsg");
my $msg = "ncm-ncd finished with status: exitcode ". ($ec >> 8) . " (ec $ec)";
my $msg = "ncm-ncd finished with status: ". ($ec >> 8) . " (ec $ec";
my $log_level = 'info';
if ( $ec ) {
$msg .= " (some configuration modules failed to run successfully)";
$log_level = 'warn';
$msg .= ", some configuration modules failed to run successfully)";
$result = 1;
} else {
$msg .= " (all configuration modules ran successfully)";
$msg .= ", all configuration modules ran successfully)";
}
$this_app->info($msg);
$this_app->$log_level($msg);

# Process delayed signal if any and reestablish
# immediate processing of signals
process_signal();
immediate_signals();

} else {
$this_app->error("Command $verb not found");
$this_app->error("Command ". ${$p->get_command()}[0] . " not found or not executable");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, if this is going to be frequently used, i propose to add to CAF (one or more) of:

  • $p->get_executable()
  • pass optional loglevel to is_executable to log the failure
  • $p->execute_if_executable() (better to find shorter name like eie 😄 ) which is basically
sub execute_if_exists 
{
    my $(self) = @_;
    if ($self->is_executable()) {
        return $self->execute();
    } else {
        $self->{log}->error("Command ".$self->get_executable()." not found or not executable");
        return 1;
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for get_executable()... In fact in another issue I advocated for this!

As for the execute_if_exists() proposal, I'm wondering why it would not be the standard behaviour for execute(). Is there really a use case to executing a non executable command?

Probably better to open an issue in CAF for this discussion...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use the constant NCD_EXECUTABLE here?

$result = 1;
}
}
Expand Down