db: make *_details.value non-nullable #5274

shwstppr · 2021-08-04T07:37:02Z

Description

Fixes #4897
Some details tables were allowing null values for detail value which can cause NPE in some cases.

mysql> SELECT TABLE_NAME, COLUMN_NAME, COLUMN_TYPE FROM information_schema.columns WHERE table_schema='cloud' AND table_name LIKE'%_details' AND column_name='value' AND IS_NULLABLE='YES';
+-------------------------------+-------------+---------------+
| TABLE_NAME                    | COLUMN_NAME | COLUMN_TYPE   |
+-------------------------------+-------------+---------------+
| account_details               | value       | varchar(255)  |
| cluster_details               | value       | varchar(255)  |
| data_center_details           | value       | varchar(1024) |
| domain_details                | value       | varchar(255)  |
| image_store_details           | value       | varchar(255)  |
| storage_pool_details          | value       | varchar(255)  |
| template_deploy_as_is_details | value       | text          |
| user_vm_deploy_as_is_details  | value       | text          |
| user_vm_details               | value       | varchar(5120) |
+-------------------------------+-------------+---------------+
9 rows in set (0.00 sec)

Brings consistency for value column of *_details tables with preventing null values.

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?

weizhouapache · 2021-08-04T07:51:29Z

@blueorangutan package

blueorangutan · 2021-08-04T07:52:03Z

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

blueorangutan · 2021-08-04T08:34:23Z

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

weizhouapache · 2021-08-04T09:01:38Z

@blueorangutan test

blueorangutan · 2021-08-04T09:02:04Z

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

nvazquez

Code LGTM. I'm not aware of any case that will attempt to add a detail key with a null value on these tables

blueorangutan · 2021-08-04T22:13:48Z

Trillian test result (tid-1473)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45528 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5274-t1473-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_resource_accounting.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 88 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_deploy_and_upgrade_kubernetes_cluster	`Failure`	716.21	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	233.74	test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	1.21	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.20	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	90.63	test_kubernetes_clusters.py

nvazquez · 2021-08-07T03:54:19Z

@blueorangutan test

blueorangutan · 2021-08-07T03:55:03Z

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

blueorangutan · 2021-08-07T04:30:07Z

Trillian Build Failed (tid-1526)

shwstppr · 2021-08-08T11:33:30Z

@blueorangutan test

blueorangutan · 2021-08-08T11:34:03Z

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

blueorangutan · 2021-08-09T02:11:27Z

Trillian test result (tid-1535)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 49467 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5274-t1535-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_list_ids_parameter.py
Intermittent failure detected: /marvin/tests/smoke/test_projects.py
Intermittent failure detected: /marvin/tests/smoke/test_templates.py
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 84 look OK, 5 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_create_template_with_checksum_sha1	`Error`	65.39	test_templates.py
test_03_create_template_with_checksum_sha256	`Error`	65.39	test_templates.py
test_04_create_template_with_checksum_md5	`Error`	65.39	test_templates.py
test_05_create_template_with_no_checksum	`Error`	65.40	test_templates.py
test_03_delete_template	`Error`	1.06	test_templates.py
test_04_extract_template	`Error`	1.18	test_templates.py
test_09_list_templates_download_details	`Failure`	0.04	test_templates.py
test_01_template_usage	`Error`	123.76	test_usage.py
test_01_volume_usage	`Error`	186.97	test_usage.py
test_06_download_detached_volume	`Error`	297.60	test_volumes.py
ContextSuite context=TestVPCNics>:setup	`Error`	0.00	test_vpc_router_nics.py
ContextSuite context=TestRVPCSite2SiteVpn>:setup	`Error`	0.00	test_vpc_vpn.py
ContextSuite context=TestVPCSite2SiteVPNMultipleOptions>:setup	`Error`	0.00	test_vpc_vpn.py
ContextSuite context=TestVpcRemoteAccessVpn>:setup	`Error`	0.00	test_vpc_vpn.py
ContextSuite context=TestVpcSite2SiteVpn>:setup	`Error`	0.00	test_vpc_vpn.py

rohityadavcloud

LGTM - needs testing

rohityadavcloud · 2021-08-09T08:31:37Z

@blueorangutan test

blueorangutan · 2021-08-09T08:32:03Z

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

blueorangutan · 2021-08-10T00:25:34Z

Trillian test result (tid-1540)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 54453 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5274-t1540-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_outofbandmanagement.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dnsservice.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 86 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_05_ping_in_cpvm_success	`Failure`	15.71	test_diagnostics.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	524.53	test_vpc_redundant.py
test_01_vpc_site2site_vpn_multiple_options	`Failure`	285.90	test_vpc_vpn.py

nvazquez · 2021-08-12T15:26:02Z

@shwstppr can you please solve the conflict?

GabrielBrascher

Code LGTM. I am +1 in having non-null values and I don't see any way of this PR causing regression issues.

However, I could not find why/how these values have been set to NULL.
Some of them seem to be created as NOT NULL values (see reate-schema.sql).
On the other hand, I could indeed find a case where the field was created without NOT NULL, such as template_deploy_as_is_details created in schema-41400to41500.sql.

Probably something that I am missing. Maybe a DB table change via Java or an upgrade schema-4XYto4XY+1.sql that would change values to NULL.

Has anyone mapped the cause of these NULL values? This question is just to avoid it happening again in the future.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2021-08-13T04:16:07Z

@GabrielBrascher Some tables have been altered using upgrade SQL (very old change, not much there in commit message - c11dbad9c9b), https://github.com/apache/cloudstack/blob/main/engine/schema/src/main/resources/META-INF/db/schema-410to420.sql#L1873-L1876

Others were created without NOT NULL constraint. Not sure if was for some use-case or an oversight
https://github.com/apache/cloudstack/blob/main/engine/schema/src/main/resources/META-INF/db/schema-41000to41100.sql#L576-L583
https://github.com/apache/cloudstack/blob/main/engine/schema/src/main/resources/META-INF/db/schema-41400to41500.sql#L555-L562

@blueorangutan package

blueorangutan · 2021-08-13T04:17:03Z

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

blueorangutan · 2021-08-13T04:42:42Z

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

GabrielBrascher · 2021-08-13T13:20:37Z

@shwstppr looks like a couple of legacy SQL upgrades then, thanks for checking it.
This PR LGTM 👍

nvazquez · 2021-08-13T14:08:28Z

@blueorangutan test

blueorangutan · 2021-08-13T14:09:03Z

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

blueorangutan · 2021-08-14T00:22:43Z

Trillian test result (tid-1629)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34902 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5274-t1629-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

* server: fix reset sshkey is broken in master/4.16 * Revert "server: fix reset sshkey is broken in master/4.16" This reverts commit db278cf. * update #5390 * server: fix another regression of #4819 and #5274 * update #5390

shwstppr mentioned this pull request Aug 4, 2021

Eliminate *_details table inconsistency #4897

Closed

nvazquez approved these changes Aug 4, 2021

View reviewed changes

nvazquez added this to the 4.16.0.0 milestone Aug 4, 2021

nvazquez added component:database type:cleanup labels Aug 4, 2021

shwstppr marked this pull request as ready for review August 6, 2021 04:40

nvazquez closed this Aug 7, 2021

nvazquez reopened this Aug 7, 2021

DaanHoogland approved these changes Aug 8, 2021

View reviewed changes

rohityadavcloud approved these changes Aug 9, 2021

View reviewed changes

GabrielBrascher approved these changes Aug 12, 2021

View reviewed changes

Merge branch 'main' into fix-details-table-nullable-value

00ddd0b

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

nvazquez merged commit 87ddc76 into apache:main Aug 14, 2021

weizhouapache added a commit to shapeblue/cloudstack that referenced this pull request Aug 31, 2021

server: fix another regression of apache#4819 and apache#5274

542ec42

shwstppr mentioned this pull request Sep 9, 2021

server: fix addCluster for vmware, others #5421

Merged

12 tasks

db: make *_details.value non-nullable #5274

db: make *_details.value non-nullable #5274

Uh oh!

Conversation

shwstppr commented Aug 4, 2021 • 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!

weizhouapache commented Aug 4, 2021

Uh oh!

blueorangutan commented Aug 4, 2021

Uh oh!

blueorangutan commented Aug 4, 2021

Uh oh!

weizhouapache commented Aug 4, 2021

Uh oh!

blueorangutan commented Aug 4, 2021

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Aug 4, 2021

Uh oh!

nvazquez commented Aug 7, 2021

Uh oh!

blueorangutan commented Aug 7, 2021

Uh oh!

blueorangutan commented Aug 7, 2021

Uh oh!

shwstppr commented Aug 8, 2021

Uh oh!

blueorangutan commented Aug 8, 2021

Uh oh!

blueorangutan commented Aug 9, 2021

Uh oh!

rohityadavcloud left a comment

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Aug 9, 2021

Uh oh!

blueorangutan commented Aug 9, 2021

Uh oh!

blueorangutan commented Aug 10, 2021

Uh oh!

nvazquez commented Aug 12, 2021

Uh oh!

GabrielBrascher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueorangutan commented Aug 13, 2021

Uh oh!

blueorangutan commented Aug 13, 2021

Uh oh!

GabrielBrascher commented Aug 13, 2021

Uh oh!

nvazquez commented Aug 13, 2021

Uh oh!

blueorangutan commented Aug 13, 2021

Uh oh!

blueorangutan commented Aug 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shwstppr commented Aug 4, 2021 •

edited

Loading

GabrielBrascher left a comment •

edited

Loading

shwstppr commented Aug 13, 2021 •

edited

Loading