Refactor type and range validation in configuration update process #9107

winterhazel · 2024-05-22T11:58:16Z

Description

The configuration update process mixes type validation with range validation in com.cloud.configuration.ConfigurationManagerImpl#validateConfigurationValue. To improve this process, this PR splits these validations into two methods (validateValueType and validateValueRange) and cleans up the code.

Furthermore, this PR also fixes an issue that allows non-numeric values to be inserted into Double configurations, such as zone.virtualnetwork.ipv6subnet.capacity.notificationthreshold.

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

Bug Severity

How Has This Been Tested?

I manually verified the behavior of trying to update configurations of the following types, and compared them with the previous version to ensure the result was expected:

String - quota.currency.symbol and alert.email.addresses (that is, one that does not have null as a valid value and another that does);
Boolean - admin.is.allowed.to.deploy.anywhere;
Integer - storage.cleanup.interval. (the validations for this type also apply to Short and Long);
Float - cpu.overprovisioning.factor (also applies to Double).

Number	Test	Result	Expected result (Y/N)?
1	Value `"null"` in a `String` that does not have `null` as a valid value	The value is updated in the database, but the default value is used	Y
2	Value `"1"` in a `String` that does not have `null` as a valid value	Success	Y
3	Value `"1.1"` in a `String` that does not have `null` as a valid value	Success	Y
4	Value `"test"` in a `String` that does not have `null` as a valid value	Success	Y
5	Value `"null"` in a `String` that has `null` as a valid value	Success	Y
6	Value `"1"` in a `String` that has `null` as a valid value	Success	Y
7	Value `"1.1"` in a `String` that has `null` as a valid value	Success	Y
8	Value `"test"` in a `String` that has `null` as a valid value	Success	Y
9	Value `"null"` in a `Boolean`	Exception	Y
10	Value `"1"` in a `Boolean`	Exception	Y
11	Value `"1.1"` in a `Boolean`	Exception	Y
12	Value `"test"` in a `Boolean`	Exception	Y
13	Value `"true"` in a `Boolean`	Success	Y
14	Value `"false"` in a `Boolean`	Success	Y
15	Value `"fALse"` in a `Boolean`	Exception	Y
16	Value `"null"` in an `Integer`	Exception	Y
17	Value `"1"` in an `Integer`	Success	Y
18	Value `"1.1"` in an `Integer`	Exception	Y
19	Value `"test"` in an `Integer`	Exception	Y
20	Value `"null"` in a `Float`	Exception	Y
21	Value `"1"` in a `Float`	Success	Y
22	Value `"1.1"` in a `Float`	Success	Y
23	Value `"test"` in a `Float`	Exception	Y

winterhazel · 2024-05-22T12:03:53Z

@blueorangutan package

blueorangutan · 2024-05-22T12:06:03Z

@winterhazel a [SL] 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 · 2024-05-22T13:29:11Z

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

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

…gerImpl.java Co-authored-by: Henrique Sato <henriquesato2003@gmail.com>

codecov · 2024-06-14T19:01:05Z

Codecov Report

Attention: Patch coverage is 72.28916% with 23 lines in your changes missing coverage. Please review.

Project coverage is 15.58%. Comparing base (b155e3d) to head (ef851d8).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
.../cloud/configuration/ConfigurationManagerImpl.java	72.28%	13 Missing and 10 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9107      +/-   ##
============================================
+ Coverage     15.57%   15.58%   +0.01%     
- Complexity    12051    12075      +24     
============================================
  Files          5505     5505              
  Lines        482738   482725      -13     
  Branches      61341    62781    +1440     
============================================
+ Hits          75202    75251      +49     
+ Misses       399227   399166      -61     
+ Partials       8309     8308       -1

Flag	Coverage Δ
uitests	`4.17% <ø> (ø)`
unittests	`16.37% <72.28%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-06-20T08:19:36Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

winterhazel · 2024-06-27T12:44:12Z

@blueorangutan package

blueorangutan · 2024-06-27T12:46:03Z

@winterhazel a [SL] 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 · 2024-06-27T13:56:15Z

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

github-actions · 2024-06-29T02:12:47Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

winterhazel · 2024-07-01T12:25:34Z

@blueorangutan package

blueorangutan · 2024-07-01T12:26:03Z

@winterhazel a [SL] 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 · 2024-07-01T13:09:10Z

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

winterhazel · 2024-07-01T15:20:03Z

@blueorangutan package

blueorangutan · 2024-07-01T15:22:03Z

@winterhazel a [SL] 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 · 2024-07-01T16:22:21Z

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

winterhazel · 2024-07-25T11:55:33Z

@blueorangutan package

blueorangutan · 2024-07-25T11:56:04Z

@winterhazel a [SL] 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 · 2024-08-12T13:47:55Z

[SF] Trillian Build Failed (tid-11061)

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

hsato03

LGTM. didn't test

JoaoJandre · 2024-08-22T17:44:53Z

Hey @winterhazel

I did some testing, overall it looks good, but I've found one case that I think we should address in this PR.

Number	Config	Value	Result
1	storage.cleanup.interval	oasij	Error
2	storage.cleanup.interval	.	Error
3	storage.cleanup.interval	(	Error
4	storage.cleanup.interval	102938120381209381203981239128	Error
5	storage.cleanup.interval		Error
6	storage.cleanup.interval	""	Error
7	storage.cleanup.interval	86400	Ok
8	cpu.overprovisioning.factor	a	Error
9	cpu.overprovisioning.factor	^	Error
10	cpu.overprovisioning.factor	1.1.1	Error
11	cpu.overprovisioning.factor	10928091283012830123810293	OK
12	cpu.overprovisioning.factor	9999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999	OK, but this should not be
13	admin.is.allowed.to.deploy.anywhere	1	Error
14	admin.is.allowed.to.deploy.anywhere	0	Error
15	admin.is.allowed.to.deploy.anywhere	a	Error
16	admin.is.allowed.to.deploy.anywhere	FALSE	Error
17	admin.is.allowed.to.deploy.anywhere	TRUE	Error
18	admin.is.allowed.to.deploy.anywhere	true	OK
19	admin.is.allowed.to.deploy.anywhere	false	OK

Test 12 was sucessful, but the number informed is bigger than the maximum value of Floats and Doubles. Double.parseDouble() returns Infinity if the number being parsed would overflow. We should not allow this config value, we should return an error, like what is done in test 4.

winterhazel · 2024-08-22T19:46:53Z

Thanks for testing @JoaoJandre. I addressed the issue you pointed out.

winterhazel · 2024-08-22T19:47:13Z

@blueorangutan package

blueorangutan · 2024-08-22T19:48:03Z

@winterhazel a [SL] 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 · 2024-08-22T20:48:56Z

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

DaanHoogland · 2024-08-23T08:28:21Z

@blueorangutan test

blueorangutan · 2024-08-23T08:30:03Z

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

blueorangutan · 2024-08-23T23:59:54Z

[SF] Trillian test result (tid-11158)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 53213 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9107-t11158-kvm-ol8.zip
Smoke tests completed. 137 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_secure_vm_migration	`Error`	135.45	test_vm_life_cycle.py
test_01_secure_vm_migration	`Error`	135.45	test_vm_life_cycle.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	391.95	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	487.68	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	487.70	test_vpc_redundant.py

DaanHoogland · 2024-08-26T09:08:10Z

@blueorangutan test

blueorangutan · 2024-08-26T09:10:03Z

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

blueorangutan · 2024-08-26T09:13:26Z

[SF] Trillian Build Failed (tid-11181)

JoaoJandre

LGTM

DaanHoogland · 2024-08-26T12:49:12Z

@blueorangutan LLtest

blueorangutan · 2024-08-26T12:50:02Z

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

github-actions · 2024-09-04T08:40:31Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

winterhazel · 2024-09-04T12:55:46Z

@blueorangutan package

blueorangutan · 2024-09-04T12:58:04Z

@winterhazel a [SL] 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 · 2024-09-04T14:10:04Z

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

JoaoJandre · 2024-09-06T13:51:03Z

Merging based on approvals and manual testing #9107 (comment)

…pache#9107) * Refactor type and range validation in configuration update process * Update server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java Co-authored-by: Henrique Sato <henriquesato2003@gmail.com> * Remove duplicate imports * Fix tests * Address Joao's reviews --------- Co-authored-by: Henrique Sato <henriquesato2003@gmail.com>

Refactor type and range validation in configuration update process

91ca46c

winterhazel force-pushed the updateconfig-refactor branch from f2e3757 to 91ca46c Compare May 22, 2024 12:03

hsato03 reviewed Jun 14, 2024

View reviewed changes

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java Outdated Show resolved Hide resolved

Update server/src/main/java/com/cloud/configuration/ConfigurationMana…

dbcb349

…gerImpl.java Co-authored-by: Henrique Sato <henriquesato2003@gmail.com>

github-actions bot added the status:has-conflicts label Jun 20, 2024

Merge main

46e6be3

github-actions bot removed the status:has-conflicts label Jun 27, 2024

github-actions bot added the status:has-conflicts label Jun 29, 2024

sureshanaparti added this to the 4.20.0.0 milestone Jun 29, 2024

Merge main

e7a8fd6

github-actions bot removed the status:has-conflicts label Jul 1, 2024

winterhazel added 2 commits July 1, 2024 10:31

Remove duplicate imports

bc4e12d

Fix tests

229178a

winterhazel requested a review from hsato03 August 22, 2024 13:00

JoaoJandre reviewed Aug 22, 2024

View reviewed changes

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java Show resolved Hide resolved

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java Show resolved Hide resolved

hsato03 approved these changes Aug 22, 2024

View reviewed changes

winterhazel added 2 commits August 22, 2024 16:33

Address Joao's reviews

efd2c26

Merge branch 'main' into updateconfig-refactor

1cb6a99

JoaoJandre approved these changes Aug 26, 2024

View reviewed changes

github-actions bot added the status:has-conflicts label Sep 4, 2024

Merge main

ef851d8

github-actions bot removed the status:has-conflicts label Sep 4, 2024

JoaoJandre merged commit f156c4e into apache:main Sep 6, 2024

Refactor type and range validation in configuration update process #9107

Refactor type and range validation in configuration update process #9107

Uh oh!

Conversation

winterhazel commented May 22, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

How Has This Been Tested?

Uh oh!

winterhazel commented May 22, 2024

Uh oh!

blueorangutan commented May 22, 2024

Uh oh!

blueorangutan commented May 22, 2024

Uh oh!

Uh oh!

codecov bot commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jun 20, 2024

Uh oh!

winterhazel commented Jun 27, 2024

Uh oh!

blueorangutan commented Jun 27, 2024

Uh oh!

blueorangutan commented Jun 27, 2024

Uh oh!

github-actions bot commented Jun 29, 2024

Uh oh!

winterhazel commented Jul 1, 2024

Uh oh!

blueorangutan commented Jul 1, 2024

Uh oh!

blueorangutan commented Jul 1, 2024

Uh oh!

winterhazel commented Jul 1, 2024

Uh oh!

blueorangutan commented Jul 1, 2024

Uh oh!

blueorangutan commented Jul 1, 2024

Uh oh!

winterhazel commented Jul 25, 2024

Uh oh!

blueorangutan commented Jul 25, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

Uh oh!

Uh oh!

hsato03 left a comment

Choose a reason for hiding this comment

Uh oh!

JoaoJandre commented Aug 22, 2024

Uh oh!

winterhazel commented Aug 22, 2024

Uh oh!

winterhazel commented Aug 22, 2024

Uh oh!

blueorangutan commented Aug 22, 2024

Uh oh!

blueorangutan commented Aug 22, 2024

Uh oh!

DaanHoogland commented Aug 23, 2024

Uh oh!

blueorangutan commented Aug 23, 2024

Uh oh!

blueorangutan commented Aug 23, 2024

Uh oh!

DaanHoogland commented Aug 26, 2024

Uh oh!

blueorangutan commented Aug 26, 2024

Uh oh!

blueorangutan commented Aug 26, 2024

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 14, 2024 •

edited

Loading