winterhazel commented on issue #13796: URL: https://github.com/apache/cloudstack/issues/13796#issuecomment-5207354085
@Dogface2k > Therefore, if the runtime prevention and the idempotent database repair both ship in RC3, we do not need to copy the same repair into every historical 4.20 → 4.21 → 4.22 transition or add it again to 4.23 → 4.24 solely for this defect. That gives installations a reachable repair without executing the same cleanup repeatedly during one upgrade. I intend to fix the regression on 4.22.2 as well, so we need to add the repair to the 4.22.1 -> 4.22.2 path; otherwise, people that do not want to upgrade to 4.23 yet may still have old inconsistent entries in 4.22.2. > I would not restore PR #10008 verbatim. Its transaction removed project memberships first and then returned the result of _userDao.remove(userId). Transaction.execute commits whenever the callback returns normally, including when it returns false. That means the old ordering could commit the project cleanup while leaving the user active if the user DAO returned false. `_userDao.remove(userId)` only returns `false` if no user entries were updated (soft deleted) because there is no entry identified by `userId` (if a database error occurs, an exception is thrown instead). The `AccountManagerImpl#getValidUserVO` called by `AccountManagerImpl#deleteUser` ensures this user exists by querying the database. It may return `false` if the user's entry was deleted by another thread after `AccountManagerImpl#getValidUserVO`, but I don't think commiting the project cleanup in this scenario is an issue. > CloudStack’s normal deleteUserFromProject flow performs additional business bookkeeping. For each individual project membership it: > Removes the project_account entry. > Decrements the account’s ResourceType.project count when the removed membership has the Admin role. > Removes any corresponding user-scoped project invitation. > The user-deletion path should reuse or centralise those project-layer semantics instead of bypassing them with only a bulk DAO removal. Otherwise the immediate NPE is removed while project quota/resource-count state can remain incorrect until a later recalculation. That would still be a partial fix. Makes sense. We can add this additional cleanup. > The migration also needs to leave project resource counts consistent. Those counts should be recalculated from the surviving Admin memberships rather than blindly decremented, because recalculation remains correct if the cleanup is retried or if an installation already has inconsistent values. I think we can leave this adjustment for the automatic recalculation. The account will not be able to exceed its configured limits anyways. > There is one additional runtime path that must be covered before this class of corruption can be considered closed: moveUser. That's a good catch, I actually didn't know this API existed. The suggestion makes sense, but is complex. Therefore, for 4.23 RC3, I think we can just add a guard to prevent moving a user that is currently associated to a project, avoiding the creation of more inconsistent entries. > A defensive null check in ProjectJoinDaoImpl would also be reasonable so that one manually corrupted row cannot make the complete listProjects response fail, but only as secondary hardening. It must log and skip the invalid membership; it must not use findByIdIncludingRemoved to expose a deleted user, and it must not replace either runtime prevention or the database repair. This can also be implemented in a future release. > There is also one RC-specific caveat: a database already upgraded by RC2 may already report schema version 4.23.0.0, so moving from RC2 to RC3 would not normally rerun 42210→42300. If RC-to-RC database upgrades are intended to be supported, that path needs explicit handling or a documented one-off repair. This does not affect the normal supported upgrade path from released 4.22.x versions into 4.23 GA. RC-to-RC upgrade is not intended to be supported. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
