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.

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

Avoid die in check_takeover for uninitialized variables #18330

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

mpagot
Copy link
Contributor

@mpagot mpagot commented Dec 15, 2023

Avoid an exception if get_hana_topology return an empty result. It could happens if SAPHanaSR-showAttr is not successful and output is not what expected.
This new behavior is obtained avoiding the usage of local variables in check_takeover.
Sleep 30 moved in a different position only to be used once for each get_hana_topology and not to be called when there's no more retry to run.
Add some unit testing for these two functions.

  • Related ticket: TEAM-8878

Verification run:

sle-15-SP4-HanaSr-Azure-Byos-x86_64-Build15-SP4_2023-12-15T05:03:11Z-hanasr_azure_test_sapconf_spn@64bit

# Test died: Use of uninitialized value $host_entry{"sync_state"}
 in string eq at /var/lib/openqa/pool/75/os-autoinst-distri-opensuse/lib/sles4sap_publiccloud.pm line 365.

It is because field sync_state is missing in SAPHanaSR-showAttr --format=script
It needs PR improvement: https://github.com/os-autoinst/os-autoinst-distri-opensuse/compare/117380726ba2dd7f10aa6c2a3078a08886185ed7..18da85a3f6ba7f6965c97064170154e325960242

sle-12-SP5-Azure-SAP-PAYG-Incidents-x86_64-Build:31892:nfs-utils-SAPHanaSR-ScaleUp-PerfOpt@az_Standard_E8s_v3

@mpagot mpagot marked this pull request as draft December 15, 2023 14:24
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files.

@mpagot mpagot added the WIP Work in progress label Dec 15, 2023
@mpagot mpagot force-pushed the sles4sap_get_hana_topology branch from 0e292c1 to 7b36f97 Compare December 15, 2023 14:27
@mpagot mpagot force-pushed the sles4sap_get_hana_topology branch from 7b36f97 to 1173807 Compare December 15, 2023 14:38
Copy link
Contributor

@alvarocarvajald alvarocarvajald left a comment

Choose a reason for hiding this comment

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

LGTM

@lilyeyes
Copy link
Contributor

But in the VRs the Test died: Use of uninitialized value $host_entry{"sync_state"} in string eq ... is still existing with this branch: http://openqaworker15.qa.suse.cz/tests/273037#step/Crash_site_b-primary/99

Copy link
Contributor

@lilyeyes lilyeyes left a comment

Choose a reason for hiding this comment

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

LGTM.

Avoid an exception if get_hana_topology return an empty result. It could
happens if SAPHanaSR-showAttr is not succesful and output is not
parsable.
This new behavior is obtained avoiding the usage of local variables in
check_takeover.
Sleep 30 moved in a different position only to be used once for each
get_hana_topology and not to be called when there's no more retry to
run.
Add some unit testing for these two functions.
@mpagot mpagot force-pushed the sles4sap_get_hana_topology branch from 1173807 to 18da85a Compare December 18, 2023 09:59
@mpagot mpagot marked this pull request as ready for review December 18, 2023 11:14
@mpagot mpagot removed the WIP Work in progress label Dec 18, 2023
Copy link
Contributor

@hadeskun hadeskun left a comment

Choose a reason for hiding this comment

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

LGTM

@mpagot
Copy link
Contributor Author

mpagot commented Dec 18, 2023

But in the VRs the Test died: Use of uninitialized value $host_entry{"sync_state"} in string eq ... is still existing with this branch: http://openqaworker15.qa.suse.cz/tests/273037#step/Crash_site_b-primary/99

Yes, fixed in https://github.com/os-autoinst/os-autoinst-distri-opensuse/compare/117380726ba2dd7f10aa6c2a3078a08886185ed7..18da85a3f6ba7f6965c97064170154e325960242

@mpagot mpagot merged commit d7cf7b8 into os-autoinst:master Dec 18, 2023
8 checks passed
Copy link
Contributor

@lpalovsky lpalovsky left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants