config cleanup - #13884
Conversation
boring-cyborg
Bot
added
component:api
component:kvm
component:management-server
component:networking
component:orchestration
component:primary-storage
component:projects
component:quota
component:secondary-storage
component:security-group
component:ssvm
component:storage
component:usage-server
labels
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13884 +/- ##
=========================================
Coverage 19.78% 19.79%
- Complexity 19998 20018 +20
=========================================
Files 6371 6387 +16
Lines 575899 575896 -3
Branches 70495 70475 -20
=========================================
+ Hits 113967 114000 +33
+ Misses 449501 449462 -39
- Partials 12431 12434 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.69% |
| Branch coverage | 18.92% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18911 |
|
[SF] Trillian Build Failed (tid-16787) |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
[SF] Trillian Build Failed (tid-16808) |
|
[SF] Trillian Build Failed (tid-16861) |
|
[SF] Trillian test result (tid-16866)
|
|
@nvazquez @vladimirpetrov , this is the one I spoke about that needs to go in early in the cycle. cc @weizhouapache @winterhazel |
There was a problem hiding this comment.
🔵 Needs a closer look
The change set refactors configuration access across many subsystems, so correctness depends on broad regression testing and careful review of the new ConfigKey definitions and registrations.
Review details
- Files reviewed: 166/166 changed files
- Comments generated: 6
- Review effort level: Lite
| ConfigKey<Integer> VmDiskThrottlingBytesReadRate = new ConfigKey<>("Advanced", Integer.class, "vm.disk.throttling.bytes_read_rate", "0", | ||
| "Default disk I/O read rate in bytes per second allowed in User vm's disk.", true); | ||
|
|
||
| ConfigKey<Integer> VmDiskThrottlingBytesWriteRate = new ConfigKey<>("Advanced", Integer.class, "vm.disk.throttling.bytes_write_rate", "0", | ||
| "Default disk I/O writerate in bytes per second allowed in User vm's disk.", true); |
| ConfigKey<String> BaremetalIpmiRetryTimes = new ConfigKey<>("Advanced", String.class, "baremetal.ipmi.fail.retry", "5", | ||
| "ipmi interface will be temporary out of order after power operations(e.g. cycle, on), it leads following commands fail immediately. The value specifies retry times before accounting it as real failure", true); |
| ConfigKey<String> DefaultExternalFirewallCapacity = new ConfigKey<>("Advanced", String.class, "external.firewall.default.capacity", "50", | ||
| "default number of networks permitted per external load firewall device", true); |
| ConfigKey<String> DefaultExternalLoadBalancerCapacity = new ConfigKey<>("Advanced", String.class, "external.lb.default.capacity", "50", | ||
| "default number of networks permitted per external load balancer device", true); |
| "After starting provision process, periodcially echo security agent installed in the template. Treat provisioning as success only if echo successfully", true); | ||
|
|
||
| ConfigKey<String> BaremetalIpmiLanInterface = new ConfigKey<>("Advanced", String.class, "baremetal.ipmi.lan.interface", "default", | ||
| "option specified in -I option of impitool. candidates are: open/bmc/lipmi/lan/lanplus/free/imb, see ipmitool man page for details. default value 'default' means using default option of ipmitool", true); |
| "The mount point on the Management Server for Secondary Storage.", true); | ||
|
|
||
| ConfigKey<Boolean> SystemVMAutoReserveCapacity = new ConfigKey<>("Advanced", Boolean.class, "system.vm.auto.reserve.capacity", "true", | ||
| "Indicates whether or not to automatically reserver system VM standby capacity.", true); |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed correctness issues (e.g., a missing AgentManager import causing a compilation failure) and at least one behavior-affecting config read change that should be reconciled before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
server/src/main/java/com/cloud/storage/secondary/SecondaryStorageVmManager.java:60
- Typo in the config description string: "reserver" should be "reserve".
- Files reviewed: 166/166 changed files
- Comments generated: 2
- Review effort level: Lite
| (srcdata.getType() == DataObjectType.VOLUME && destData.getType() == DataObjectType.VOLUME)) { | ||
|
|
||
| int nMaxExecutionMinutes = NumbersUtil.parseInt(configDao.getValue(Config.SecStorageCmdExecutionTimeMax.key()), 30); | ||
| int nMaxExecutionMinutes = AgentManager.SecStorageCmdExecutionTimeMax.value(); |
| String execTime = UsageService.UsageStatsJobExecTime.value(); | ||
| Integer aggregationRange = UsageService.UsageStatsJobAggregationRange.value(); | ||
| String execTimeZone = UsageService.UsageExecutionTimezone.value(); | ||
| String aggregationTimeZone = UsageService.UsageAggregationTimezone.value(); | ||
| Integer sanityCheckInterval = UsageService.UsageSanityCheckInterval.value(); |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19070 |
|
Description
This PR tries to get rid of the Config enum or the old mechanism to define configuration items, which is long overdue. The change should be transparent, but is huge so good regression testing is required. And merg early in the release cycle is recommended.
Commits are being done in batches for review convenience. They could have been done in separate PRs but in the end the changes are rather trivial.
Fixes: #10752
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?