default should map always, not require the minHypervisorVersion #6841

DaanHoogland · 2022-10-21T13:03:51Z

Description

This PR fixes an logic error in #6758

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

…rorVersion

DaanHoogland · 2022-10-21T13:20:53Z

@blueorangutan package

blueorangutan · 2022-10-21T13:22:03Z

@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-10-21T13:59:28Z

Packaging result: ✔️ el7 ✔️ el8 ✖️ debian ✔️ suse15. SL-JID 4520

sonarcloud · 2022-10-21T14:08:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

DaanHoogland · 2022-10-21T14:25:09Z

@blueorangutan test

blueorangutan · 2022-10-21T14:26:03Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2022-10-22T02:56:43Z

Trillian test result (tid-5187)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43602 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6841-t5187-kvm-centos7.zip
Smoke tests completed. 103 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	462.06	test_vpc_redundant.py

weizhouapache

code lgtm

However, need to verify if #6736 is fixed

blueorangutan · 2022-10-24T18:51:10Z

Trillian test result (tid-5190)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41170 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6841-t5190-kvm-centos7.zip
Smoke tests completed. 103 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	458.19	test_vpc_redundant.py

DaanHoogland · 2022-11-03T08:53:34Z

@borisstoyanov I think we need this in for the upload of OVAs not having a hypervisor version and preventing those that have a too high hypervisor version.

blueorangutan · 2022-12-02T13:46:17Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 4746

blueorangutan · 2022-12-02T15:51:48Z

Packaging result: ✖️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 4750

blueorangutan · 2022-12-02T17:28:23Z

Packaging result: ✔️ el7 ✖️ el8 ✔️ debian ✔️ suse15. SL-JID 4754

blueorangutan · 2022-12-06T10:00:48Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 4777

DaanHoogland · 2022-12-06T15:51:18Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2022-12-06T15:52:03Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

blueorangutan · 2022-12-07T04:07:07Z

Trillian test result (tid-5407)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42907 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6841-t5407-vmware-67u3.zip
Smoke tests completed. 95 look OK, 2 have errors, 8 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_3d_gpu_support	`Error`	594.47	test_deploy_vgpu_enabled_vm.py
test_deploy_vgpu_enabled_vm	`Error`	3.02	test_deploy_vgpu_enabled_vm.py
test_13_migrate_volume_and_change_offering	`Error`	15.57	test_volumes.py
ContextSuite context=TestVolumes>:teardown	`Error`	22.30	test_volumes.py
all_test_internal_lb	`Skipped`	---	test_internal_lb.py
all_test_templates	`Skipped`	---	test_templates.py
all_test_create_list_domain_account_project	`Skipped`	---	test_create_list_domain_account_project.py
all_test_primary_storage	`Skipped`	---	test_primary_storage.py
all_test_create_network	`Skipped`	---	test_create_network.py
all_test_network_acl	`Skipped`	---	test_network_acl.py
all_test_network_ipv6	`Skipped`	---	test_network_ipv6.py
all_test_deploy_vm_extra_config_data	`Skipped`	---	test_deploy_vm_extra_config_data.py

blueorangutan · 2022-12-09T07:05:58Z

Trillian test result (tid-5434)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 52438 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6841-t5434-vmware-67u3.zip
Smoke tests completed. 104 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_upgrade_kubernetes_cluster	`Error`	4005.55	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	583.69	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	381.46	test_kubernetes_clusters.py

weizhouapache · 2022-12-09T07:17:11Z

code lgtm, but I wonder ##6736 will be back again

DaanHoogland · 2022-12-12T09:54:15Z

code lgtm, but I wonder ##6736 will be back again

I don´t think so. @borisstoyanov can you look at that?

DaanHoogland · 2022-12-29T16:18:54Z

@DaanHoogland this is closed ?

yes it was, forgot to reopen to restart actions...

codecov · 2022-12-29T18:45:21Z

Codecov Report

Merging #6841 (7a48cef) into main (4a06363) will increase coverage by 0.89%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #6841      +/-   ##
============================================
+ Coverage     10.84%   11.73%   +0.89%     
- Complexity     7095     8028     +933     
============================================
  Files          2485     2514      +29     
  Lines        245370   266358   +20988     
  Branches      38319    46572    +8253     
============================================
+ Hits          26602    31257    +4655     
- Misses       215500   230996   +15496     
- Partials       3268     4105     +837

Impacted Files	Coverage Δ
...om/cloud/storage/dao/GuestOSHypervisorDaoImpl.java	`32.25% <0.00%> (ø)`
...va/com/cloud/upgrade/dao/DatabaseAccessObject.java	`0.00% <0.00%> (-97.50%)`	⬇️
...ain/java/com/cloud/upgrade/dao/DbUpgradeUtils.java	`0.00% <0.00%> (-92.31%)`	⬇️
...in/java/com/cloud/upgrade/dao/Upgrade444to450.java	`0.00% <0.00%> (-50.00%)`	⬇️
...in/java/com/cloud/upgrade/dao/Upgrade453to460.java	`0.00% <0.00%> (-50.00%)`	⬇️
...java/com/cloud/upgrade/DatabaseUpgradeChecker.java	`0.00% <0.00%> (-46.67%)`	⬇️
...in/java/com/cloud/upgrade/dao/Upgrade431to440.java	`0.00% <0.00%> (-40.00%)`	⬇️
...in/java/com/cloud/upgrade/dao/Upgrade432to440.java	`0.00% <0.00%> (-40.00%)`	⬇️
...in/java/com/cloud/upgrade/dao/LegacyDbUpgrade.java	`0.00% <0.00%> (-33.34%)`	⬇️
.../com/cloud/storage/dao/StoragePoolTagsDaoImpl.java	`12.90% <0.00%> (-32.26%)`	⬇️
... and 348 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

vladimirpetrov · 2023-01-18T15:43:17Z

From what I can see, this haven't been fixed yet, here is what I got when testing as described in issue #6736:

When registering the mentioned template (PI-VA-3.10.0.0.205.ova) with 'Read from OVA', here is the result:
Did not find a guest OS (Red Hat Enterprise Linux 7 (64-bit)) with type "rhel7_64Guest" and minimal hypervisor hardware version null.

when 'Read from OVA' is false, the same template is properly registered.

sonarcloud · 2023-01-18T17:03:38Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

DaanHoogland · 2023-01-19T08:39:43Z

From what I can see, this haven't been fixed yet, here is what I got when testing as described in issue #6736:

When registering the mentioned template (PI-VA-3.10.0.0.205.ova) with 'Read from OVA', here is the result: Did not find a guest OS (Red Hat Enterprise Linux 7 (64-bit)) with type "rhel7_64Guest" and minimal hypervisor hardware version null.

when 'Read from OVA' is false, the same template is properly registered.

i think we have to check if this is a scenario that would work before this PR.

rohityadavcloud · 2023-05-08T07:23:15Z

engine/schema/src/main/java/com/cloud/storage/dao/GuestOSHypervisorDaoImpl.java

@@ -157,7 +157,7 @@ public List<GuestOSHypervisorVO> listByOsNameAndHypervisorMinimumVersion(String
        sc.and(sc.entity().getGuestOsName(), SearchCriteria.Op.EQ, guestOsName);
        sc.and(sc.entity().getHypervisorType(), SearchCriteria.Op.EQ, hypervisorType);
        sc.and().op(sc.entity().getHypervisorVersion(), SearchCriteria.Op.GTEQ, minHypervisorVersion);
-        sc.or(sc.entity().getHypervisorVersion(), SearchCriteria.Op.NEQ, "default");
+        sc.or(sc.entity().getHypervisorVersion(), SearchCriteria.Op.EQ, "default");


should this be GTEQ?

no, default is not part of the simver and it is OR'ed so I think EQ is good

weizhouapache · 2023-05-12T08:00:45Z

added to 4.18.1.0 milestone

rohityadavcloud · 2023-06-27T09:33:53Z

Not sure if manual testing is needed, but other LGTM. Thanks @DaanHoogland

rohityadavcloud · 2023-08-08T13:32:43Z

ping @weizhouapache
@blueorangutan package

blueorangutan · 2023-08-08T13:34:03Z

@rohityadavcloud a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-08-08T14:45:18Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6707

weizhouapache · 2023-08-17T14:03:22Z

manual test is required to test if #6736 is fixed or not.

weizhouapache · 2023-08-17T14:10:49Z

@DaanHoogland
I doubt if it works.

it looks default have much fewer guest os mappings than 7.0. If we search guest os mapping by hypervisor version default, it may return empty results, which leads to #6736 to appear again.

mysql> select count(1) from guest_os_hypervisor where hypervisor_type ='VMware' and hypervisor_version ='default';
+----------+
| count(1) |
+----------+
|      118 |
+----------+
1 row in set (0.00 sec)

mysql> 
mysql> 
mysql> select count(1) from guest_os_hypervisor where hypervisor_type ='VMware' and hypervisor_version ='7.0';
+----------+
| count(1) |
+----------+
|      258 |
+----------+
1 row in set (0.00 sec)

DaanHoogland · 2023-08-18T08:12:10Z

@weizhouapache I think this is a solution looking for a problem. Shall I close?

weizhouapache · 2023-08-18T08:29:37Z

@weizhouapache I think this is a solution looking for a problem. Shall I close?

@DaanHoogland
The old logic seems wrong but fixed #6736 (the return results are always not empty)
the new logic looks good, but #6736 may come back again (the return results might be null).
I have no idea how to proceed ..

DaanHoogland · 2023-08-18T09:53:29Z

@weizhouapache I think this is a solution looking for a problem. Shall I close?

@DaanHoogland The old logic seems wrong but fixed #6736 (the return results are always not empty) the new logic looks good, but #6736 may come back again (the return results might be null). I have no idea how to proceed ..

ok, closing. we can always re-open

default should allow mapping, not default should match the minHypervi…

7a48cef

…rorVersion

boring-cyborg bot added the component:storage label Oct 21, 2022

DaanHoogland marked this pull request as ready for review October 24, 2022 13:22

DaanHoogland requested review from borisstoyanov and weizhouapache October 24, 2022 13:23

weizhouapache reviewed Oct 24, 2022

View reviewed changes

DaanHoogland changed the title ~~default should map always, not should match the minHypervirorVersion~~ default should map always, not the minHypervirorVersion Nov 3, 2022

DaanHoogland changed the title ~~default should map always, not the minHypervirorVersion~~ default should map always, not require the minHypervisorVersion Nov 3, 2022

DaanHoogland assigned DaanHoogland and borisstoyanov and unassigned DaanHoogland Dec 2, 2022

DaanHoogland reopened this Dec 29, 2022

DaanHoogland assigned vladimirpetrov Dec 30, 2022

DaanHoogland requested a review from vladimirpetrov December 30, 2022 11:15

DaanHoogland closed this Jan 18, 2023

DaanHoogland reopened this Jan 18, 2023

rohityadavcloud reviewed May 8, 2023

View reviewed changes

weizhouapache added this to the 4.18.1.0 milestone May 12, 2023

rohityadavcloud approved these changes Jun 27, 2023

View reviewed changes

DaanHoogland changed the base branch from main to 4.18 July 3, 2023 09:40

rohityadavcloud requested a review from weizhouapache July 7, 2023 17:29

weizhouapache modified the milestones: 4.18.1.0, 4.18.2.0 Aug 18, 2023

DaanHoogland added the Severity:Trivial label Aug 18, 2023

DaanHoogland closed this Aug 18, 2023

rohityadavcloud removed this from the 4.18.2.0 milestone Aug 29, 2023

default should map always, not require the minHypervisorVersion #6841

default should map always, not require the minHypervisorVersion #6841

Conversation

DaanHoogland commented Oct 21, 2022

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

DaanHoogland commented Oct 21, 2022

blueorangutan commented Oct 21, 2022

blueorangutan commented Oct 21, 2022

sonarcloud bot commented Oct 21, 2022

DaanHoogland commented Oct 21, 2022

blueorangutan commented Oct 21, 2022

blueorangutan commented Oct 22, 2022

weizhouapache left a comment

Choose a reason for hiding this comment

blueorangutan commented Oct 24, 2022

DaanHoogland commented Nov 3, 2022

blueorangutan commented Dec 2, 2022

blueorangutan commented Dec 2, 2022

blueorangutan commented Dec 2, 2022

blueorangutan commented Dec 6, 2022

DaanHoogland commented Dec 6, 2022

blueorangutan commented Dec 6, 2022

blueorangutan commented Dec 7, 2022

blueorangutan commented Dec 9, 2022

weizhouapache commented Dec 9, 2022

DaanHoogland commented Dec 12, 2022

DaanHoogland commented Dec 29, 2022

codecov bot commented Dec 29, 2022 • edited

Codecov Report

vladimirpetrov commented Jan 18, 2023

sonarcloud bot commented Jan 18, 2023

DaanHoogland commented Jan 19, 2023

rohityadavcloud May 8, 2023

Choose a reason for hiding this comment

DaanHoogland May 9, 2023

Choose a reason for hiding this comment

weizhouapache commented May 12, 2023

rohityadavcloud commented Jun 27, 2023

rohityadavcloud commented Aug 8, 2023

blueorangutan commented Aug 8, 2023

blueorangutan commented Aug 8, 2023

weizhouapache commented Aug 17, 2023

weizhouapache commented Aug 17, 2023 • edited

DaanHoogland commented Aug 18, 2023

weizhouapache commented Aug 18, 2023

DaanHoogland commented Aug 18, 2023

codecov bot commented Dec 29, 2022 •

edited

weizhouapache commented Aug 17, 2023 •

edited