Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

Merged
nvazquez merged 2 commits intoapache:mainapache/cloudstack:mainfrom
shapeblue:fix-details-table-nullable-valueshapeblue/cloudstack:fix-details-table-nullable-valueCopy head branch name to clipboard
Aug 14, 2021
Merged

db: make *_details.value non-nullable#5274
nvazquez merged 2 commits intoapache:mainapache/cloudstack:mainfrom
shapeblue:fix-details-table-nullable-valueshapeblue/cloudstack:fix-details-table-nullable-valueCopy head branch name to clipboard

Conversation

@shwstppr
Copy link
Contributor

@shwstppr shwstppr commented Aug 4, 2021 •
edited
Loading

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

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

@shwstppr
db: make *_details.value non-nullable
4d5a613 
Fixes apache#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.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@shwstppr shwstppr mentioned this pull request Aug 4, 2021
Closed
@weizhouapache
Copy link
Contributor

weizhouapache commented Aug 4, 2021

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Aug 4, 2021

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

@blueorangutan
Copy link

blueorangutan commented Aug 4, 2021

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

@weizhouapache
Copy link
Contributor

weizhouapache commented Aug 4, 2021

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Aug 4, 2021

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

nvazquez
nvazquez approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

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

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

@nvazquez nvazquez added this to the 4.16.0.0 milestone Aug 4, 2021
@blueorangutan
Copy link

blueorangutan commented Aug 4, 2021

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

@shwstppr shwstppr marked this pull request as ready for review August 6, 2021 04:40
@nvazquez nvazquez closed this Aug 7, 2021
@nvazquez nvazquez reopened this Aug 7, 2021
@nvazquez
Copy link
Contributor

nvazquez commented Aug 7, 2021

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Aug 7, 2021

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

@blueorangutan
Copy link

blueorangutan commented Aug 7, 2021

Trillian Build Failed (tid-1526)

@shwstppr
Copy link
Contributor Author

shwstppr commented Aug 8, 2021

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Aug 8, 2021

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

@blueorangutan
Copy link

blueorangutan commented Aug 9, 2021

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

yadvr
yadvr approved these changes Aug 9, 2021
View reviewed changes
Copy link
Member

@yadvr yadvr left a comment

Choose a reason for hiding this comment

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

LGTM - needs testing

@yadvr
Copy link
Member

yadvr commented Aug 9, 2021

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Aug 9, 2021

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

@blueorangutan
Copy link

blueorangutan commented Aug 10, 2021

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
Copy link
Contributor

nvazquez commented Aug 12, 2021

@shwstppr can you please solve the conflict?

GabrielBrascher
GabrielBrascher approved these changes Aug 12, 2021
View reviewed changes
Copy link
Member

@GabrielBrascher GabrielBrascher left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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.

@shwstppr
Merge branch 'main' into fix-details-table-nullable-value
00ddd0b 
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@shwstppr
Copy link
Contributor Author

shwstppr commented Aug 13, 2021 •
edited
Loading

@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
Copy link

blueorangutan commented Aug 13, 2021

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

@blueorangutan
Copy link

blueorangutan commented Aug 13, 2021

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

@GabrielBrascher
Copy link
Member

GabrielBrascher commented Aug 13, 2021

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

@nvazquez
Copy link
Contributor

nvazquez commented Aug 13, 2021

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Aug 13, 2021

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

@blueorangutan
Copy link

blueorangutan commented Aug 14, 2021

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

@nvazquez 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
@weizhouapache
server: fix another regression of apache#4819 and apache#5274
542ec42
nvazquez pushed a commit that referenced this pull request Sep 8, 2021
@weizhouapache
server: fix reset sshkey is broken in master/4.16 (#5390)
b13930f 
* 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 shwstppr mentioned this pull request Sep 9, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes

@DaanHoogland DaanHoogland DaanHoogland approved these changes

@GabrielBrascher GabrielBrascher GabrielBrascher approved these changes

@nvazquez nvazquez nvazquez approved these changes

Assignees

No one assigned

Labels

component:database type:cleanup

Projects

None yet

Milestone

4.16.0.0

Development

Successfully merging this pull request may close these issues.

Eliminate *_details table inconsistency

7 participants

@shwstppr @weizhouapache @blueorangutan @nvazquez @yadvr @GabrielBrascher @DaanHoogland
Morty Proxy This is a proxified and sanitized view of the page, visit original site.