Liron Aravot has uploaded a new change for review. Change subject: core: WIP: NPEs and wrong persisted information when reusing LUNs ......................................................................
core: WIP: NPEs and wrong persisted information when reusing LUNs Generally, the following issues caused that a created lun based storage domain existed on vdsm side while not having it's correct data in the DB which led to an NPE and wrong data shown to the user during execution of GetLunsByVgIdQuery. This patch fixes the following issues: 1. When removing a domain that used lun storage and there's a disk used by that lun - LUNs volume_group_id still contains the removed domain id which doesn't exist anymore. 2. When extending a domain to use an existing lun, the domain id isn't set to that lun which causes to NPE in GetLunsByVgIdQuery or wrong data provided to the user. 3. When creating a domain from an existing lun, the domain id isn't set to this lun which might causes to NPE in GetLunsByVgIdQuery or wrong data provided to the user. 4. When removing lun disk which isn't used as storage domain (and therefore - not used anymore), the lun remained in the DB. Change-Id: I38a0e3c68cb8bd80c2f78ee5aacfccc8c987a79e Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=875909 Signed-off-by: Liron Aravot <lara...@redhat.com> --- M backend/manager/dbscripts/storages_san_sp.sql M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddSANStorageDomainCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ExtendSANStorageDomainCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/LunDAO.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/LunDAODbFacadeImpl.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/LunDAOTest.java 8 files changed, 48 insertions(+), 3 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/29/9229/1 diff --git a/backend/manager/dbscripts/storages_san_sp.sql b/backend/manager/dbscripts/storages_san_sp.sql index dad6c9a..aeea92a 100644 --- a/backend/manager/dbscripts/storages_san_sp.sql +++ b/backend/manager/dbscripts/storages_san_sp.sql @@ -23,6 +23,19 @@ +Create or replace FUNCTION UpdateLUNsVolumeGroupId(v_LUN_id VARCHAR(50), +v_volume_group_id VARCHAR(50)) +RETURNS VOID + AS $procedure$ +BEGIN +UPDATE LUNs set volume_group_id = v_volume_group_id where LUN_id = v_LUN_ID; +END; $procedure$ +LANGUAGE plpgsql; + + + + + Create or replace FUNCTION DeleteLUN(v_LUN_id VARCHAR(50)) RETURNS VOID diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddSANStorageDomainCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddSANStorageDomainCommand.java index 59ed51e..41245b4 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddSANStorageDomainCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddSANStorageDomainCommand.java @@ -65,7 +65,7 @@ @Override public Void runInTransaction() { for (LUNs lun : luns) { - proceedLUNInDb(lun, getStorageDomain().getstorage_type()); + proceedLUNInDb(lun, getStorageDomain().getstorage_type(), getStorageDomain().getstorage()); } return null; } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ExtendSANStorageDomainCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ExtendSANStorageDomainCommand.java index cd36ee8..4bf6b1e 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ExtendSANStorageDomainCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ExtendSANStorageDomainCommand.java @@ -25,7 +25,7 @@ protected void executeCommand() { setStorageDomainStatus(StorageDomainStatus.Locked, null); for (LUNs lun : getParameters().getLunsList()) { - proceedLUNInDb(lun, getStorageDomain().getstorage_type()); + proceedLUNInDb(lun, getStorageDomain().getstorage_type(), getStorageDomain().getstorage()); } if (Backend .getInstance() diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java index 59057d1..17815f7 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java @@ -196,6 +196,8 @@ if (DbFacade.getInstance().getDiskLunMapDao().getDiskIdByLunId(lun.getLUN_id()) == null) { DbFacade.getInstance().getLunDao().remove(lun.getLUN_id()); numOfRemovedLuns++; + } else { + DbFacade.getInstance().getLunDao().updateLUNsVolumeGroupId(lun.getLUN_id(), ""); } } if (numOfRemovedLuns > 0) { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java index b03719b..d984468 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java @@ -227,9 +227,16 @@ } public static void proceedLUNInDb(final LUNs lun, StorageType storageType) { + proceedLUNInDb(lun,storageType,""); + } + + public static void proceedLUNInDb(final LUNs lun, StorageType storageType, String volumeGroupId) { if (DbFacade.getInstance().getLunDao().get(lun.getLUN_id()) == null) { DbFacade.getInstance().getLunDao().save(lun); + } else if (!volumeGroupId.isEmpty()){ + DbFacade.getInstance().getLunDao().updateLUNsVolumeGroupId(lun.getLUN_id(), volumeGroupId); } + for (storage_server_connections connection : lun.getLunConnections()) { List<storage_server_connections> connections = DbFacade.getInstance() .getStorageServerConnectionDao().getAllForConnection(connection); diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/LunDAO.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/LunDAO.java index 938b680..45b12ba 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/LunDAO.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/LunDAO.java @@ -45,6 +45,11 @@ List<LUNs> getAll(); /** + * Updates the lun volume group id + */ + void updateLUNsVolumeGroupId(String id, String volumeGroupId); + + /** * Saves the supplied LUN instance. * * @param lun diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/LunDAODbFacadeImpl.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/LunDAODbFacadeImpl.java index ebab1c1..daa6820 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/LunDAODbFacadeImpl.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/LunDAODbFacadeImpl.java @@ -46,6 +46,14 @@ return getCallsHandler().executeRead("GetLUNByLUNId", MAPPER, parameterSource); } + @Override + public void updateLUNsVolumeGroupId(String id, String volumeGroupId) { + MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource().addValue("LUN_id", id) + .addValue("volume_group_id", volumeGroupId); + + getCallsHandler().executeModification("UpdateLUNsVolumeGroupId", parameterSource); + } + @SuppressWarnings("unchecked") @Override public List<LUNs> getAllForStorageServerConnection(String id) { @@ -55,7 +63,6 @@ return getCallsHandler().executeReadList("GetLUNsBystorage_server_connection", MAPPER, parameterSource); } - @SuppressWarnings("unchecked") @Override public List<LUNs> getAllForVolumeGroup(String id) { MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/LunDAOTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/LunDAOTest.java index 2c4436d..71c4c69 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/LunDAOTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/LunDAOTest.java @@ -70,6 +70,17 @@ } /** + * Ensures that LUNs are returned for the connection. + */ + @Test + public void testUpdateLUNsVolumeGroupId() { + String testGroupId = "testvolgroupId"; + dao.updateLUNsVolumeGroupId(existingLUN.getLUN_id(), testGroupId); + LUNs dbLun = dao.get(existingLUN.getLUN_id()); + assertEquals("LUNs volume group id wasn't updated", testGroupId, dbLun.getvolume_group_id()); + } + + /** * Ensures that an empty collection is returned. */ @Test -- To view, visit http://gerrit.ovirt.org/9229 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I38a0e3c68cb8bd80c2f78ee5aacfccc8c987a79e Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Aravot <lara...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
