Fix sdaf network peering issue #22709
Conversation
lilyeyes
force-pushed
the
fix-sdaf-network-peering-issue2
branch
13 times, most recently
from
79f4777 to
a7e882d
Compare
lilyeyes
requested review from
a-kpappas,
alvarocarvajald,
Amrysliu,
BillAnastasiadis,
emiura,
jankohoutek,
lpalovsky and
mpagot
as code owners
lilyeyes
force-pushed
the
fix-sdaf-network-peering-issue2
branch
from
a7e882d to
d30d107
Compare
TEAM-10465 - [SDAF] Fix network space assignment logic
lilyeyes
force-pushed
the
fix-sdaf-network-peering-issue2
branch
from
d30d107 to
d51d8c4
Compare
| @@ -311,6 +311,7 @@ sub assign_defined_network { | |||
| my @lease_files; | |||
| my $lease_file; | |||
| my $count = 0; | |||
| my $num = 10; # Exit the dead loop if exceed | |||
There was a problem hiding this comment.
What about covering the new behavior in a unit test in 18_networking.t ?
| @@ -223,7 +223,7 @@ sub acquire_network_file_lease { | |||
| my $lease_id = az_storage_blob_lease_acquire(%arguments); | |||
|
|
|||
| # Return unless the file lease was successful | |||
| return unless $lease_id; | |||
| return if (!$lease_id || $lease_id =~ 'ErrorCode'); | |||
There was a problem hiding this comment.
I think this change is not needed here. I think we should improve the handling of the return value within az_storage_blob_lease_acquire.
That function seems to need some adjustment. For example:
- here it has a broken documentation sentence https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/20203/files#diff-2b900fafc5f19c0adfc596d832d2c3fd8b3fc90b64ed8a2d2ad6680596f28924R1516
- generally speaking the documentation talk about the return value but focus more on the return of the inner called az cli than talking about what the perl function is going to call. The return value should exactly specify the data type of the return value.
The function is returning a string rappresenting ... or is returning .... if the .... - This line can be improved https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/20203/files#diff-2b900fafc5f19c0adfc596d832d2c3fd8b3fc90b64ed8a2d2ad6680596f28924R1553 or at least extended.
ErrorCodeis something coming out from the az cli, I think it is better to deal with this within theaz_storage_blob_lease_acquirethat is about the handling of the az cli.
return -1 if ($lease_id =~ 'ErrorCode');
return ($lease_id) if az_validate_uuid_pattern(uuid => $lease_id);
return -1; or return undef;
All of the 3 should be covered by Unit tests
There was a problem hiding this comment.
Thinking a little bit more about it: how is it possible az_validate_uuid_pattern is declaring a UUID to be valid if it contains ErrorCode. I think the code should be improved there.
I'm also thinking about az_validate_uuid_pattern return value. At the moment it return the input UUID string or 0. I think it could be changed to return 1 (valid UUID) or 0 (invalid UUID). Or something similar.
There was a problem hiding this comment.
here some suggestions about how to proceed
Add retry as well for sdaf network peering issue:
ErrorCode:LeaseAlreadyPresentPrevious PR #22666 fixed
when find network being used instead of checking other network from the list, it just created new file.There is also a PR from Michele to fix his comments left here: #22741
Related ticket: TEAM-10465 - [SDAF] Fix network space assignment logic
Verification run (test results are as expected):
https://openqaworker15.qa.suse.cz/tests/333419#step/deploy_workload_zone/95 (using fake data to produce: searched for
1times less than10times, foundErrorCodethen continue to search nextoldnetwork)https://openqaworker15.qa.suse.cz/tests/333419#step/deploy_workload_zone/128 (then found next
oldnetwork is usable at the second time, then use it)2.
https://openqaworker15.qa.suse.cz/tests/333411#step/deploy_workload_zone/359 (searched for
11times more than10times (all foundErrorCode), then returnto create anew` network)3.
https://openqaworker15.qa.suse.cz/tests/333420#step/deploy_workload_zone/92 (normal run is as expected)