Refactor Azure diagnostic log retrieval and improve error handling

Centralize the logic for fetching Azure VM boot diagnostics in azure_cli.pm
by enhancing az_vm_diagnostic_log_get with timeout support and better
error reporting. Refactor qesap_az_diagnostic_log to use this centralized
function, reducing code duplication. Update unit tests in 15_qesap_azure.t
and 21_sles4sap_azure_cli.t to reflect these changes and cover new
failure scenarios.

Author: mpagot

lib/sles4sap/azure_cli.pm

@@ -1427,19 +1427,39 @@ Return a list of diagnostic file paths on the JumpHost
 
 =item B<resource_group> - existing resource group where to search for a specific VM
 
+=item B<timeout> - max expected execution time for a single az command - default 240sec
+
+=item B<verbose> - use tee to print the output also on the terminal while redirecting to file. Default 0
+
 =back
 =cut
 
 sub az_vm_diagnostic_log_get(%args) {
     croak("Argument < resource_group > missing") unless $args{resource_group};
 
+    my $timeout = $args{timeout} // 240;
     my @diagnostic_log_files;
     my $vm_data = az_vm_list(resource_group => $args{resource_group}, query => '[].{id:id,name:name}');
     my $az_get_logs_cmd = 'az vm boot-diagnostics get-boot-log --ids';
+    my $ret;
+    my $err;
+    my $redirect = $args{verbose} ? '|& tee' : '&>';
+    my $boot_diagnostics_log;
     foreach (@{$vm_data}) {
-        my $boot_diagnostics_log = '/tmp/boot-diagnostics_' . $_->{name} . '.txt';
-        push @diagnostic_log_files, $boot_diagnostics_log unless
-          script_run(join(' ', $az_get_logs_cmd, $_->{id}, '|&', 'tee', $boot_diagnostics_log));
+        $boot_diagnostics_log = '/tmp/boot-diagnostics_' . $_->{name} . '.txt';
+        eval {
+            $ret = script_run(
+                join(' ', $az_get_logs_cmd, $_->{id}, $redirect, $boot_diagnostics_log),
+                timeout => $timeout);
+        };
+        $err = $@;
+        if ($err || !defined $ret || $ret != 0) {
+            my $detail = ($err && $err =~ /\S/) ? "$err" : "Exit code: $ret";
+            my $fail_msg = "Failed to get boot diagnostics for $_->{name}: $detail";
+            record_info('Diag Warn', $fail_msg, result => 'fail');
+            next;
+        }
+        push(@diagnostic_log_files, $boot_diagnostics_log);
     }
     return @diagnostic_log_files;
 }

lib/sles4sap/ipaddr2.pm

@@ -1712,7 +1712,8 @@ Collect logs from the cloud infrastructure
 
 sub ipaddr2_deployment_logs {
     my @diagnostic_log_files = az_vm_diagnostic_log_get(
-        resource_group => ipaddr2_azure_resource_group());
+        resource_group => ipaddr2_azure_resource_group(),
+        verbose => 1);    #TODO remove it
     while (my $file = pop @diagnostic_log_files) {
         upload_logs($file, failok => 1);
     }

lib/sles4sap/qesap/azure.pm

@@ -33,10 +33,10 @@ use Carp qw(croak);
 use Mojo::JSON qw(decode_json);
 use Exporter 'import';
 use File::Basename;
+use testapi;
+use mmapi 'get_current_job_id';
 use sles4sap::azure_cli;
 use sles4sap::qesap::utils;
-use mmapi 'get_current_job_id';
-use testapi;
 
 our @EXPORT = qw(
   qesap_az_get_resource_group
@@ -325,33 +325,12 @@ Return a list of diagnostic file paths on the JumpHost
 sub qesap_az_diagnostic_log {
     my (%args) = @_;
     my $fatal = $args{fatal} // 1;
-    my $timeout = $args{timeout} // 240;
-    my @diagnostic_log_files;
     my @failures;
 
-    my $rg = qesap_az_get_resource_group();
-    my $vm_data = az_vm_list(resource_group => $rg, query => '[].{id:id,name:name}');
-    my $az_get_logs_cmd = 'az vm boot-diagnostics get-boot-log --ids';
-
-    foreach (@{$vm_data}) {
-        record_info('az vm boot-diagnostics json', "id: $_->{id} name: $_->{name}");
-        my $boot_diagnostics_log = '/tmp/boot-diagnostics_' . $_->{name} . '.txt';
-
-        my $ret;
-        eval {
-            $ret = script_run("$az_get_logs_cmd $_->{id} &> $boot_diagnostics_log", timeout => $timeout);
-        };
-        my $error = $@;
-
-        if ($error || !defined $ret || $ret != 0) {
-            my $detail = ($error && $error =~ /\S/) ? "$error" : "Exit code: $ret";
-            my $fail_msg = "Failed to get boot diagnostics for $_->{name}: $detail";
-            record_info('Diag Warn', $fail_msg, result => 'fail');
-            push @failures, $fail_msg;
-            next;
-        }
-        push(@diagnostic_log_files, $boot_diagnostics_log);
-    }
+    my @diagnostic_log_files = az_vm_diagnostic_log_get(
+        resource_group => qesap_az_get_resource_group(),
+        timeout => $args{timeout} // 240,
+        verbose => 1);    #TODO remove it
 
     if (@failures && $fatal) {
         die "Fatal Error:\n" . join("\n", @failures);

t/15_qesap_azure.t

@@ -325,46 +325,72 @@ subtest '[qesap_az_diagnostic_log] no VMs' => sub {
     my $qesap = Test::MockModule->new('sles4sap::qesap::azure', no_auto => 1);
     my @calls;
     $qesap->redefine(qesap_az_get_resource_group => sub { return 'DENTIST'; });
+    $qesap->redefine(az_vm_diagnostic_log_get => sub { push @calls, {@_}; return (); });
+    $qesap->redefine(record_info => sub { note(join(' ', 'RECORD_INFO -->', @_)); });
+
+    my @log_files = qesap_az_diagnostic_log();
+
+    ok((any { $_->{resource_group} eq 'DENTIST' } @calls), 'Proper resource group in vm list');
+    #ok((any { $_->{query} =~ /id:id,name:name/ } @calls), 'Proper query in vm list');
+    ok((scalar @log_files == 0), 'No returned logs');
+};
+
+subtest '[qesap_az_diagnostic_log] no VMs integration test' => sub {
+    my $qesap = Test::MockModule->new('sles4sap::qesap::azure', no_auto => 1);
+    my $azcli = Test::MockModule->new('sles4sap::azure_cli', no_auto => 1);
+    my @calls;
+    $qesap->redefine(qesap_az_get_resource_group => sub { return 'DENTIST'; });
     $qesap->redefine(record_info => sub { note(join(' ', 'RECORD_INFO -->', @_)); });
 
     # Configure vm list to return no VMs
-    $qesap->redefine(az_vm_list => sub { push @calls, {@_}; return []; });
+    $azcli->redefine(script_output => sub { push @calls, $_[0]; return '[]'; });
 
     my @log_files = qesap_az_diagnostic_log();
 
     note("\n  C-->  " . join("\n  C-->  ", @calls));
-    ok((any { $_->{resource_group} eq 'DENTIST' } @calls), 'Proper resource group in vm list');
-    ok((any { $_->{query} =~ /id:id,name:name/ } @calls), 'Proper query in vm list');
+    ok((any { /az vm list.*DENTIST.*/ } @calls), 'List all VMs in the resource group');
     ok((scalar @log_files == 0), 'No returned logs');
 };
 
 subtest '[qesap_az_diagnostic_log] one VMs' => sub {
     my $qesap = Test::MockModule->new('sles4sap::qesap::azure', no_auto => 1);
     my @calls;
     $qesap->redefine(qesap_az_get_resource_group => sub { return 'DENTIST'; });
+    $qesap->redefine(az_vm_diagnostic_log_get => sub { push @calls, {@_}; return ('pink_coral.log'); });
     $qesap->redefine(record_info => sub { note(join(' ', 'RECORD_INFO -->', @_)); });
 
+    my @log_files = qesap_az_diagnostic_log();
+
+    ok((scalar @log_files == 1), 'Exactly one returned logs for one VM');
+};
+
+subtest '[qesap_az_diagnostic_log] one VMs integration test' => sub {
+    my $qesap = Test::MockModule->new('sles4sap::qesap::azure', no_auto => 1);
+    my $azcli = Test::MockModule->new('sles4sap::azure_cli', no_auto => 1);
+    my @calls;
+    $qesap->redefine(qesap_az_get_resource_group => sub { return 'DENTIST'; });
+
+    $azcli->redefine(script_run => sub { push @calls, $_[0]; return 0; });
     # Configure vm list to return one VM
-    $qesap->redefine(az_vm_list => sub {
-            push @calls, {@_};
-            return [{name => "NEMO", id => "MARLIN"}];
+    $azcli->redefine(script_output => sub {
+            push @calls, $_[0];
+            return '[{"name":"NEMO", "id":"MARLIN"}]';
     });
-    $qesap->redefine(script_run => sub { push @calls, $_[0]; return 0; });
+    $azcli->redefine(record_info => sub { note(join(' ', 'RECORD_INFO -->', @_)); });
 
     my @log_files = qesap_az_diagnostic_log();
 
     note("\n  C-->  " . join("\n  C-->  ", @calls));
-    ok((any { $_->{resource_group} eq 'DENTIST' } @calls), 'Proper resource group in vm list');
-    ok((any { /az vm boot-diagnostics get-boot-log.*/ } @calls), 'Proper base command for vm boot-diagnostics get-boot-log');
-    ok((any { /.*--ids MARLIN.*/ } @calls), 'Proper id in boot-diagnostics');
-    ok((any { /.*boot-diagnostics_NEMO.*/ } @calls), 'Proper output file in boot-diagnostics');
     ok((scalar @log_files == 1), 'Exactly one returned logs for one VM');
 };
 
 subtest '[qesap_az_diagnostic_log] three VMs' => sub {
     my $qesap = Test::MockModule->new('sles4sap::qesap::azure', no_auto => 1);
     my @calls;
     $qesap->redefine(qesap_az_get_resource_group => sub { return 'DENTIST'; });
+    $qesap->redefine(az_vm_diagnostic_log_get => sub {
+            push @calls, {@_};
+            return ('pink_coral.log', 'blue_coral.log', 'white_coral.log'); });
     $qesap->redefine(record_info => sub { note(join(' ', 'RECORD_INFO -->', @_)); });
 
     # Configure vm list to return three VMs
@@ -378,10 +404,27 @@ subtest '[qesap_az_diagnostic_log] three VMs' => sub {
     });
     $qesap->redefine(script_run => sub { push @calls, $_[0]; return 0; });
 
+    my @log_files = qesap_az_diagnostic_log();
+    ok((scalar @log_files == 3), 'Exactly three returned logs for three VMs');
+};
+
+subtest '[qesap_az_diagnostic_log] three VMs integration test' => sub {
+    my $qesap = Test::MockModule->new('sles4sap::qesap::azure', no_auto => 1);
+    my $azcli = Test::MockModule->new('sles4sap::azure_cli', no_auto => 1);
+    my @calls;
+    $qesap->redefine(qesap_az_get_resource_group => sub { return 'DENTIST'; });
+
+    $azcli->redefine(script_run => sub { push @calls, $_[0]; return 0; });
+    # Configure vm list to return 3 VMs
+    $azcli->redefine(script_output => sub {
+            push @calls, $_[0];
+            return '[{"name":"DORY", "id":"BLUE_TANG"},{"name":"BRUCE", "id":"GREAT_WHITE"},{"name":"CRUSH", "id":"SEA_TURTLE"}]';
+    });
+    $azcli->redefine(record_info => sub { note(join(' ', 'RECORD_INFO -->', @_)); });
+
     my @log_files = qesap_az_diagnostic_log();
 
     note("\n  C-->  " . join("\n  C-->  ", @calls));
-    ok((any { ref($_) eq 'HASH' && $_->{resource_group} eq 'DENTIST' } @calls), 'Proper resource group in vm list');
 
     my %expected_vms = (
         DORY => "BLUE_TANG",
@@ -396,76 +439,4 @@ subtest '[qesap_az_diagnostic_log] three VMs' => sub {
     ok((scalar @log_files == 3), 'Exactly three returned logs for three VMs');
 };
 
-subtest '[qesap_az_diagnostic_log] Partial Success (fatal = 0)' => sub {
-    my $qesap = Test::MockModule->new('sles4sap::qesap::azure', no_auto => 1);
-    my @record_calls;
-
-    $qesap->redefine(qesap_az_get_resource_group => sub { return 'DENTIST'; });
-    $qesap->redefine(az_vm_list => sub {
-            return [
-                {name => "DORY", id => "BLUE_TANG"},
-                {name => "BRUCE", id => "GREAT_WHITE"},
-                {name => "CRUSH", id => "SEA_TURTLE"}
-            ];
-    });
-
-    $qesap->redefine(script_run => sub {
-            my ($cmd) = @_;
-            return ($cmd =~ /BRUCE/) ? 1 : 0;
-    });
-    $qesap->redefine(record_info => sub { push @record_calls, $_[0] if $_[0] eq 'Diag Warn'; });
-
-    my @log_files;
-    lives_ok {
-        @log_files = qesap_az_diagnostic_log(fatal => 0);
-    } 'Fail silently';
-
-    is(scalar @log_files, 2, 'Return exactly 2 successful logs');
-    ok((any { /boot-diagnostics_DORY.txt/ } @log_files), 'DORY log present');
-    ok((any { /boot-diagnostics_CRUSH.txt/ } @log_files), 'CRUSH log present');
-    ok(!(any { /boot-diagnostics_BRUCE.txt/ } @log_files), 'BRUCE log absent');
-    is(scalar @record_calls, 1, 'One failure for BRUCE');
-};
-
-subtest '[qesap_az_diagnostic_log] Cumulative Failures (fatal = 1)' => sub {
-    my $qesap = Test::MockModule->new('sles4sap::qesap::azure', no_auto => 1);
-    $qesap->redefine(qesap_az_get_resource_group => sub { return 'DENTIST'; });
-    $qesap->redefine(record_info => sub { 1; });
-
-    $qesap->redefine(az_vm_list => sub {
-            return [
-                {name => "NEMO", id => "MARLIN"},
-                {name => "DORY", id => "BLUE_TANG"}
-            ];
-    });
-
-    $qesap->redefine(script_run => sub { return 1; });
-    throws_ok {
-        qesap_az_diagnostic_log(fatal => 1);
-    } qr/Fatal Error:.*NEMO.*DORY/s, 'Die as expected';
-};
-
-subtest '[qesap_az_diagnostic_log] Mixed Error' => sub {
-    my $qesap = Test::MockModule->new('sles4sap::qesap::azure', no_auto => 1);
-    $qesap->redefine(qesap_az_get_resource_group => sub { return 'DENTIST'; });
-    $qesap->redefine(record_info => sub { 1; });
-
-    $qesap->redefine(az_vm_list => sub {
-            return [
-                {name => "NEMO", id => "MARLIN"},
-                {name => "DORY", id => "BLUE_TANG"}
-            ];
-    });
-
-    $qesap->redefine(script_run => sub {
-            my ($cmd) = @_;
-            die "Timeout issue" if $cmd =~ /DORY/;
-            return 1;    # NEMO
-    });
-
-    throws_ok {
-        qesap_az_diagnostic_log(fatal => 1);
-    } qr/Exit code: 1.*DORY: Timeout issue/s, 'Exitcode and timeout as expected';
-};
-
 done_testing;

t/21_sles4sap_azure_cli.t

@@ -860,14 +860,83 @@ subtest '[az_vm_diagnostic_log_get]' => sub {
             push @calls, $_[0];
             # simulate 2 VM
             return '[{"id": "0001", "name": "Truffaldino"}, {"id": "0002", "name": "Mirandolina"}]'; });
+    $azcli->redefine(script_run => sub { push @calls, $_[0]; return 0; });
+    $azcli->redefine(record_info => sub { note(join(' ', 'RECORD_INFO -->', @_)); });
+
+    my @ret = az_vm_diagnostic_log_get(resource_group => 'Arlecchino');
+
+    note("\n  C-->  " . join("\n  C-->  ", @calls));
+    note("\n  R-->  " . join("\n  R-->  ", @ret));
+    ok((any { /az vm list/ } @calls), 'Correct composition of the list command');
+    ok((any { /az vm boot-diagnostics get-boot-log/ } @calls), 'Correct composition of the main command');
+    ok((any { /--ids 0001.*Truffaldino\.txt/ } @calls), 'Correct composition of the --id for the first VM');
+    ok((any { /--ids 0002.*Mirandolina\.txt/ } @calls), 'Correct composition of the --id for the second VM');
+    ok((none { /.*tee.*/ } @calls), 'Correctly not use tee to redirect output to file');
+
+    ok scalar @ret == 2, "Two logs one for each VM";
+    ok((any { /\/tmp\/boot-diagnostics_.*/ } @ret), 'Correct log filenames');
+};
+
+subtest '[az_vm_diagnostic_log_get] verbose' => sub {
+    my $azcli = Test::MockModule->new('sles4sap::azure_cli', no_auto => 1);
+    my @calls;
+    $azcli->redefine(script_output => sub {
+            push @calls, $_[0];
+            # simulate 2 VM
+            return '[{"id": "0001", "name": "Truffaldino"}, {"id": "0002", "name": "Mirandolina"}]'; });
+    $azcli->redefine(script_run => sub { push @calls, $_[0]; return 0; });
+    $azcli->redefine(record_info => sub { note(join(' ', 'RECORD_INFO -->', @_)); });
+
+    my @ret = az_vm_diagnostic_log_get(resource_group => 'Arlecchino', verbose => 1);
+
+    note("\n  C-->  " . join("\n  C-->  ", @calls));
+    note("\n  R-->  " . join("\n  R-->  ", @ret));
+    ok((any { /.*tee.*/ } @calls), 'Correctly use tee to redirect output to file');
+};
+
+subtest '[az_vm_diagnostic_log_get] no VMs' => sub {
+    my $azcli = Test::MockModule->new('sles4sap::azure_cli', no_auto => 1);
+    my @calls;
+    $azcli->redefine(script_output => sub {
+            push @calls, $_[0];
+            # simulate 2 VM
+            return '[]'; });
     $azcli->redefine(script_run => sub { push @calls, $_[0]; });
+    $azcli->redefine(record_info => sub { note(join(' ', 'RECORD_INFO -->', @_)); });
 
-    az_vm_diagnostic_log_get(resource_group => 'Arlecchino');
+    my @ret = az_vm_diagnostic_log_get(resource_group => 'Arlecchino');
 
-    note("\n  -->  " . join("\n  -->  ", @calls));
+    note("\n  C-->  " . join("\n  C-->  ", @calls));
+    note("\n  R-->  " . join("\n  R-->  ", @ret));
+    ok((any { /az vm list/ } @calls), 'Correct composition of the list command');
+    ok((none { /az vm boot-diagnostics get-boot-log/ } @calls), 'Correct composition of the main command');
+    ok scalar @ret == 0, "No logs";
+};
+
+subtest '[az_vm_diagnostic_log_get] one fail' => sub {
+    my $azcli = Test::MockModule->new('sles4sap::azure_cli', no_auto => 1);
+    my @calls;
+    $azcli->redefine(script_output => sub {
+            push @calls, $_[0];
+            # simulate 2 VM
+            return '[{"id": "0001", "name": "Truffaldino"}, {"id": "0002", "name": "Mirandolina"}]'; });
+    $azcli->redefine(script_run => sub {
+            my ($cmd) = @_;
+            push @calls, $cmd;
+            return ($cmd =~ /Mirandolina/) ? 1 : 0; });
+    $azcli->redefine(record_info => sub { note(join(' ', 'RECORD_INFO -->', @_)); });
+
+    my @ret = az_vm_diagnostic_log_get(resource_group => 'Arlecchino');
+
+    note("\n  C-->  " . join("\n  C-->  ", @calls));
+    note("\n  R-->  " . join("\n  R-->  ", @ret));
+    ok((any { /az vm list/ } @calls), 'Correct composition of the list command');
     ok((any { /az vm boot-diagnostics get-boot-log/ } @calls), 'Correct composition of the main command');
-    ok((any { /--ids 0001.*tee.*Truffaldino\.txt/ } @calls), 'Correct composition of the --id for the first VM');
-    ok((any { /--ids 0002.*tee.*Mirandolina\.txt/ } @calls), 'Correct composition of the --id for the second VM');
+    ok((any { /--ids 0001.*Truffaldino\.txt/ } @calls), 'Correct composition of the --id for the first VM');
+    ok((any { /--ids 0002.*Mirandolina\.txt/ } @calls), 'Correct composition of the --id for the second VM');
+
+    ok scalar @ret == 1, "One log for the non failing VM";
+    ok((any { /\/tmp\/boot-diagnostics_.*/ } @ret), 'Correct log filenames');
 };
 
 subtest '[az_storage_account_create]' => sub {