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 <[email protected]>
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 <[email protected]>
|
@blueorangutan package |
Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
|
@blueorangutan package |
|
[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)
|
|
Upgrade testing is done. Built a second environment for it: populated 4.23.0.0, Ubuntu 24.04, 2 management servers, three hypervisors with 2 hosts each (VMware ESXi 8.0.3, XenServer 8.3, KVM). Populated before upgrading with 3 isolated networks each with a running VM on its own hypervisor, a standard VPC, a redundant VPC (so a redundant VR pair), 6 routers Running, 5 VMs and a snapshot. Upgraded to Upgrade results
CloudStack refuses to upgrade the database while another management server is still up, so I tested that first: mgmt2 left running on 4.23, mgmt1 started on 24.0.0. It refused and the database stayed at 4.23.0.0. One note: that message only goes to The two issuesIssue 1, export-templates.sh: fixed by 569417c and verified. I patched the file by hand on the management server and both KVM hosts. Issue 2, minreq.sysvmtemplate.version: still open. It reproduces the same way on the upgraded environment, so it is not environment specific. However, the overall error message in case of a badly formatted input is quite descriptive Separate problem, not caused by this PRFlagging here because it will stop anyone upgrading to 24.0.0 on Ubuntu. After the upgrade the management server starts and listens on 8080 but serves HTTP 503 permanently. Reproduced on both management servers. The management server ends up with two copies of the same BouncyCastle class on its classpath, one complete and one old and incomplete, and Java loads whichever it finds first. The order is not defined, so which one wins is luck. On both Ubuntu management servers it picked the incomplete one and the server never finished starting. My el9 environment has both copies too but picks the good one, so this is not Ubuntu specific. Forcing the good copy first on the classpath makes it start normally in about 75 seconds. Raising this as a separate issue with the details. |
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?