Conversation
|
@blueorangutan package |
|
@shwstppr 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14033 +/- ##
============================================
+ Coverage 19.78% 19.91% +0.13%
- Complexity 19992 20201 +209
============================================
Files 6371 6373 +2
Lines 575903 577230 +1327
Branches 70496 70696 +200
============================================
+ Hits 113945 114980 +1035
- Misses 449530 449683 +153
- Partials 12428 12567 +139
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:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19055 |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds the database upgrade plumbing for a 4.23.0.0 → 4.24.0.0 engine schema upgrade.
Changes:
- Adds new (currently comment-only) schema upgrade and cleanup SQL scripts for 4.23.0.0 → 4.24.0.0.
- Introduces
Upgrade42300to42400Java upgrade class to define the version range and target version. - Registers the new upgrade step in
DatabaseUpgradeChecker’s upgrade path chain.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| engine/schema/src/main/resources/META-INF/db/schema-42300to42400.sql | Adds the 4.23.0.0 → 4.24.0.0 schema upgrade script file (currently header/comments only). |
| engine/schema/src/main/resources/META-INF/db/schema-42300to42400-cleanup.sql | Adds the corresponding cleanup script file (currently header/comments only). |
| engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade42300to42400.java | Adds the Java upgrade step declaring version range and upgraded version. |
| engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java | Wires the new upgrade step into the upgrader chain. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
let’s do #14033 instead
|
maybe this can go on top of that one |
Adds a 4.23.0 to 24.0.0 upgrade path. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
bf8ced5 to
ccc870c
Compare
shwstppr
changed the title
| .next("4.21.0.0", new Upgrade42100to42200()) | ||
| .next("4.22.0.0", new Upgrade42200to42210()) | ||
| .next("4.22.1.0", new Upgrade42210to42300()) | ||
| .next("4.23.0.0", new Upgrade42300to2400()) |
There was a problem hiding this comment.
when we reach 30, we might get some clashes with the old 3.x version. I think we should disgard thos by then.
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
|
@blueorangutan package |
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
|
@blueorangutan package |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-16998) |
|
[SF] Trillian Build Failed (tid-17000) |
|
[SF] Trillian Build Failed (tid-35) |
|
[SF] Trillian Build Failed (tid-17001) |
|
[SF] Trillian Build Failed (tid-17002) |
|
[SF] Trillian Build Failed (tid-17003) |
|
[SF] Trillian Build Failed (tid-17006) |
|
[SF] Trillian Build Failed (tid-17005) |
|
@blueorangutan test |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-36)
|
|
@weizhouapache maybe @nvazquez or @RosiKyu can advise about that. |
|
Hi @weizhouapache - @RosiKyu is performing additional manual tests and if no further issues it should be good to merge, smoke test failures seem intermittent - no issues observed with system VMs on created envs |
|
Tested this on a 24.0.0 build: OL9 Management server, 2 KVM hosts, Advanced Networking, single MS. The database was seeded at 4.0.0 and ran the full upgrade chain, so the new What I checked
Issue 1: export-templates.sh has a syntax error and does not run at allThe new cutover if/else in Expected result: the script runs and derives the template filename prefix from metadata.ini. Actual result: it aborts immediately with a syntax error, for any input. This is not only in the diff, it is in the packaged RPM, and since it ships in Issue 2: a 4 part minreq.sysvmtemplate.version breaks listRouters and all VR operations
Steps: set Expected result: either the value is rejected when it is set, or the version check treats it as "router needs upgrading". Actual result: the value is accepted with no error, then listRouters fails and every VR operation fails. listRouters: createEgressFirewallRule, job fails with resultCode 530: Still to verifyI am building a second environment for the upgrade side: a populated 4.23.0.0 database, mixed hypervisors including VMware, and two management servers. That will cover the things this environment could not: upgrade of a database with real data in it, the clustered upgrade guard with both MS up, the VMware path, and what |
There was a problem hiding this comment.
🟡 Changes recommended
Template-version validation gaps remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
engine/schema/templateConfig.sh:38
- This branch only checks whether a fourth component exists; it never validates the complete stripped version. For example,
getTemplateVersion "24.0"succeeds and exportsFULL_VERSION=24.0., while a legacy value such as4.23.0.1.2silently drops the fifth component, even thoughCloudStackVersion.parserejects both forms. Validate the input as exactly three or four numeric components before extracting fields so the build cannot emit unparseable metadata.
utils/src/main/java/org/apache/cloudstack/utils/CloudStackVersion.java:263
- The updated parseability check rejects four-component versions at major 24 and above, but the
trimRouterVersionJavadoc immediately above still documents every four-component value as valid. Please qualify those formats by the legacy-major rule and document the post-cutovermajor.minor.securityform so callers are not given an incorrect contract.
if (tokens.length >= 3 && FULL_VERSION_FORMAT.matcher(tokens[2]).matches() && isParseableVersion(tokens[2])) {
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
| TEMPLATE_MAJOR_VERSION="$(cut -d'.' -f1 <<<"$TEMPLATE_VERSION")" | ||
| if [[ "$TEMPLATE_MAJOR_VERSION" -ge "$NEW_VERSIONING_CUTOVER_MAJOR_VERSION" ]]; then | ||
| # New versioning (major.minor.security): the generated template files use the full version as-is. | ||
| VERSION="${TEMPLATE_VERSION}" | ||
| else | ||
| # Legacy versioning drops the trailing security component only for four-component metadata; | ||
| # a valid three-component major.minor.patch value is already the filename version. | ||
| if [[ "$TEMPLATE_VERSION" == *.*.*.* ]]; then | ||
| VERSION="${TEMPLATE_VERSION%.*}" | ||
| else | ||
| VERSION="${TEMPLATE_VERSION}" | ||
| fi | ||
| fi |
|
[SF] Trillian test result (tid-17008)
|
Description
Adds the 4.23.0.0 to 24.0.0 database upgrade path for CloudStack's versioning cutover (management server versions drop the leading 4. and move to a 3-component major.minor.security scheme from 24.0.0 onwards; legacy 4-component major.minor.patch.security versions remain supported below major 24 - Reference PR: #14041).
Includes:
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?