Skip to content
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

Feature - TargetType compatibility check #1180

Merged
merged 16 commits into from
Oct 15, 2021
Merged

Feature - TargetType compatibility check #1180

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
668e31b
added targettype compatibility check in deployment
singrob Sep 6, 2021
ce32bf3
added targettype compatibility for autssignment
singrob Sep 10, 2021
e591fd8
added / fixed tests for auto assignment compatibility check
singrob Sep 15, 2021
8449342
adapted rollout creation to use JPA specifications for compatibility …
singrob Sep 16, 2021
1fea35f
fix unit tests and javadoc
singrob Sep 17, 2021
8917e8c
fix copyright header
singrob Sep 17, 2021
2d8b575
fixed review findings
singrob Sep 22, 2021
27da500
Merge remote-tracking branch 'origin/master' into feature/targettype_…
singrob Sep 30, 2021
3038636
Merge remote-tracking branch 'origin/master' into feature/targettype_…
singrob Oct 1, 2021
5454679
fixed review findings
singrob Oct 1, 2021
1c855f1
replaced validated-DS management calls
singrob Oct 1, 2021
234e654
Merge remote-tracking branch 'origin/master' into feature/targettype_…
singrob Oct 1, 2021
a170ffd
Merge remote-tracking branch 'origin/master' into feature/targettype_…
singrob Oct 11, 2021
aba216d
Merge remote-tracking branch 'origin/master' into feature/targettype_…
singrob Oct 12, 2021
3235908
improved compatibility check in DeploymentManagementTest
singrob Oct 13, 2021
32fee34
fixed review findings
singrob Oct 14, 2021
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
47 changes: 29 additions & 18 deletions ...hawkbit-repository-api/src/main/java/org/eclipse/hawkbit/repository/TargetManagement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,22 @@
import org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions;
import org.eclipse.hawkbit.repository.builder.TargetCreate;
import org.eclipse.hawkbit.repository.builder.TargetUpdate;
import org.eclipse.hawkbit.repository.exception.AssignmentQuotaExceededException;
import org.eclipse.hawkbit.repository.exception.EntityAlreadyExistsException;
import org.eclipse.hawkbit.repository.exception.EntityNotFoundException;
import org.eclipse.hawkbit.repository.exception.AssignmentQuotaExceededException;
import org.eclipse.hawkbit.repository.exception.RSQLParameterSyntaxException;
import org.eclipse.hawkbit.repository.exception.RSQLParameterUnsupportedFieldException;
import org.eclipse.hawkbit.repository.model.DistributionSet;
import org.eclipse.hawkbit.repository.model.DistributionSetType;
import org.eclipse.hawkbit.repository.model.MetaData;
import org.eclipse.hawkbit.repository.model.RolloutGroup;
import org.eclipse.hawkbit.repository.model.Tag;
import org.eclipse.hawkbit.repository.model.Target;
import org.eclipse.hawkbit.repository.model.TargetFilterQuery;
import org.eclipse.hawkbit.repository.model.TargetMetadata;
import org.eclipse.hawkbit.repository.model.TargetTag;
import org.eclipse.hawkbit.repository.model.TargetType;
import org.eclipse.hawkbit.repository.model.TargetTagAssignmentResult;
import org.eclipse.hawkbit.repository.model.TargetType;
import org.eclipse.hawkbit.repository.model.TargetUpdateStatus;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
Expand Down Expand Up @@ -137,7 +138,7 @@ long countByFilters(Collection<TargetUpdateStatus> status, Boolean overdueState,
* if distribution set with given ID does not exist
*/
@PreAuthorize(SpringEvalExpressions.HAS_AUTH_READ_TARGET + SpringEvalExpressions.HAS_AUTH_OR
+ SpringEvalExpressions.HAS_AUTH_READ_REPOSITORY)
+ SpringEvalExpressions.HAS_AUTH_READ_REPOSITORY)
boolean existsByInstalledOrAssignedDistributionSet(long distId);

/**
Expand Down Expand Up @@ -232,9 +233,9 @@ long countByFilters(Collection<TargetUpdateStatus> status, Boolean overdueState,
void deleteByControllerID(@NotEmpty String controllerID);

/**
* Finds all targets for all the given parameter {@link TargetFilterQuery}
* and that don't have the specified distribution set in their action
* history.
* Finds all targets for all the given parameter {@link TargetFilterQuery} and
* that don't have the specified distribution set in their action history and
* are compatible with the passed {@link DistributionSetType}.
*
* @param pageRequest
* the pageRequest to enhance the query for paging and sorting
Expand All @@ -248,13 +249,13 @@ long countByFilters(Collection<TargetUpdateStatus> status, Boolean overdueState,
* if distribution set with given ID does not exist
*/
@PreAuthorize(SpringEvalExpressions.HAS_AUTH_READ_TARGET)
Page<Target> findByTargetFilterQueryAndNonDS(@NotNull Pageable pageRequest, long distributionSetId,
Page<Target> findByTargetFilterQueryAndNonDSAndCompatible(@NotNull Pageable pageRequest, long distributionSetId,
@NotNull String rsqlParam);

/**
* Counts all targets for all the given parameter {@link TargetFilterQuery}
* and that don't have the specified distribution set in their action
* history.
* Counts all targets for all the given parameter {@link TargetFilterQuery} and
* that don't have the specified distribution set in their action history and
* are compatible with the passed {@link DistributionSetType}.
*
* @param distributionSetId
* id of the {@link DistributionSet}
Expand All @@ -266,36 +267,46 @@ Page<Target> findByTargetFilterQueryAndNonDS(@NotNull Pageable pageRequest, long
* if distribution set with given ID does not exist
*/
@PreAuthorize(SpringEvalExpressions.HAS_AUTH_READ_TARGET)
long countByRsqlAndNonDS(long distributionSetId, @NotNull String rsqlParam);
long countByRsqlAndNonDSAndCompatible(long distributionSetId, @NotNull String rsqlParam);

/**
* Finds all targets for all the given parameter {@link TargetFilterQuery}
* and that are not assigned to one of the {@link RolloutGroup}s
* Finds all targets for all the given parameter {@link TargetFilterQuery} and
* that are not assigned to one of the {@link RolloutGroup}s and are compatible
* with the passed {@link DistributionSetType}.
*
* @param pageRequest
* the pageRequest to enhance the query for paging and sorting
* @param groups
* the list of {@link RolloutGroup}s
* @param rsqlParam
* filter definition in RSQL syntax
* @param distributionSetType
* type of the {@link DistributionSet} the targets must be compatible
* with
* @return a page of the found {@link Target}s
*/
@PreAuthorize(SpringEvalExpressions.HAS_AUTH_READ_TARGET)
Page<Target> findByTargetFilterQueryAndNotInRolloutGroups(@NotNull Pageable pageRequest,
@NotEmpty Collection<Long> groups, @NotNull String rsqlParam);
Page<Target> findByTargetFilterQueryAndNotInRolloutGroupsAndCompatible(@NotNull Pageable pageRequest,
@NotEmpty Collection<Long> groups, @NotNull String rsqlParam,
@NotNull DistributionSetType distributionSetType);

/**
* Counts all targets for all the given parameter {@link TargetFilterQuery}
* and that are not assigned to one of the {@link RolloutGroup}s
* Counts all targets for all the given parameter {@link TargetFilterQuery} and
* that are not assigned to one of the {@link RolloutGroup}s and are compatible
* with the passed {@link DistributionSetType}.
*
* @param groups
* the list of {@link RolloutGroup}s
* @param rsqlParam
* filter definition in RSQL syntax
* @param distributionSetType
* type of the {@link DistributionSet} the targets must be compatible
* with
* @return count of the found {@link Target}s
*/
@PreAuthorize(SpringEvalExpressions.HAS_AUTH_READ_TARGET)
long countByRsqlAndNotInRolloutGroups(@NotEmpty Collection<Long> groups, @NotNull String rsqlParam);
long countByRsqlAndNotInRolloutGroupsAndCompatible(@NotEmpty Collection<Long> groups, @NotNull String rsqlParam,
@NotNull DistributionSetType distributionSetType);

/**
* Finds all targets of the provided {@link RolloutGroup} that have no
Expand Down
89 changes: 75 additions & 14 deletions ...ository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaDeploymentManagement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.eclipse.hawkbit.repository.TenantConfigurationManagement;
import org.eclipse.hawkbit.repository.event.remote.TargetAssignDistributionSetEvent;
import org.eclipse.hawkbit.repository.exception.CancelActionNotAllowedException;
import org.eclipse.hawkbit.repository.exception.DistributionSetTypeNotInTargetTypeException;
import org.eclipse.hawkbit.repository.exception.EntityNotFoundException;
import org.eclipse.hawkbit.repository.exception.ForceQuitActionNotAllowedException;
import org.eclipse.hawkbit.repository.exception.MultiAssignmentIsNotEnabledException;
Expand Down Expand Up @@ -229,10 +230,70 @@ private List<DeploymentRequest> validateRequestForAssignments(List<DeploymentReq
deploymentRequests = deploymentRequests.stream().distinct().collect(Collectors.toList());
checkIfRequiresMultiAssignment(deploymentRequests);
}
checkForTargetTypeCompatibility(deploymentRequests);
checkQuotaForAssignment(deploymentRequests);
return deploymentRequests;
}

private void checkForTargetTypeCompatibility(final List<DeploymentRequest> deploymentRequests) {
singrob marked this conversation as resolved.
Show resolved Hide resolved
final List<String> controllerIds = deploymentRequests.stream().map(DeploymentRequest::getControllerId)
Copy link
Contributor

Choose a reason for hiding this comment

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

what about:

Suggested change
final List<String> controllerIds = deploymentRequests.stream().map(DeploymentRequest::getControllerId)
if (CollectionUtils.isEmpty(deploymentRequests)) {
return;
}
final List<String> controllerIds = deploymentRequests.stream().map(DeploymentRequest::getControllerId)
.distinct().collect(Collectors.toList());
final List<Long> dsIds = deploymentRequests.stream().map(DeploymentRequest::getDistributionSetId).distinct()
.collect(Collectors.toList());
if (controllerIds.size() > 1 && dsIds.size() > 1) {
throw new IllegalStateException(
"Assigning multiple Targets to multiple Distribution Sets simultaniously is not allowed!");
}
if (dsIds.size() == 1) {
checkCompatibilityForSingleDsAssignment(dsIds.iterator().next(), controllerIds);
} else {
checkCompatibilityForMultiDsAssignment(controllerIds.iterator().next(), dsIds);
}
}
private void checkCompatibilityForSingleDsAssignment(final Long dsId, final List<String> controllerIds) {
final DistributionSetType dsType = distributionSetManagement.getValidAndComplete(dsId).getType();
final Set<TargetType> incompatibleTargetTypes = Lists
.partition(controllerIds, Constants.MAX_ENTRIES_IN_STATEMENT).stream()
.map(ids -> targetRepository
.findAll(TargetSpecifications.hasControllerIdIn(ids).and(TargetSpecifications.hasTargetType())
.and(TargetSpecifications.notCompatibleToDsTypeId(dsType.getId()))))
.flatMap(List::stream).map(Target::getTargetType).collect(Collectors.toSet());
if (!incompatibleTargetTypes.isEmpty()) {
throw new DistributionSetTypeNotInTargetTypeException(Collections.singleton(dsType),
incompatibleTargetTypes);
}
}
private void checkCompatibilityForMultiDsAssignment(final String controllerId, final List<Long> dsIds) {
final TargetType targetType = targetRepository.findOne(TargetSpecifications.hasControllerId(controllerId))
.map(Target::getTargetType).orElseThrow(() -> new EntityNotFoundException(Target.class, controllerId));
if (targetType != null) {
// we assume that list of assigned DS is less than
// MAX_ENTRIES_IN_STATEMENT
final Set<DistributionSetType> incompatibleDsTypes = distributionSetManagement.get(dsIds).stream()
.map(DistributionSet::getType).collect(Collectors.toSet());
incompatibleDsTypes.removeAll(targetType.getCompatibleDistributionSetTypes());
if (!incompatibleDsTypes.isEmpty()) {
throw new DistributionSetTypeNotInTargetTypeException(incompatibleDsTypes,
Collections.singleton(targetType));
}
}
}

I changed the DistributionSetTypeNotInTargetTypeException with the assumption that it takes two sets of DsTypes and TargetTypes in order to generate the error like "Distribution Set type 'MyDsType' is not compatible to TargetType(s) 'MyTargetType1', 'MyTargetType2'". Please adapt if not necessary, but I believe you got an idea ;)

Copy link
Contributor Author

@singrob singrob Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the big suggestion, have only a few points:

  1. I like the restructure and extension of the exception handling, will adapt accordingly
        if (CollectionUtils.isEmpty(deploymentRequests)) {
            return;
        }

no need for this check, since it is enforced by the validator on the interface
3. I had a "fail fast"-approach in mind when writing the checks, just to reduce load on the DB. With your approach all the data will be loaded and checked. But I guess with the new target specification notCompatibleToDsTypeId the load on the DB won't be that high.
4.

        final TargetType targetType = targetRepository.findOne(TargetSpecifications.hasControllerId(controllerId))
                .map(Target::getTargetType).orElseThrow(() -> new EntityNotFoundException(Target.class, controllerId));

I guess this will throw the exception for all existing targets without a target type

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Yes, you are right, should be:
        final TargetType targetType = targetRepository.findOne(TargetSpecifications.hasControllerId(controllerId))
                .orElseThrow(() -> new EntityNotFoundException(Target.class, controllerId)).getTargetType();

.distinct().collect(Collectors.toList());
final List<Long> distSetIds = deploymentRequests.stream().map(DeploymentRequest::getDistributionSetId)
.distinct().collect(Collectors.toList());

if (distSetIds.size() == 1) {
checkCompatibilityByDistSetId(distSetIds.get(0), controllerIds);
return;
}

// Check for multi assignment case
if (distSetIds.size() > 1 && controllerIds.size() == 1) {
checkCompatibilityByControllerId(controllerIds.get(0), distSetIds);
return;
}

throw new IllegalStateException("List of deployment requests has illegal state");
}

private void checkCompatibilityByDistSetId(final Long distSetId, final List<String> controllerIds) {
final DistributionSetType distSetType = distributionSetManagement.getValidAndComplete(distSetId).getType();
final long compatibleTargets = Lists.partition(controllerIds, Constants.MAX_ENTRIES_IN_STATEMENT).stream()
.map(ids -> targetRepository.count(TargetSpecifications.hasControllerIdIn(ids)
.and(TargetSpecifications.isCompatibleWithDistributionSetType(distSetType.getId()))))
.mapToLong(Long::longValue).sum();
// Only load all targets if incompatibility was detected
if (controllerIds.size() > compatibleTargets) {
Lists.partition(controllerIds, Constants.MAX_ENTRIES_IN_STATEMENT).forEach(ids -> {
final Optional<JpaTarget> inCompatibleTarget = targetRepository
.findAll(TargetSpecifications.hasControllerIdIn(ids).and(TargetSpecifications.hasTargetType()))
.stream().filter(jpaTarget -> !jpaTarget.getTargetType()
.containsCompatibleDistributionSetType(distSetType))
.findFirst();
inCompatibleTarget.ifPresent(target -> {
throw new DistributionSetTypeNotInTargetTypeException(distSetType.getId(),
target.getTargetType().getId());
});
});
}
}

private void checkCompatibilityByControllerId(final String controllerId, final List<Long> distSetIds) {
final Target target = targetRepository.findOne(TargetSpecifications.hasControllerId(controllerId))
.orElseThrow(() -> new EntityNotFoundException(Target.class, controllerId));

if (target.getTargetType() != null) {
final Optional<DistributionSetType> incompatibleDistSet = distributionSetManagement.get(distSetIds).stream()
.map(DistributionSet::getType).filter(distributionSetType -> !target.getTargetType()
.containsCompatibleDistributionSetType(distributionSetType))
.findFirst();

incompatibleDistSet.ifPresent(distSetType -> {
throw new DistributionSetTypeNotInTargetTypeException(distSetType.getId(),
target.getTargetType().getId());
});
}
}

private static Map<Long, List<TargetWithActionType>> convertRequest(
final Collection<DeploymentRequest> deploymentRequests) {
return deploymentRequests.stream().collect(Collectors.groupingBy(DeploymentRequest::getDistributionSetId,
Expand All @@ -256,17 +317,17 @@ private DistributionSetAssignmentResult assignDistributionSetToTargetsWithRetry(
}

/**
* method assigns the {@link DistributionSet} to all {@link Target}s by
* their IDs with a specific {@link ActionType} and {@code forcetime}.
* method assigns the {@link DistributionSet} to all {@link Target}s by their
* IDs with a specific {@link ActionType} and {@code forcetime}.
*
*
* In case the update was executed offline (i.e. not managed by hawkBit) the
* handling differs my means that:<br/>
* A. it ignores targets completely that are in
* {@link TargetUpdateStatus#PENDING}.<br/>
* B. it created completed actions.<br/>
* C. sets both installed and assigned DS on the target and switches the
* status to {@link TargetUpdateStatus#IN_SYNC} <br/>
* C. sets both installed and assigned DS on the target and switches the status
* to {@link TargetUpdateStatus#IN_SYNC} <br/>
* D. does not send a {@link TargetAssignDistributionSetEvent}.<br/>
*
* @param initiatedBy
Expand All @@ -281,9 +342,9 @@ private DistributionSetAssignmentResult assignDistributionSetToTargetsWithRetry(
* the assignment strategy (online /offline)
* @return the assignment result
*
* @throw IncompleteDistributionSetException if mandatory
* {@link SoftwareModuleType} are not assigned as define by the
* {@link DistributionSetType}.
* @throws IncompleteDistributionSetException
* if mandatory {@link SoftwareModuleType} are not assigned as
* define by the {@link DistributionSetType}.
*/
private DistributionSetAssignmentResult assignDistributionSetToTargets(final String initiatedBy, final Long dsID,
final Collection<TargetWithActionType> targetsWithActionType, final String actionMessage,
Expand Down Expand Up @@ -351,9 +412,9 @@ private List<JpaAction> doAssignDistributionSetToTargets(final String initiatedB
}

/**
* split tIDs length into max entries in-statement because many database
* have constraint of max entries in in-statements e.g. Oracle with maximum
* 1000 elements, so we need to split the entries here and execute multiple
* split tIDs length into max entries in-statement because many database have
* constraint of max entries in in-statements e.g. Oracle with maximum 1000
* elements, so we need to split the entries here and execute multiple
* statements
*/
private static List<List<Long>> getTargetEntitiesAsChunks(final List<JpaTarget> targetEntities) {
Expand Down Expand Up @@ -813,10 +874,10 @@ public int deleteActionsByStatusAndLastModifiedBefore(final Set<Status> status,
return 0;
}
/*
* We use a native query here because Spring JPA does not support to
* specify a LIMIT clause on a DELETE statement. However, for this
* specific use case (action cleanup), we must specify a row limit to
* reduce the overall load on the database.
* We use a native query here because Spring JPA does not support to specify a
* LIMIT clause on a DELETE statement. However, for this specific use case
* (action cleanup), we must specify a row limit to reduce the overall load on
* the database.
*/

final int statusCount = status.size();
Expand Down
15 changes: 8 additions & 7 deletions ...t-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,8 @@ private RolloutGroup fillRolloutGroupWithTargets(final JpaRollout rollout, final

final long targetsInGroupFilter = DeploymentHelper.runInNewTransaction(txManager,
"countAllTargetsByTargetFilterQueryAndNotInRolloutGroups",
count -> targetManagement.countByRsqlAndNotInRolloutGroups(readyGroups, groupTargetFilter));
count -> targetManagement.countByRsqlAndNotInRolloutGroupsAndCompatible(readyGroups, groupTargetFilter,
rollout.getDistributionSet().getType()));
final long expectedInGroup = Math
.round((double) (group.getTargetPercentage() / 100) * (double) targetsInGroupFilter);
final long currentlyInGroup = DeploymentHelper.runInNewTransaction(txManager,
Expand Down Expand Up @@ -574,8 +575,8 @@ private Long assignTargetsToGroupInNewTransaction(final JpaRollout rollout, fina
final PageRequest pageRequest = PageRequest.of(0, Math.toIntExact(limit));
final List<Long> readyGroups = RolloutHelper.getGroupsByStatusIncludingGroup(rollout.getRolloutGroups(),
RolloutGroupStatus.READY, group);
final Page<Target> targets = targetManagement.findByTargetFilterQueryAndNotInRolloutGroups(pageRequest,
readyGroups, targetFilter);
final Page<Target> targets = targetManagement.findByTargetFilterQueryAndNotInRolloutGroupsAndCompatible(
pageRequest, readyGroups, targetFilter, rollout.getDistributionSet().getType());

createAssignmentOfTargetsToGroup(targets, group);

Expand All @@ -584,8 +585,8 @@ private Long assignTargetsToGroupInNewTransaction(final JpaRollout rollout, fina
}

/**
* Schedules a group of the rollout. Scheduled Actions are created to
* achieve this. The creation of those Actions is allowed to fail.
* Schedules a group of the rollout. Scheduled Actions are created to achieve
* this. The creation of those Actions is allowed to fail.
*/
private boolean scheduleRolloutGroup(final JpaRollout rollout, final JpaRolloutGroup group) {
final long targetsInGroup = rolloutTargetGroupRepository.countByRolloutGroup(group);
Expand Down Expand Up @@ -648,8 +649,8 @@ private void createAssignmentOfTargetsToGroup(final Page<Target> targets, final

/**
* Creates an action entry into the action repository. In case of existing
* scheduled actions the scheduled actions gets canceled. A scheduled action
* is created in-active.
* scheduled actions the scheduled actions gets canceled. A scheduled action is
* created in-active.
*/
private void createScheduledAction(final Collection<Target> targets, final DistributionSet distributionSet,
final ActionType actionType, final Long forcedTime, final Rollout rollout,
Expand Down
Loading