Fix ConfigurationVO load exception after schema change #10485

abh1sar · 2025-02-28T18:11:40Z

Description

This PR fixes #10480

The configuration table schema was changed in PR #10300
But it causes problem if the ConfigurationVO class structure was cached with the old fields.

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)
build/CI
test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov · 2025-02-28T18:19:48Z

Codecov Report

Attention: Patch coverage is 33.33333% with 10 lines in your changes missing coverage. Please review.

Project coverage is 16.26%. Comparing base (eab37ec) to head (e56109a).
Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/main/java/com/cloud/utils/db/GenericDaoBase.java	25.00%	9 Missing ⚠️
...b/src/main/java/com/cloud/utils/db/GenericDao.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #10485      +/-   ##
============================================
+ Coverage     16.15%   16.26%   +0.10%     
- Complexity    13274    13387     +113     
============================================
  Files          5666     5675       +9     
  Lines        498078   498943     +865     
  Branches      60267    60337      +70     
============================================
+ Hits          80481    81162     +681     
- Misses       408584   408740     +156     
- Partials       9013     9041      +28

Flag	Coverage Δ
uitests	`3.99% <ø> (-0.01%)`	⬇️
unittests	`17.12% <33.33%> (+0.11%)`	⬆️

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.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

abh1sar · 2025-03-03T08:30:02Z

@blueorangutan package

blueorangutan · 2025-03-03T08:32:04Z

@abh1sar 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 · 2025-03-03T08:33:34Z

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

abh1sar · 2025-03-03T11:28:17Z

@blueorangutan package

blueorangutan · 2025-03-03T11:30:04Z

@abh1sar 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 · 2025-03-03T12:31:53Z

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

abh1sar · 2025-03-03T12:52:05Z

@blueorangutan test

blueorangutan · 2025-03-03T12:54:04Z

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

blueorangutan · 2025-03-03T13:50:24Z

[SF] Trillian Build Failed (tid-12539)

rohityadavcloud · 2025-03-04T06:09:08Z

@blueorangutan test

blueorangutan · 2025-03-04T06:10:03Z

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

blueorangutan · 2025-03-04T07:12:28Z

[SF] Trillian Build Failed (tid-12546)

abh1sar · 2025-03-04T08:22:36Z

@blueorangutan package

blueorangutan · 2025-03-04T08:24:03Z

@abh1sar 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 · 2025-03-04T08:24:05Z

[SF] Trillian Build Failed (tid-12553)

blueorangutan · 2025-03-04T09:27:00Z

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

blueorangutan · 2025-03-04T10:29:47Z

[SF] Trillian Build Failed (tid-12555)

JoaoJandre · 2025-03-04T15:05:24Z

Instead of changing how we get the value, shouldn't we normalize the database data so that it works with the current way of getting the values?

Otherwise, if someone in the future creates a method to get the value the old way and only tests on a new install, it might introduce a bug for people running old installs.

Signed-off-by: Abhishek Kumar <[email protected]>

…StringValueInternal()

into upgrade-config

abh1sar · 2025-03-11T13:15:24Z

@JoaoJandre We identified the issue with how BackupDaoImpl class caches the columns of the table. Even though both configurations table and ConfigurationVO code has the new schema, the ConfigurationsDao._allColumns field still had the older schema from before upgrade. That's why after management server restart ConfigurationsDaoImpl_allColumns was getting regenerated with the correct fields.
I have reverted the older commit and added the commit to regenerate ConfigurationsDaoImpl._allColumns when the Configurations table schema is changed.

rohityadavcloud · 2025-03-13T10:30:36Z

@blueorangutan package

blueorangutan · 2025-03-13T10:32:04Z

@rohityadavcloud 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 · 2025-03-13T11:37:15Z

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

DaanHoogland · 2025-03-13T13:47:17Z

@blueorangutan test

blueorangutan · 2025-03-13T13:48:03Z

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

blueorangutan · 2025-03-13T13:50:44Z

[SF] Trillian Build Failed (tid-12681)

JoaoJandre · 2025-03-13T14:34:54Z

engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade42010to42100.java

@@ -98,6 +100,8 @@ protected void migrateConfigurationScopeToBitmask(Connection conn) {
        migrateExistingConfigurationScopeValues(conn);
        DbUpgradeUtils.dropTableColumnsIfExist(conn, "configuration", List.of("scope"));
        DbUpgradeUtils.changeTableColumnIfNotExist(conn, "configuration", "new_scope", "scope", "BIGINT NOT NULL DEFAULT 0 COMMENT 'Bitmask for scope(s) of this parameter'");
+        ConfigurationDao dao = new ConfigurationDaoImpl();
+        dao.markForColumnsRefresh();


Instead of marking columns for refresh later, why not do it on the upgrade?

@JoaoJandre cc @abh1sar there could be a better way to refresh columns but I wasn't able to do it after the upgrade from this class. At runtime, we have a different ConfigurationDaoImpl instance which was created as the bean and needs columns refresh. I was not being able to access it from here directly. Maybe something like this can help,

try { ConfigurationDao dao = ComponentContext.getDelegateComponentOfType(ConfigurationDao.class); dao.refreshColumns(); } catch (NoSuchBeanDefinitionException ignored) { logger.debug("No ConfigurationDao bean found"); }

@shwstppr @JoaoJandre
sorry for the delay. I tested the above, but ComponentContext only keeps track of PluggableService class.
I was not able to access ConfigurationDaoImpl any other way as well.
Any other ideas are welcome.

@abh1sar thanks for checking. No ideas at the moment
Only improvement suggestion I've at the moment is making markForColumnsRefresh static so we don't need to create new instance and as it only toggles a static member of the class

@shwstppr markForColumnRefresh accesses the non-static _table field in GenericDaoBase.

Thanks @abh1sar for checking. My bad.

JoaoJandre · 2025-04-08T12:33:55Z

@abh1sar could you explain how does the class structure get cached? I think I may be missing something, as on upgrade, the MGMT server will be down, after it is started which cache could it have? How can we reproduce this error?

shwstppr

Code LGTM but others will have a better say

shwstppr

@abh1sar based on your testing and discussion this would need changes.

I was able to not see the error by evicting old connections when HIkariCP is used for pooling.

diff --git a/engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java b/engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java
index 1e3b3a7e5e..6ea7242f83 100644
--- a/engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java
+++ b/engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java
@@ -379,6 +379,7 @@ public class DatabaseUpgradeChecker implements SystemIntegrityChecker {
         } finally {
             txn.close();
         }
+        TransactionLegacy.refreshConnections(TransactionLegacy.CLOUD_DB);
         return version;
     }
 
diff --git a/framework/db/src/main/java/com/cloud/utils/db/TransactionLegacy.java b/framework/db/src/main/java/com/cloud/utils/db/TransactionLegacy.java
index 88af397c06..18a90749e4 100644
--- a/framework/db/src/main/java/com/cloud/utils/db/TransactionLegacy.java
+++ b/framework/db/src/main/java/com/cloud/utils/db/TransactionLegacy.java
@@ -605,6 +605,15 @@ public class TransactionLegacy implements Closeable {
         return _conn;
     }
 
+    public static void refreshConnections(final short dbId) {
+        if (dbId != CLOUD_DB) {
+            return;
+        }
+        if (s_ds instanceof HikariDataSource) {
+            ((HikariDataSource)s_ds).getHikariPoolMXBean().softEvictConnections();
+        }
+    }
+
     protected boolean takeOver(final String name, final boolean create) {
         if (_stack.size() != 0) {
             if (!create) {

I'm not sure about DBCP. I've not reproduced the issue there yet and based on my little research it may need closing the datasource.
Also, we need to remove that markForColumnsRefresh logic as that doesn't seem to be the problem.

Look up only selected fields in getConfigStringValueInternal()

af6d9f1

boring-cyborg bot added the component:vpc label Feb 28, 2025

abh1sar changed the title ~~Fix ConfigurationVO load exception on fresh install~~ Fix ConfigurationVO load exception after schema change Feb 28, 2025

DaanHoogland added this to the 4.21.0 milestone Mar 3, 2025

Merge branch 'main' into upgrade-config

8ab6142

shwstppr and others added 3 commits March 11, 2025 18:29

fix column cache issue

adf411a

Signed-off-by: Abhishek Kumar <[email protected]>

Revert commit for af6d9f1 : Look up only selected fields in getConfig…

b6873ea

…StringValueInternal()

Merge branch 'upgrade-config' of https://github.com/shapeblue/cloudstack

e56109a

into upgrade-config

boring-cyborg bot added the component:database label Mar 11, 2025

abh1sar self-assigned this Mar 11, 2025

rohityadavcloud requested review from shwstppr, harikrishna-patnala, vishesh92 and vladimirpetrov March 13, 2025 10:30

rohityadavcloud assigned harikrishna-patnala and vladimirpetrov and unassigned abh1sar Mar 13, 2025

abh1sar requested a review from nvazquez March 13, 2025 12:07

JoaoJandre reviewed Mar 13, 2025

View reviewed changes

shwstppr approved these changes Apr 8, 2025

View reviewed changes

shwstppr requested changes Apr 10, 2025

View reviewed changes

Fix ConfigurationVO load exception after schema change #10485

Are you sure you want to change the base?

Fix ConfigurationVO load exception after schema change #10485

Conversation

abh1sar commented Feb 28, 2025

Description

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?

codecov bot commented Feb 28, 2025 • edited Loading

Codecov Report

abh1sar commented Mar 3, 2025

blueorangutan commented Mar 3, 2025

blueorangutan commented Mar 3, 2025

abh1sar commented Mar 3, 2025

blueorangutan commented Mar 3, 2025

blueorangutan commented Mar 3, 2025

abh1sar commented Mar 3, 2025

blueorangutan commented Mar 3, 2025

blueorangutan commented Mar 3, 2025

rohityadavcloud commented Mar 4, 2025

blueorangutan commented Mar 4, 2025

blueorangutan commented Mar 4, 2025

abh1sar commented Mar 4, 2025

blueorangutan commented Mar 4, 2025

blueorangutan commented Mar 4, 2025

blueorangutan commented Mar 4, 2025

blueorangutan commented Mar 4, 2025

JoaoJandre commented Mar 4, 2025

abh1sar commented Mar 11, 2025

rohityadavcloud commented Mar 13, 2025

blueorangutan commented Mar 13, 2025

blueorangutan commented Mar 13, 2025

DaanHoogland commented Mar 13, 2025

blueorangutan commented Mar 13, 2025

blueorangutan commented Mar 13, 2025

JoaoJandre Mar 13, 2025

Choose a reason for hiding this comment

shwstppr Mar 18, 2025

Choose a reason for hiding this comment

abh1sar Apr 8, 2025

Choose a reason for hiding this comment

shwstppr Apr 8, 2025

Choose a reason for hiding this comment

abh1sar Apr 8, 2025 • edited Loading

Choose a reason for hiding this comment

shwstppr Apr 8, 2025

Choose a reason for hiding this comment

JoaoJandre commented Apr 8, 2025

shwstppr left a comment

Choose a reason for hiding this comment

shwstppr left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2025 •

edited

Loading

abh1sar Apr 8, 2025 •

edited

Loading