SAPHanaSR-showAttr output is now parsed to decode_json() like format #21264
Conversation
|
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
Can you also add VRs for SPN and MSI? |
|
https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/t/18_saputils.t |
There was a problem hiding this comment.
I'm trying to figure out how existing test [calculate_hana_topology] invalid output is working with the new code, or better to say I'm trying to figure out how the new code is working with invalid input. Please follow and correct my analysis if needed:
Input is an invalid string like PUFFI, so we are entering the else. @all_lines is getting only one element.
@hosts_parameters , @globals_parameters and @resources_parameters will all be empty.
So also @all_hosts, @all_globals and @all_resources will also be empty.
... but then I cannot follow anymore...
For example I cannot understand how code like this one below does not result in an exception when input is invalid
$topology{'Global'}{'global'}{'cib-last-written'} = $script_topology{'global'}->{'cib-time'};
| Sites/site_b/b="SOK" | ||
| Hosts/vmhana01/remoteHost="vmhana02" | ||
| Hosts/vmhana01/sync_state="PRIM" |
There was a problem hiding this comment.
why to remove this line? It is present in one of your VR https://openqaworker15.qa.suse.cz/tests/314953/logfile?filename=serial_terminal.txt
|
|
||
| while (my ($key, $value) = each %$topology) { | ||
| ok((keys %$value eq 3), "Parsed input has 3 values for each host, so 3 inner keys."); | ||
| while (my ($key, $value) = each %{$topology->{'Host'}}) { |
There was a problem hiding this comment.
As your new result structure is changes please add more code in this test to also validate the %{$topology->{'Site'}} part
| ok((any { qr/vmhana02/ } keys %$topology), 'External hash has key vmhana02'); | ||
| ok keys %{$topology->{'Host'} == 2, 'Output is about exactly 2 hosts'; | ||
| ok((any { qr/vmhana01/ } keys %{$topology->{'Host'}}), 'External hash has key vmhana01'); | ||
| ok((any { qr/vmhana02/ } keys %{$topology->{'Host'}}), 'External hash has key vmhana02'); | ||
| }; | ||
|
|
||
| subtest '[calculate_hana_topology] internal keys' => sub { |
There was a problem hiding this comment.
Please add a new test about calculate_hana_topology to use integer value for node_state
lib/saputils.pm
Outdated
| $topology{'Site'}{$script_topology{$host}->{'site'}}{'opMode'} = $script_topology{$host}->{'op_mode'}; | ||
| $topology{'Site'}{$script_topology{$host}->{'site'}}{'srMode'} = $script_topology{$host}->{'srmode'}; | ||
| $topology{'Site'}{$script_topology{$host}->{'site'}}{'srPoll'} = $script_topology{$host}->{'sync_state'}; | ||
| $topology{'Site'}{$script_topology{$host}->{'site'}}{'lss'} = ($script_topology{$host}->{'node_state'} eq 'online' or $script_topology{$host}->{'node_state'} =~ /[1-9]+/) ? '4' : '1'; |
There was a problem hiding this comment.
So, no matter of which pacemaker or saphana_showattr is installed in the SUT, we are internally always represent the state with 4 or 1
Am I reading it right?
Before this PR the calculate_hana_topology was transparently copy over whatever node_state value was in the output. Then the caller, before to call check_hana_topology was deciding what was the expected string to match
How it is working now is that, without to care about which pacemaker is installed, we check for online or for a digit:
- If there's
onlineor a digit, no matter which digit, we represent it with4 - otherwise we represent it with
1,
Am I interpreting it right? Is it ok we put 4 no matter which digit it was? Is it ok we are doing this translation without to care the pacemaker version?
lib/sles4sap_publiccloud.pm
Outdated
|
|
||
| my $topology = $self->get_hana_topology(); | ||
| foreach (qw(vhost remoteHost srmode op_mode)) { die "Missing '$_' field in topology output" unless defined(%$topology{$hostname}->{$_}); } | ||
| for my $site (keys %{$topology->{'Site'}}) { | ||
| foreach (qw(srMode opMode)) { die("enable_replication [ERROR] Missing '$_' field in topology output of Site->$site") unless defined($topology->{'Site'}->{$site}->{$_}); } |
There was a problem hiding this comment.
can we split it in multiple line (just different indentation)
There was a problem hiding this comment.
Is this test on srMode and opMode to be performed here for all site even if we only care about $args{site_name} ?
| data later used for "ansible: hana_vars:" section in config.yaml file. | ||
|
|
||
| =cut | ||
|
|
||
| sub create_hana_vars_section { | ||
| my ($ha_enabled) = @_; |
There was a problem hiding this comment.
I think it is a mistake in rebasing https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/21245/files#diff-4b7291e584e1d0b487f685a7b45efc74dcd804ee2236acf7a866003d0f4fb87aL1051
This argument is no more needed
| # Only takes parameter and value for lines about one specific host at time | ||
| my %host_parameter = map { | ||
| my ($node, $parameter, $value) = split(/[\/=]/, $_); | ||
| if ($host eq $node) { ($parameter, $value) } else { () } |
There was a problem hiding this comment.
How this line works in Perl?
if ($host eq $node) { ($parameter, $value) } else { () }
} @hosts_parameters;
There's an if/else, each branch has an expression like (something) (but it is not assigned to anything). Then after the if/else, without any separation, there's a list... is it a "conditional push to the list"?
If it is so, why we are pushing empty element () ?
Why not to have ?
push @hosts_parameters, ($parameter, $value) if ($host eq $node);
Read this last suggestion not like a request to change your code but as a request for an explanation about how it works.
| my $node_state_match = (defined $args{node_state_match}) ? | ||
| ($args{node_state_match} eq 'online' or ($args{node_state_match} =~ /[1-9][0-9]+/ xor $args{node_state_match} !~ /[0-3]/)) ? '4' : $args{node_state_match} : '4'; |
There was a problem hiding this comment.
This expression is overly complicated. First, I would remove the (defined $args{node_state_match}) ? as if $args{node_state_match} is undef, it will evaluate to the empty string. I think you'd better use something like:
my $node_state_match = $args{node_state_match} // '4';
$node_state_match = $args{node_state_match} =~ /^online$|^[1-9]\d+$/ ? '4' : $args{node_state_match};
The other thing is that if $args{node_state_match} matches to a number starting with 1-9 followed by another number, then it will not match to a single digit number between 0 and 3, so the third condition of the expression can be removed too.
Also, IIRC it's possible that Perl tidy will complain about a variable declaration with the ternary operator.
|
|
||
| return \%topology; | ||
| # We encode to the JSON to be sure that output is always same | ||
| $topology_json = encode_json(\%topology); |
There was a problem hiding this comment.
What's the purpose of calling encode_json and then right after calling decode_json? Couldn't you have returned \%topology directly?
SAPHanaSR-showAttr output is now parsed to decode_json() like format as first step to incllude ANGI support