[server] More checks when deleing old images; look for revivable images

Revivable images are those where the expiry date lies in the future,
aren't missing the HDD image, yet are marked as invalid. Make those
valid again.

2 files changed, 63 insertions, 20 deletions

diff --git a/dozentenmodulserver/src/main/java/org/openslx/bwlp/sat/database/mappers/DbImage.java b/dozentenmodulserver/src/main/java/org/openslx/bwlp/sat/database/mappers/DbImage.java
index a6752f2d..e1031bea 100644
--- a/dozentenmodulserver/src/main/java/org/openslx/bwlp/sat/database/mappers/DbImage.java
+++ b/dozentenmodulserver/src/main/java/org/openslx/bwlp/sat/database/mappers/DbImage.java
@@ -193,10 +193,6 @@ public class DbImage {
 	/**
 	 * Private helper to create an {@link ImageSummaryRead} instance from a
 	 * {@link ResultSet}
-	 *
-	 * @param rs
-	 * @return
-	 * @throws SQLException
 	 */
 	private static ImageSummaryRead resultSetToSummary(UserInfo user, ResultSet rs) throws SQLException {
 		ImagePermissions defaultPermissions = DbImagePermissions.fromResultSetDefault(rs);
@@ -885,7 +881,7 @@ public class DbImage {
 	}
 
 	/**
-	 * Get all images with mussing virtid or osid.
+	 * Get all images with missing virtid or osid.
 	 *
 	 * @return
 	 * @throws SQLException
@@ -1205,5 +1201,27 @@ public class DbImage {
 		}
 	}
 
+	/**
+	 * Get potentially still valid image versions, i.e. those where
+	 * the expire time lies in the future, but which are marked disabled.
+	 */
+	public static Set<LocalImageVersion> getAllInvalid() throws SQLException {
+		try (MysqlConnection connection = Database.getConnection()) {
+			// Get
+			MysqlStatement sstmt = connection.prepareStatement(localImageBaseSql
+					+ " WHERE v.isvalid = 0 AND v.expiretime > :now");
+			sstmt.setLong("now", Util.unixTime());
+			ResultSet rs = sstmt.executeQuery();
+			Set<LocalImageVersion> list = new HashSet<>();
+			while (rs.next()) {
+				list.add(toLocalImageVersion(rs));
+			}
+			return list;
+		} catch (SQLException e) {
+			LOGGER.error("Query failed in DbImage.resetDeleteState()", e);
+			throw e;
+		}
+	}
+
 
 }
diff --git a/dozentenmodulserver/src/main/java/org/openslx/bwlp/sat/maintenance/DeleteOldImages.java b/dozentenmodulserver/src/main/java/org/openslx/bwlp/sat/maintenance/DeleteOldImages.java
index 79843c02..c3833f86 100644
--- a/dozentenmodulserver/src/main/java/org/openslx/bwlp/sat/maintenance/DeleteOldImages.java
+++ b/dozentenmodulserver/src/main/java/org/openslx/bwlp/sat/maintenance/DeleteOldImages.java
@@ -1,5 +1,6 @@
 package org.openslx.bwlp.sat.maintenance;
 
+import java.io.File;
 import java.sql.SQLException;
 import java.util.HashSet;
 import java.util.List;
@@ -66,53 +67,62 @@ public class DeleteOldImages implements Runnable {
 	@Override
 	public void run() {
 		// Get all images currently marked as "Should delete" and reset them to "keep"
+		// This is in case a previous run crashed
 		LOGGER.info("Looking for old image versions to delete...");
-		Set<String> resetList;
 		try {
-			resetList = DbImage.resetDeleteState();
+			DbImage.resetDeleteState();
 		} catch (SQLException e1) {
-			resetList = new HashSet<>();
 		}
 		if (!FileSystem.isStorageMounted()) {
 			LOGGER.warn("Will not execute deletion of old images; store seems to be unmounted!");
 			return;
 		}
-		Set<LocalImageVersion> versions = new HashSet<>();
+		Set<LocalImageVersion> markAsDelete = new HashSet<>();
 		// First get a list of all image versions which reached their expire date,
 		// no matter if valid or invalid
 		try {
 			List<LocalImageVersion> list = DbImage.getExpiringLocalImageVersions(0);
-			versions.addAll(list);
+			for (LocalImageVersion iv : list) {
+				if (iv.isValid) {
+					LOGGER.info("Invalidating: " + iv.imageVersionId + " - expired");
+				}
+			}
+			markAsDelete.addAll(list);
 		} catch (SQLException e) {
 			LOGGER.error("Will not be able to clean up old image versions");
 		}
+		// Next, get versions that have no osId or virtId
 		try {
 			List<LocalImageVersion> list = DbImage.getVersionsWithMissingData();
-			versions.addAll(list);
+			for (LocalImageVersion iv : list) {
+				if (iv.isValid) {
+					LOGGER.info("Invalidating: " + iv.imageVersionId + " - osId or virtId is NULL");
+				}
+			}
+			markAsDelete.addAll(list);
 		} catch (SQLException e) {
 			LOGGER.error("Will not be able to clean up invalid image versions");
 		}
 		// Mark all as invalid. This will also trigger mails if they have been valid before
 		try {
-			DbImage.markValid(false, false, versions.toArray(new LocalImageVersion[versions.size()]));
+			DbImage.markValid(false, false, markAsDelete.toArray(new LocalImageVersion[markAsDelete.size()]));
 		} catch (SQLException e) {
 			LOGGER.error("Could not mark images to be deleted as invalid. Cleanup of old images failed.");
 			return;
 		}
 		int hardDeleteCount = 0;
 		final long hardDelete = Util.unixTime() - 86400;
-		for (LocalImageVersion version : versions) {
+		Set<String> markAsDeleteIds = new HashSet<>();
+		for (LocalImageVersion version : markAsDelete) {
 			if (version.expireTime < hardDelete) {
 				// Delete them permanently only if they expired (at least) one day ago
 				hardDeleteCount++;
 				try {
 					DbImage.setDeletion(DeleteState.SHOULD_DELETE, version.imageVersionId);
+					markAsDeleteIds.add(version.imageVersionId);
 				} catch (SQLException e) {
 				}
 			}
-			// Remove all versions from our reset list that were just disabled again, so we keep those
-			// that have potentially been falsely disabled before
-			resetList.remove(version.imageVersionId);
 		}
 		// Delete base images with no image versions (including invalid ones)
 		int baseDeleteCount = 0;
@@ -121,11 +131,26 @@ public class DeleteOldImages implements Runnable {
 		} catch (SQLException e) {
 			// Logging done in method
 		}
-		LOGGER.info("Deletion done. Soft: " + (versions.size() - hardDeleteCount) + ", hard: "
+		LOGGER.info("Deletion done. Soft: " + (markAsDelete.size() - hardDeleteCount) + ", hard: "
 				+ hardDeleteCount + ", base: " + baseDeleteCount);
-		// Aftermath: We might have a list of image versions that have been un-marked from deletion,
-		// and weren't re-marked in this run. This means there might have been clock skew or other problems.
-		// So let's check those images' files, and if they're ok, we also set the 'isvalid' flag again
+		// Check for any VMs that ended up with isvalid = false, but seem to be fine, i.e. are
+		// not expired and the according image file is still around
+		try {
+			Set<LocalImageVersion> list = DbImage.getAllInvalid();
+			for (LocalImageVersion iv : list) {
+				if (markAsDeleteIds.contains(iv.imageVersionId))
+					continue;
+				File img = FileSystem.composeAbsoluteImagePath(iv);
+				if (img.isFile() && !img.isDirectory()) {
+					DbImage.markValid(true, true, iv);
+					if (!DeleteState.KEEP.equals(iv.deleteState)) {
+						DbImage.setDeletion(DeleteState.KEEP, iv.imageVersionId);
+					}
+					LOGGER.info("Revived image version " + iv.imageVersionId + ": Was disabled for no good reason");
+				}
+			}
+		} catch (SQLException e) {
+		}
 	}
 
 	public static StringBuilder hardDeleteImages() {