Skip to content

Fix ConfigurationVO load exception after schema change #10485

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix ConfigurationVO load exception after schema change #10485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
af6d9f1
Look up only selected fields in getConfigStringValueInternal()
abh1sar Feb 28, 2025
8ab6142
Merge branch 'main' into upgrade-config
abh1sar Mar 4, 2025
adf411a
fix column cache issue
shwstppr Mar 5, 2025
b6873ea
Revert commit for af6d9f1e0c669794b48a3d05dfda98fcae549cd9 : Look up …
abh1sar Mar 7, 2025
e56109a
Merge branch 'upgrade-config' of https://github.com/shapeblue/cloudst…
abh1sar Mar 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade42010to42100.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,19 @@
// under the License.
package com.cloud.upgrade.dao;

import com.cloud.upgrade.SystemVmTemplateRegistration;
import com.cloud.utils.db.TransactionLegacy;
import com.cloud.utils.exception.CloudRuntimeException;

import java.io.InputStream;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.util.List;

import org.apache.cloudstack.framework.config.ConfigKey;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
import org.apache.cloudstack.framework.config.dao.ConfigurationDaoImpl;

import com.cloud.upgrade.SystemVmTemplateRegistration;
import com.cloud.utils.db.TransactionLegacy;
import com.cloud.utils.exception.CloudRuntimeException;

public class Upgrade42010to42100 extends DbUpgradeAbstractImpl implements DbUpgrade, DbUpgradeSystemVmTemplate {
private SystemVmTemplateRegistration systemVmTemplateRegistration;
Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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");
        }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator Author

@abh1sar abh1sar Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @abh1sar for checking. My bad.

}

protected void migrateExistingConfigurationScopeValues(Connection conn) {
Expand Down
1 change: 0 additions & 1 deletion ...config/src/test/java/org/apache/cloudstack/framework/config/impl/ConfigDepotImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ private void runTestGetConfigStringValueExpiry(long wait, int configDBRetrieval)
String result = configDepotImpl.getConfigStringValue(key, ConfigKey.Scope.Global, null);
Assert.assertEquals(value, result);
Mockito.verify(_configDao, Mockito.times(configDBRetrieval)).findById(key);

}

@Test
Expand Down
2 changes: 2 additions & 0 deletions framework/db/src/main/java/com/cloud/utils/db/GenericDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,4 +311,6 @@
Integer countAll();

List<T> findByUuids(String... uuidArray);

default void markForColumnsRefresh() {}

Check warning on line 315 in framework/db/src/main/java/com/cloud/utils/db/GenericDao.java

View check run for this annotation

Codecov / codecov/patch

framework/db/src/main/java/com/cloud/utils/db/GenericDao.java#L315 

Added line #L315 was not covered by tests
}
20 changes: 20 additions & 0 deletions framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,11 @@
import java.util.Date;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.TimeZone;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -129,8 +131,11 @@
protected final static TimeZone s_gmtTimeZone = TimeZone.getTimeZone("GMT");

protected final static Map<Class<?>, GenericDao<?, ? extends Serializable>> s_daoMaps = new ConcurrentHashMap<Class<?>, GenericDao<?, ? extends Serializable>>(71);

private final static Set<String> tableWithColumnsNeedRefresh = new HashSet<>();
private final ConversionSupport _conversionSupport;


protected Class<T> _entityBeanType;
protected String _table;

Expand Down Expand Up @@ -1914,6 +1919,7 @@

@DB()
protected void toEntityBean(final ResultSet result, final T entity) throws SQLException {
refreshColumnsIfNeeded();

Check warning on line 1922 in framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java

View check run for this annotation

Codecov / codecov/patch

framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java#L1922 

Added line #L1922 was not covered by tests
ResultSetMetaData meta = result.getMetaData();
for (int index = 1, max = meta.getColumnCount(); index <= max; index++) {
setField(entity, result, meta, index);
Expand Down Expand Up @@ -2481,4 +2487,18 @@
public SumCount() {
}
}

@Override
public void markForColumnsRefresh() {

Check warning on line 2492 in framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java

View check run for this annotation

Codecov / codecov/patch

framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java#L2492 

Added line #L2492 was not covered by tests
tableWithColumnsNeedRefresh.add(_table);
}

private void refreshColumnsIfNeeded() {

Check warning on line 2496 in framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java

View check run for this annotation

Codecov / codecov/patch

framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java#L2496 

Added line #L2496 was not covered by tests
if (!tableWithColumnsNeedRefresh.contains(_table)) {
return;

Check warning on line 2498 in framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java

View check run for this annotation

Codecov / codecov/patch

framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java#L2498 

Added line #L2498 was not covered by tests
}
final SqlGenerator generator = new SqlGenerator(_entityBeanType);
_allColumns = generator.getAllColumns();
tableWithColumnsNeedRefresh.remove(_table);
}

Check warning on line 2503 in framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java

View check run for this annotation

Codecov / codecov/patch

framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java#L2500-L2503 

Added lines #L2500 - L2503 were not covered by tests
}
Loading