Fix for mapping guest OS type read from OVF to existing guest OS in C… by harikrishna-patnala · Pull Request #4553 · apache/cloudstack

harikrishna-patnala · 2020-12-19T19:20:43Z

Description

This fix address the issue when CloudStack does reverse look up for guest OS type read from OVF to guest OS type in CloudStack during VMware template registration. After reading the OS type from OVF and while trying to map in CS, currently the first in the list of guest_os_hypervisor table with guest os name is picked and assigned to template. This cannot be true always as there are cases that multiple entries in guest_os_hypervisor table can be of different versions of same OS or it can also be different types of OS as well (in case of types like Other 32bit or otherGuest)

For example

"otherGuest" os name in OVF file can map to guest_os_id either to "85" or "60". But these two are completely different OS types and there chances that wrong guest OS is marked to template.

We are trying to fix the problem using the description field of the OS in OVF file and equals ignore match with display name in guest_os table. If description field is not present or if it does not match with any of guest os display names, then last in the list of guest_os_hypervisor table is picked so that latest version of that guest OS is assigned to the template.

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?

Tested registering a template with different types of OSes in OVF file.

OVF file containing osType: darwin18_64Guest
- CS maps to single entry in guest_os_hypervisor table and OS type is assigned to "macOS 10.14 (64 bit)"
OVF file containing osType: "otherGuest' and Description: "(other 32-bit)"
- CS maps to two entries in guest_os_hypervisor table, but based on description it assigned to "other 32-bit"

OVF file containing osType: "otherGuest" and no Description
- CS maps to two entries in guest_os_hypervisor table but since no description is there CS maps to the latest guest os type which is "other 32-bit"
MariaDB [cloud]> select * from guest_os where id in (85,60) order by id DESC;
+----+-------------+------+--------------------------------------+------------------+---------------------+---------+-----------------+
| id | category_id | name | uuid | display_name | created | removed | is_user_defined |
+----+-------------+------+--------------------------------------+------------------+---------------------+---------+-----------------+
| 85 | 9 | NULL | 06bb4c79-1e50-11eb-bde5-1e005f013ade | SCO OpenServer 5 | 2020-11-04 03:44:52 | NULL | 0 |
| 60 | 7 | NULL | 06b43ca5-1e50-11eb-bde5-1e005f013ade | Other (32-bit) | 2020-11-04 03:44:52 | NULL | 0 |
+----+-------------+------+--------------------------------------+------------------+---------------------+---------+-----------------+

harikrishna-patnala · 2020-12-20T17:26:09Z

@blueorangutan package

blueorangutan · 2020-12-20T17:27:16Z

@harikrishna-patnala a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-12-20T18:03:59Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2501

yadvr · 2020-12-20T20:12:01Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2020-12-20T20:13:17Z

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

shwstppr

Code changes lgtm

blueorangutan · 2020-12-21T07:01:50Z

Trillian test result (tid-3355)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37325 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4553-t3355-vmware-67u3.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_deploy_and_upgrade_kubernetes_cluster	`Failure`	769.20	test_kubernetes_clusters.py

yadvr · 2020-12-21T07:10:58Z

Marking it as blocker - the deploy as-is created regression in VM template registration.

harikrishna-patnala · 2020-12-21T08:16:20Z

@blueorangutan package

blueorangutan · 2020-12-21T08:17:19Z

@harikrishna-patnala a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

DaanHoogland

I think the assuption of what is "like" another string is so dangerous that we might as well just pick the last if the strings aren't equal.

DaanHoogland · 2020-12-21T08:28:03Z

+     * @param str1
+     *      String that needs editing
+     * @param str2
+     *      Target string that needs after editing


Suggested change

* @param str1

* String that needs editing

* @param str2

* Target string that needs after editing

* @param str1 String that needs editing

* @param str2 Target string that is required/expected after editing

Updated this section with suggested changes

DaanHoogland · 2020-12-21T08:34:42Z

+     *      Target string that needs after editing
+     * @return minimum number of edits required to convert str1 to str2
+     */
+    public static int minimumEditDistance(String str1, String str2) {


can we have some unit tests for this, please?
I think it assumes a definition of what an edit is, that we need to make explicit as well.

Added unit tests in new commit.

blueorangutan · 2020-12-21T08:57:53Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2503

DaanHoogland · 2020-12-21T11:45:27Z

+    public void testBestMatchStringWithEditDistance() {
+        String str1 = "FreeBSD 11 (32bit)";
+        String str2 = "FreeBSD 12 (64bit)";
+        String targetString = "FreeBSD 64bit";


maybe not here but as extra:

Suggested change

public void testBestMatchStringWithEditDistance() {

String str1 = "FreeBSD 11 (32bit)";

String str2 = "FreeBSD 12 (64bit)";

String targetString = "FreeBSD 64bit";

public void testBestFreeBSDWithEditDistance() {

String str1 = "FreeBSD 11 (32bit)";

String str2 = "FreeBSD 22 (64bit)";

String targetString = "FreeBSD 11 (64bit)";

Suggested change

public void testBestMatchStringWithEditDistance() {

String str1 = "FreeBSD 11 (32bit)";

String str2 = "FreeBSD 12 (64bit)";

String targetString = "FreeBSD 64bit";

public void testBestFedoraWithEditDistance() {

String str1 = "Fedora 29 (32bit)";

String str2 = "FreeBSD 32 (64bit)";

String targetString = "FreeBSD 33 (32bit)";

Suggested change

public void testBestMatchStringWithEditDistance() {

String str1 = "FreeBSD 11 (32bit)";

String str2 = "FreeBSD 12 (64bit)";

String targetString = "FreeBSD 64bit";

public void testBestUbuntuWithEditDistance() {

String str1 = "ubuntu 14.04 (32bit)";

String str2 = "ubuntu 20.10 (64bit)";

String targetString = "ubuntu 18.04 (64bit)";

nvazquez

I like the proposed approach, but I'm also thinking that we could check the longest prefix match or a mix of both approaches.

Examples:

Ubuntu 4.10
Ubuntu 8.10

Ubuntu 8.04
Ubuntu 8.10

In case 1) we have minDistance = 1 however by longer prefix we could have chosen case 2)

Opinions? @DaanHoogland @rhtyd @harikrishna-patnala @andrijapanicsb

DaanHoogland · 2020-12-21T14:37:46Z

ubuntu 18.04 (32bit) matching
ubuntu 18.04 (64bit) or
ubuntu 20.10 (32bit), should surely match the 32 bit rather than the exact version. How can we encode this?

harikrishna-patnala · 2020-12-21T15:56:31Z

ubuntu 18.04 (32bit) matching
ubuntu 18.04 (64bit) or
ubuntu 20.10 (32bit), should surely match the 32 bit rather than the exact version. How can we encode this?

since we are using a generic algorithm here I think if we have to handle these kind of special cases we need to add special checks for bits. Likewise if there is some other scenario we need to handle that separately too.. Since this string comes from a description 32 bit can written in any format (32-bit, 32bit, 32 bit, 32_bit). So atleast in this case I don't see a generic solution at the moment.

DaanHoogland · 2020-12-21T17:54:50Z

fair enough @harikrishna-patnala . I don't think we can rely on anything but an unlikely exact match.

blueorangutan · 2020-12-22T00:58:27Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-12-22T01:40:17Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2506

yadvr · 2020-12-22T01:41:50Z

@blueorangutan test matrix

blueorangutan · 2020-12-22T01:42:28Z

@rhtyd a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

blueorangutan · 2020-12-22T12:21:31Z

Trillian test result (tid-3360)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36792 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4553-t3360-xenserver-71.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_scale_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_scale_vm	`Failure`	10.25	test_scale_vm.py

harikrishna-patnala · 2020-12-22T13:42:21Z

@DaanHoogland @rhtyd @nvazquez updated the logic to equals ignore case (removed min edit distance algorithm). I've retested the cases mentioned in the description. Please review

yadvr · 2020-12-22T13:43:03Z

@blueorangutan package

blueorangutan · 2020-12-22T13:43:36Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

yadvr · 2020-12-22T13:44:38Z

+                        }
+                    }
+                }
+                if (null == guestOsId) {


Minor nit - I think variable == null maybe more readable, but as I said that's not picking

depends on how accustomed you are in right to left reading ;)

making habit of this to avoid setting null to the variable by mistake, in that case IDE also don't know if it is logically wrong "if (guestOsId = null)" :p

DaanHoogland

I think we can agree this is the least bad solution (for now)

blueorangutan · 2020-12-22T14:22:59Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2509

yadvr · 2020-12-22T14:25:27Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2020-12-22T14:26:30Z

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

blueorangutan · 2020-12-23T02:50:59Z

Trillian test result (tid-3366)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42805 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4553-t3366-vmware-67u3.zip
Intermittent failure detected: /marvin/tests/smoke/test_human_readable_logs.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 83 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_enableHumanReadableLogs	`Error`	0.31	test_human_readable_logs.py
test_03_deploy_and_upgrade_kubernetes_cluster	`Failure`	786.41	test_kubernetes_clusters.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	275.74	test_vpc_redundant.py
test_02_redundant_VPC_default_routes	`Failure`	273.51	test_vpc_redundant.py

…loudStack database while registering VMware template

…s display name

harikrishna-patnala · 2020-12-23T12:45:46Z

Resolved the merge conflicts. @rhtyd @nvazquez Please do the last review

harikrishna-patnala · 2020-12-23T12:46:06Z

@blueorangutan package

blueorangutan · 2020-12-23T12:46:34Z

@harikrishna-patnala a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-12-23T13:36:16Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2514

nvazquez

LGTM

* commit '280c13a4bb103dd748ec304bfe0714a148c24602': Updating pom.xml version numbers for release 4.15.0.0 kvm: Fix double-escape issue while creating rbd disk options (apache#4568) networkorchestrator: Fix typo in exception message (apache#4559) checkstyle: fix pom version Updating pom.xml version numbers for release 4.15.1.0-SNAPSHOT Updating pom.xml version numbers for release 4.15.0.0 vmware: Fix for mapping guest OS type read from OVF to existing guest OS in C… (apache#4553) vmware: Fix template upload from local (apache#4555)

harikrishna-patnala requested review from DaanHoogland and yadvr December 19, 2020 19:21

harikrishna-patnala added this to the 4.15.0.0 milestone Dec 19, 2020

yadvr requested review from andrijapanicsb, borisstoyanov, nvazquez and vladimirpetrov December 21, 2020 06:39

yadvr approved these changes Dec 21, 2020

View reviewed changes

yadvr closed this Dec 21, 2020

yadvr reopened this Dec 21, 2020

shwstppr approved these changes Dec 21, 2020

View reviewed changes

yadvr added the Severity:BLOCKER label Dec 21, 2020

DaanHoogland reviewed Dec 21, 2020

View reviewed changes

nvazquez reviewed Dec 21, 2020

View reviewed changes

yadvr closed this Dec 22, 2020

yadvr reopened this Dec 22, 2020

harikrishna-patnala force-pushed the guestOSMappingfromOVFinVMWare branch from f03be40 to 2815d01 Compare December 22, 2020 13:38

yadvr approved these changes Dec 22, 2020

View reviewed changes

yadvr reviewed Dec 22, 2020

View reviewed changes

DaanHoogland approved these changes Dec 22, 2020

View reviewed changes

yadvr approved these changes Dec 23, 2020

View reviewed changes

harikrishna-patnala added 4 commits December 23, 2020 17:31

Fix for mapping guest OS type read from OVF to existing guest OS in C…

7c79723

…loudStack database while registering VMware template

Added unit tests to String Utils methods and updated the code

f72f2fd

Updated the java doc section

c1a9f41

Updated os description logic to keep equals ignore match with guest o…

4b8adeb

…s display name

harikrishna-patnala force-pushed the guestOSMappingfromOVFinVMWare branch from 2815d01 to 4b8adeb Compare December 23, 2020 12:42

yadvr approved these changes Dec 23, 2020

View reviewed changes

nvazquez approved these changes Dec 23, 2020

View reviewed changes

yadvr merged commit b1ddd7c into master Dec 23, 2020

Conversation

harikrishna-patnala commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

harikrishna-patnala commented Dec 20, 2020

Uh oh!

blueorangutan commented Dec 20, 2020

Uh oh!

blueorangutan commented Dec 20, 2020

Uh oh!

yadvr commented Dec 20, 2020

Uh oh!

blueorangutan commented Dec 20, 2020

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Dec 21, 2020

Uh oh!

yadvr commented Dec 21, 2020

Uh oh!

harikrishna-patnala commented Dec 21, 2020

Uh oh!

blueorangutan commented Dec 21, 2020

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Dec 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Dec 21, 2020

Uh oh!

harikrishna-patnala commented Dec 21, 2020

Uh oh!

DaanHoogland commented Dec 21, 2020

Uh oh!

blueorangutan commented Dec 22, 2020

Uh oh!

blueorangutan commented Dec 22, 2020

Uh oh!

yadvr commented Dec 22, 2020

Uh oh!

blueorangutan commented Dec 22, 2020

Uh oh!

blueorangutan commented Dec 22, 2020

Uh oh!

harikrishna-patnala commented Dec 22, 2020

Uh oh!

yadvr commented Dec 22, 2020

Uh oh!

blueorangutan commented Dec 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala commented Dec 19, 2020 •

edited

Loading