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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
*/
package org.eclipse.hawkbit.repository.exception;

import java.util.Collection;

import org.eclipse.hawkbit.repository.model.DistributionSetType;
import org.eclipse.hawkbit.repository.model.TargetType;

Expand All @@ -24,14 +26,27 @@ public class DistributionSetTypeNotInTargetTypeException extends EntityNotFoundE
* Constructor
*
* @param distributionSetTypeId
* that is not part of given {@link TargetType}
* @param targetTypeId
* of the {@link TargetType} where given
* {@link DistributionSetType} is not part of
* that is not compatible with given {@link TargetType}s
* @param targetTypeIds
* of the {@link TargetType}s where given {@link DistributionSetType}
* is not part of
*/
public DistributionSetTypeNotInTargetTypeException(final Long distributionSetTypeId,
final Long targetTypeId) {
super(DistributionSetType.class.getSimpleName() + " with id {" + distributionSetTypeId + "} is not part of "
+ TargetType.class.getSimpleName() + " with id {" + targetTypeId + "}.");
final Collection<Long> targetTypeIds) {
super("DistributionSetType " + distributionSetTypeId + " is not compatible to TargetTypes " + targetTypeIds);
}

/**
* Constructor
*
* @param distributionSetTypeIds
* that are not compatible with given {@link TargetType}
* @param targetTypeId
* of the {@link TargetType} where given {@link DistributionSetType}s
* are not part of
*/
public DistributionSetTypeNotInTargetTypeException(final Collection<Long> distributionSetTypeIds,
final Long targetTypeId) {
super("DistributionSetTypes " + distributionSetTypeIds + " are not compatible to TargetTypes " + targetTypeId);
}
}
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 @@ -71,6 +72,7 @@
import org.eclipse.hawkbit.repository.model.DistributionSetType;
import org.eclipse.hawkbit.repository.model.SoftwareModuleType;
import org.eclipse.hawkbit.repository.model.Target;
import org.eclipse.hawkbit.repository.model.TargetType;
import org.eclipse.hawkbit.repository.model.TargetUpdateStatus;
import org.eclipse.hawkbit.repository.model.TargetWithActionType;
import org.eclipse.hawkbit.repository.model.helper.EventPublisherHolder;
Expand Down Expand Up @@ -229,10 +231,60 @@ 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

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 (controllerIds.size() > 1 && distSetIds.size() > 1) {
throw new IllegalStateException(
"Assigning multiple Targets to multiple Distribution Sets simultaneously is not allowed!");
}

if (distSetIds.size() == 1) {
checkCompatibilityForSingleDsAssignment(distSetIds.iterator().next(), controllerIds);
} else {
checkCompatibilityForMultiDsAssignment(controllerIds.iterator().next(), distSetIds);
}
}

private void checkCompatibilityForSingleDsAssignment(final Long distSetId, final List<String> controllerIds) {
final DistributionSetType distSetType = distributionSetManagement.getValidAndComplete(distSetId).getType();
final Set<Long> incompatibleTargetTypes = Lists.partition(controllerIds, Constants.MAX_ENTRIES_IN_STATEMENT)
.stream()
.map(ids -> targetRepository.findAll(TargetSpecifications.hasControllerIdIn(ids)
.and(TargetSpecifications.notCompatibleWithDistributionSetType(distSetType.getId()))))
.flatMap(List::stream).map(Target::getTargetType).map(TargetType::getId).collect(Collectors.toSet());

if (!incompatibleTargetTypes.isEmpty()) {
throw new DistributionSetTypeNotInTargetTypeException(distSetType.getId(), incompatibleTargetTypes);
}
}

private void checkCompatibilityForMultiDsAssignment(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) {
// we assume that list of assigned DS is less than MAX_ENTRIES_IN_STATEMENT
final Set<DistributionSetType> incompatibleDistSetTypes = distributionSetManagement.get(distSetIds).stream()
.map(DistributionSet::getType).collect(Collectors.toSet());
incompatibleDistSetTypes.removeAll(target.getTargetType().getCompatibleDistributionSetTypes());

if (!incompatibleDistSetTypes.isEmpty()) {
final Set<Long> distSetTypeIds = incompatibleDistSetTypes.stream().map(DistributionSetType::getId)
.collect(Collectors.toSet());
throw new DistributionSetTypeNotInTargetTypeException(distSetTypeIds, 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 +308,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 +333,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 +403,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 +865,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
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