Fix dynamic rollout override behavior (#2447)

Since static assigments of dynamic rollouts always override the oldest rollouts it seems more consistent if this is the case also with dynamic assigments. I.e. all older are overriden and if there are actions of newer rollouts - won't be assigned. Signed-off-by: Avgustin Marinov <Avgustin.Marinov@bosch.com>
2025-06-11 15:24:48 +03:00
parent 58b969db38
commit 4cfd90b745
6 changed files with 92 additions and 118 deletions
--- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java
+++ b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java
@@ -91,13 +91,12 @@ public class JpaRolloutExecutor implements RolloutExecutor {
    /**
     * Action statuses that result in a terminated action
     */
-    private static final List<Status> DEFAULT_ACTION_TERMINATION_STATUSES = List.of(
-            Status.ERROR, Status.FINISHED, Status.CANCELED);
+    private static final List<Status> DEFAULT_ACTION_TERMINATION_STATUSES = List.of(Status.ERROR, Status.FINISHED, Status.CANCELED);
    /**
     * In case of DOWNLOAD_ONLY, actions can be finished with DOWNLOADED status.
     */
-    private static final List<Status> DOWNLOAD_ONLY_ACTION_TERMINATION_STATUSES = List.of(
-            Status.ERROR, Status.FINISHED, Status.CANCELED, Status.DOWNLOADED);
+    private static final List<Status> DOWNLOAD_ONLY_ACTION_TERMINATION_STATUSES =
+            List.of(Status.ERROR, Status.FINISHED, Status.CANCELED, Status.DOWNLOADED);
    private static final Comparator<RolloutGroup> DESC_COMP = Comparator.comparingLong(RolloutGroup::getId).reversed();
    private static final String TRANSACTION_ASSIGNING_TARGETS_TO_ROLLOUT_GROUP_FAILED = "Transaction assigning Targets to RolloutGroup failed";

@@ -647,7 +646,7 @@ public class JpaRolloutExecutor implements RolloutExecutor {
                    rollout.getRolloutGroups(), RolloutGroupStatus.READY, group);
            final Slice<Target> targets;
            if (!RolloutHelper.isRolloutRetried(rollout.getTargetFilterQuery())) {
-                targets = targetManagement.findByTargetFilterQueryAndNotInRolloutAndCompatibleAndUpdatable(
+                targets = targetManagement.findByRsqlAndNotInRolloutGroupsAndCompatibleAndUpdatable(
                        readyGroups, targetFilter, rollout.getDistributionSet().getType(), pageRequest);
            } else {
                targets = targetManagement.findByFailedRolloutAndNotInRolloutGroups(
@@ -688,8 +687,7 @@ public class JpaRolloutExecutor implements RolloutExecutor {
            long targetsLeftToAdd = expectedInGroup - currentlyInGroup;
            final String groupTargetFilter = RolloutHelper.getGroupTargetFilter(
                    // don't use RolloutHelper.getTargetFilterQuery(rollout)
-                    // since it contains condition for device to be created
-                    // before the rollout
+                    // since it contains condition for device to be created before the rollout
                    rollout.getTargetFilterQuery(), group);
            long newActions = 0;
            do {
@@ -723,8 +721,8 @@ public class JpaRolloutExecutor implements RolloutExecutor {
        return false;
    }

-    private void createDynamicGroup(final JpaRollout rollout, final JpaRolloutGroup lastGroup, final int groupCount,
-            final RolloutGroupStatus status) {
+    private void createDynamicGroup(
+            final JpaRollout rollout, final JpaRolloutGroup lastGroup, final int groupCount, final RolloutGroupStatus status) {
        try {
            RolloutHelper.verifyRolloutGroupAmount(groupCount + 1, quotaManagement);
        } catch (final AssignmentQuotaExceededException e) {
@@ -738,8 +736,9 @@ public class JpaRolloutExecutor implements RolloutExecutor {

        final JpaRolloutGroup group = new JpaRolloutGroup();
        final String lastGroupWithoutSuffix = "group-" + groupCount;
-        final String suffix = lastGroup.getName().startsWith(lastGroupWithoutSuffix) ? lastGroup.getName()
-                .substring(lastGroupWithoutSuffix.length()) : "";
+        final String suffix = lastGroup.getName().startsWith(lastGroupWithoutSuffix)
+                ? lastGroup.getName().substring(lastGroupWithoutSuffix.length())
+                : "";
        final String nameAndDesc = "group-" + (groupCount + 1) + suffix;
        group.setName(nameAndDesc);
        group.setDescription(nameAndDesc);
@@ -767,15 +766,12 @@ public class JpaRolloutExecutor implements RolloutExecutor {
        ((JpaRolloutManagement) rolloutManagement).publishRolloutGroupCreatedEventAfterCommit(savedGroup, rollout);
    }

-    private int createActionsForDynamicGroupInNewTransaction(final JpaRollout rollout, final RolloutGroup group,
-            final String targetFilter, final long limit) {
+    private int createActionsForDynamicGroupInNewTransaction(
+            final JpaRollout rollout, final RolloutGroup group, final String targetFilter, final long limit) {
        return DeploymentHelper.runInNewTransaction(txManager, "createActionsForRolloutDynamicGroup", status -> {
-            final PageRequest pageRequest = PageRequest.of(0, Math.toIntExact(limit));
-            final Slice<Target> targets = targetManagement.findByTargetFilterQueryAndNoOverridingActionsAndNotInRolloutAndCompatibleAndUpdatable(
-                    rollout.getId(), rollout.getWeight().orElse(1000), rolloutGroupRepository.findByRolloutOrderByIdAsc(rollout).get(0).getId(),
-                    targetFilter, rollout.getDistributionSet().getType(), pageRequest
-                    // Dynamic rollouts shall always have weight!
-            );
+            // Dynamic rollouts shall always have weight!
+            final Slice<Target> targets = targetManagement.findByRsqlAndNoOverridingActionsAndNotInRolloutAndCompatibleAndUpdatable(
+                    rollout.getId(), targetFilter, rollout.getDistributionSet().getType(), PageRequest.of(0, Math.toIntExact(limit)));

            if (targets.getNumberOfElements() == 0) {
                return 0;
--- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaTargetManagement.java
+++ b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaTargetManagement.java
@@ -277,46 +277,18 @@ public class JpaTargetManagement implements TargetManagement {
    }

    @Override
-    public Slice<Target> findByTargetFilterQueryAndNotInRolloutAndCompatibleAndUpdatable(
-            final Collection<Long> groups, final String targetFilterQuery, final DistributionSetType dsType, final Pageable pageable) {
+    public Slice<Target> findByRsqlAndNotInRolloutGroupsAndCompatibleAndUpdatable(
+            final Collection<Long> groups, final String rsql, final DistributionSetType dsType, final Pageable pageable) {
        return targetRepository
                .findAllWithoutCount(AccessController.Operation.UPDATE,
                        combineWithAnd(List.of(
-                                RSQLUtility.buildRsqlSpecification(targetFilterQuery, TargetFields.class, virtualPropertyReplacer, database),
+                                RSQLUtility.buildRsqlSpecification(rsql, TargetFields.class, virtualPropertyReplacer, database),
                                TargetSpecifications.isNotInRolloutGroups(groups),
                                TargetSpecifications.isCompatibleWithDistributionSetType(dsType.getId()))),
                        pageable)
                .map(Target.class::cast);
    }

-    @Override
-    public Slice<Target> findByTargetFilterQueryAndNoOverridingActionsAndNotInRolloutAndCompatibleAndUpdatable(
-            final long rolloutId, final int weight, final long firstGroupId, final String targetFilterQuery,
-            final DistributionSetType distributionSetType, final Pageable pageable) {
-        return targetRepository
-                .findAllWithoutCount(AccessController.Operation.UPDATE,
-                        combineWithAnd(List.of(
-                                RSQLUtility.buildRsqlSpecification(targetFilterQuery, TargetFields.class, virtualPropertyReplacer, database),
-                                TargetSpecifications.hasNoOverridingActionsAndNotInRollout(weight, rolloutId),
-                                TargetSpecifications.isCompatibleWithDistributionSetType(distributionSetType.getId()))),
-                        pageable)
-                .map(Target.class::cast);
-    }
-
-    @Override
-    public long countByActionsInRolloutGroup(final long rolloutGroupId) {
-        return targetRepository.count(TargetSpecifications.isInActionRolloutGroup(rolloutGroupId));
-    }
-
-    @Override
-    public Slice<Target> findByFailedRolloutAndNotInRolloutGroups(String rolloutId, Collection<Long> groups, Pageable pageable) {
-        final List<Specification<JpaTarget>> specList = List.of(
-                TargetSpecifications.failedActionsForRollout(rolloutId),
-                TargetSpecifications.isNotInRolloutGroups(groups));
-
-        return JpaManagementHelper.findAllWithCountBySpec(targetRepository, specList, pageable);
-    }
-
    @Override
    public long countByRsqlAndNotInRolloutGroupsAndCompatibleAndUpdatable(
            final String rsql, final Collection<Long> groups, final DistributionSetType dsType) {
@@ -327,6 +299,14 @@ public class JpaTargetManagement implements TargetManagement {
                        TargetSpecifications.isCompatibleWithDistributionSetType(dsType.getId()))));
    }

+    @Override
+    public Slice<Target> findByFailedRolloutAndNotInRolloutGroups(String rolloutId, Collection<Long> groups, Pageable pageable) {
+        final List<Specification<JpaTarget>> specList = List.of(
+                TargetSpecifications.failedActionsForRollout(rolloutId),
+                TargetSpecifications.isNotInRolloutGroups(groups));
+        return JpaManagementHelper.findAllWithCountBySpec(targetRepository, specList, pageable);
+    }
+
    @Override
    public long countByFailedRolloutAndNotInRolloutGroups(String rolloutId, Collection<Long> groups) {
        final List<Specification<JpaTarget>> specList = List.of(
@@ -335,6 +315,24 @@ public class JpaTargetManagement implements TargetManagement {
        return JpaManagementHelper.countBySpec(targetRepository, specList);
    }

+    @Override
+    public Slice<Target> findByRsqlAndNoOverridingActionsAndNotInRolloutAndCompatibleAndUpdatable(
+            final long rolloutId, final String rsql, final DistributionSetType distributionSetType, final Pageable pageable) {
+        return targetRepository
+                .findAllWithoutCount(AccessController.Operation.UPDATE,
+                        combineWithAnd(List.of(
+                                RSQLUtility.buildRsqlSpecification(rsql, TargetFields.class, virtualPropertyReplacer, database),
+                                TargetSpecifications.hasNoOverridingActionsAndNotInRollout(rolloutId),
+                                TargetSpecifications.isCompatibleWithDistributionSetType(distributionSetType.getId()))),
+                        pageable)
+                .map(Target.class::cast);
+    }
+
+    @Override
+    public long countByActionsInRolloutGroup(final long rolloutGroupId) {
+        return targetRepository.count(TargetSpecifications.isInActionRolloutGroup(rolloutGroupId));
+    }
+
    @Override
    public Slice<Target> findByInRolloutGroupWithoutAction(final long group, final Pageable pageable) {
        if (!rolloutGroupRepository.existsById(group)) {
--- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/specifications/TargetSpecifications.java
+++ b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/specifications/TargetSpecifications.java
@@ -173,8 +173,8 @@ public final class TargetSpecifications {
     * @return the {@link Target} {@link Specification}
     */
    public static Specification<JpaTarget> isOverdue(final long overdueTimestamp) {
-        return (targetRoot, query, cb) -> cb.lessThanOrEqualTo(targetRoot.get(JpaTarget_.lastTargetQuery),
-                overdueTimestamp);
+        return (targetRoot, query, cb) ->
+                cb.lessThanOrEqualTo(targetRoot.get(JpaTarget_.lastTargetQuery), overdueTimestamp);
    }

    /**
@@ -186,7 +186,8 @@ public final class TargetSpecifications {
    public static Specification<JpaTarget> likeControllerIdOrName(final String searchText) {
        return (targetRoot, query, cb) -> {
            final String searchTextToLower = searchText.toLowerCase();
-            return cb.or(cb.like(cb.lower(targetRoot.get(JpaTarget_.controllerId)), searchTextToLower),
+            return cb.or(
+                    cb.like(cb.lower(targetRoot.get(JpaTarget_.controllerId)), searchTextToLower),
                    cb.like(cb.lower(targetRoot.get(AbstractJpaNamedEntity_.name)), searchTextToLower));
        };
    }
@@ -251,8 +252,8 @@ public final class TargetSpecifications {
     * @return the {@link Target} {@link Specification}
     */
    public static Specification<JpaTarget> hasAssignedDistributionSet(final Long distributionSetId) {
-        return (targetRoot, query, cb) -> cb.equal(
-                targetRoot.get(JpaTarget_.assignedDistributionSet).get(AbstractJpaBaseEntity_.id), distributionSetId);
+        return (targetRoot, query, cb) ->
+                cb.equal(targetRoot.get(JpaTarget_.assignedDistributionSet).get(AbstractJpaBaseEntity_.id), distributionSetId);
    }

    /**
@@ -358,10 +359,8 @@ public final class TargetSpecifications {
     */
    public static Specification<JpaTarget> hasNoActionInRolloutGroup(final Long group) {
        return (targetRoot, query, cb) -> {
-            final ListJoin<JpaTarget, RolloutTargetGroup> rolloutTargetJoin = targetRoot
-                    .join(JpaTarget_.rolloutTargetGroup, JoinType.INNER);
-            rolloutTargetJoin.on(
-                    cb.equal(rolloutTargetJoin.get(RolloutTargetGroup_.rolloutGroup).get(AbstractJpaBaseEntity_.id), group));
+            final ListJoin<JpaTarget, RolloutTargetGroup> rolloutTargetJoin = targetRoot.join(JpaTarget_.rolloutTargetGroup, JoinType.INNER);
+            rolloutTargetJoin.on(cb.equal(rolloutTargetJoin.get(RolloutTargetGroup_.rolloutGroup).get(AbstractJpaBaseEntity_.id), group));

            final ListJoin<JpaTarget, JpaAction> actionsJoin = targetRoot.join(JpaTarget_.actions, JoinType.LEFT);
            actionsJoin.on(cb.equal(actionsJoin.get(JpaAction_.rolloutGroup).get(AbstractJpaBaseEntity_.id), group));
@@ -388,7 +387,6 @@ public final class TargetSpecifications {
     * @return the {@link Target} {@link Specification}
     */
    public static Specification<JpaTarget> hasTag(final Long tagId) {
-
        return (targetRoot, query, cb) -> {
            final SetJoin<JpaTarget, JpaTargetTag> tags = targetRoot.join(JpaTarget_.tags, JoinType.LEFT);
            return cb.equal(tags.get(AbstractJpaBaseEntity_.id), tagId);
@@ -435,31 +433,14 @@ public final class TargetSpecifications {
    }

    /**
-     * {@link Specification} for retrieving {@link Target}s that have:
-     * <ul>
-     *     <li>no active (non-finished) actions with greater weight in the older rollouts</li>
-     *     <li>or have actions in the current rollout</li>
-     *     <li>or have actions with great or equal weight in the newer rollouts</li>
-     * </ul>
+     * {@link Specification} for retrieving {@link Target}s that have no overriding actions - i.e. no actions from newer rollouts
     *
-     * @param weight the referent weight
     * @return the {@link Target} {@link Specification}
     */
-    public static Specification<JpaTarget> hasNoOverridingActionsAndNotInRollout(final int weight, final long rolloutId) {
+    public static Specification<JpaTarget> hasNoOverridingActionsAndNotInRollout(final long rolloutId) {
        return (targetRoot, query, cb) -> {
            final ListJoin<JpaTarget, JpaAction> actionsJoin = targetRoot.join(JpaTarget_.actions, JoinType.LEFT);
-            actionsJoin.on(
-                    cb.or(
-                            cb.and(
-                                    cb.and(
-                                            cb.lt(actionsJoin.get(JpaAction_.rollout).get(AbstractJpaBaseEntity_.id), rolloutId),
-                                            cb.gt(actionsJoin.get(JpaAction_.weight), weight)),
-                                    cb.or(
-                                            cb.equal(actionsJoin.get(JpaAction_.active), true),
-                                            cb.equal(actionsJoin.get(JpaAction_.status), Action.Status.SCHEDULED))),
-                            cb.and(
-                                    cb.ge(actionsJoin.get(JpaAction_.rollout).get(AbstractJpaBaseEntity_.id), rolloutId),
-                                    cb.ge(actionsJoin.get(JpaAction_.weight), weight))));
+            actionsJoin.on(cb.ge(actionsJoin.get(JpaAction_.rollout).get(AbstractJpaBaseEntity_.id), rolloutId));
            return cb.isNull(actionsJoin.get(AbstractJpaBaseEntity_.id));
        };
    }
@@ -497,4 +478,4 @@ public final class TargetSpecifications {
        final Join<JpaTarget, JpaTargetType> targetTypeJoin = root.join(JpaTarget_.targetType, JoinType.LEFT);
        return targetTypeJoin.join(JpaTargetType_.distributionSetTypes, JoinType.LEFT).get(AbstractJpaBaseEntity_.id);
    }
-}
+}
--- a/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/TargetManagementSecurityTest.java
+++ b/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/TargetManagementSecurityTest.java
@@ -147,9 +147,9 @@ class TargetManagementSecurityTest extends AbstractJpaIntegrationTest {

    @Test
    @Description("Tests ManagementAPI PreAuthorized method with correct and insufficient permissions.")
-    void findByTargetFilterQueryAndNotInRolloutAndCompatibleAndUpdatablePermissionsCheck() {
+    void findByRsqlAndNotInRolloutGroupsAndCompatibleAndUpdatablePermissionsCheck() {
        assertPermissions(
-                () -> targetManagement.findByTargetFilterQueryAndNotInRolloutAndCompatibleAndUpdatable(List.of(1L), "controllerId==id",
+                () -> targetManagement.findByRsqlAndNotInRolloutGroupsAndCompatibleAndUpdatable(List.of(1L), "controllerId==id",
                        entityFactory.distributionSetType().create().build(), PAGE
                ), List.of(SpPermission.READ_TARGET, SpPermission.READ_ROLLOUT));
    }
--- a/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/TargetManagementTest.java
+++ b/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/TargetManagementTest.java
@@ -825,7 +825,7 @@ class TargetManagementTest extends AbstractJpaIntegrationTest {
        assertThat(changedLockRevisionTarget2.getLastModifiedAt()).isPositive();

        // verify updated meta data contains the updated value
-        assertThat(targetManagement.getMetadata(target.getControllerId()).get(knownKey)).isEqualTo(knownUpdateValue);
+        assertThat(targetManagement.getMetadata(target.getControllerId())).containsEntry(knownKey, knownUpdateValue);
    }

    @Test
@@ -1137,20 +1137,20 @@ class TargetManagementTest extends AbstractJpaIntegrationTest {
        // old ro with equal weight - match
        expected.add(target);
        createAction(targets.get(target++), rolloutOlder, 10, Status.RUNNING, distributionSet);
-        // old ro with bigger weight, scheduled - doesn't match
+        // old ro with bigger weight, scheduled - match
+        expected.add(target);
        createAction(targets.get(target++), rolloutOlder, 11, Status.SCHEDULED, distributionSet);
-        // old ro with bigger weight, running - doesn't match
+        // old ro with bigger weight, running - match
+        expected.add(target);
        createAction(targets.get(target++), rolloutOlder, 11, Status.RUNNING, distributionSet);
-        // old ro with bigger weight, running match
+        // old ro with bigger weight, running - match
        expected.add(target);
        createAction(targets.get(target++), rolloutOlder, 11, Status.FINISHED, distributionSet);
        // same ro - doesn't match
        createAction(targets.get(target++), rollout, 10, Status.RUNNING, distributionSet);
-        // new ro with less weight - match
-        expected.add(target);
+        // new ro with less weight - doesn't match
        createAction(targets.get(target++), rolloutNewer, 0, Status.RUNNING, distributionSet);
-        // new ro with less weight - match
-        expected.add(target);
+        // new ro with less weight - doesn't match
        createAction(targets.get(target++), rolloutNewer, 5, Status.WARNING, distributionSet);
        // NEW ro with EQUAL weight - doesn't match
        createAction(targets.get(target++), rolloutNewer, 10, Status.RUNNING, distributionSet);
@@ -1158,8 +1158,8 @@ class TargetManagementTest extends AbstractJpaIntegrationTest {
        createAction(targets.get(target), rolloutNewer, 20, Status.DOWNLOADED, distributionSet);

        final Slice<Target> matching =
-                targetManagement.findByTargetFilterQueryAndNoOverridingActionsAndNotInRolloutAndCompatibleAndUpdatable(
-                        rollout.getId(), 10, Long.MAX_VALUE, "controllerid==dyn_action_filter_*", distributionSet.getType(), PAGE);
+                targetManagement.findByRsqlAndNoOverridingActionsAndNotInRolloutAndCompatibleAndUpdatable(
+                        rollout.getId(), "controllerid==dyn_action_filter_*", distributionSet.getType(), PAGE);

        assertThat(matching.getNumberOfElements()).isEqualTo(expected.size());
        assertThat(matching.stream()