hawkbit icon indicating copy to clipboard operation
hawkbit copied to clipboard

Trigger Distribution Set assignment check on updating target config data

Open Sergey-G-dev opened this issue 5 years ago • 6 comments

This is an attempt to solve the issue described here: https://github.com/eclipse/hawkbit/issues/983

Summary: whenever a target uploads its configuration over REST endpoint '.../configData' a check for Distribution Set assignment is triggered based on existing Target Filters. This makes AutoAssignmentChecker and Scheduler obsolete. The database load is reduced significantly since there is no need to execute the assignment check regularly.

Signed-off-by: Sergey Gerasimov [email protected]

Sergey-G-dev avatar Oct 09 '20 13:10 Sergey-G-dev

Thanks a lot for your contribution! I have one remark though: "This makes AutoAssignmentChecker and Scheduler obsolete", not really. Target Filter isn't scoped to Target attributes only, this involves Target name, metadata, description, etc. as well. So as an example: if new Target matching the Filter (say by name) comes online and being registered in Hawkbit per plug-and-play mechanism it should receive the DS assignment irregardless of whether it sends attributes or not.

bogdan-bondar avatar Oct 12 '20 08:10 bogdan-bondar

You are correct, @bogdan-bondar . My choice of words wasn't very precise. By "attributes" I meant not only the actual attributes, but all the relevant fields as defined in the class TargetFields.java, since those are used by the filters. Still, I don't see how the AutoAssignmentScheduler/Checker is usefull after this change, because it does exactly the same thing, just at a scheduled time interval.

Sergey-G-dev avatar Oct 12 '20 09:10 Sergey-G-dev

SonarQube analysis reported 4 issues

  • MINOR 4 minor

Watch the comments in this conversation to review them.

hawkbit-bot avatar Oct 13 '20 15:10 hawkbit-bot

After rebasing this branch on top of the current master, CircleCI complains about 2 failed Unit/Integration tests, but the log messages make no sense to me:

[ERROR]   ControllerManagementTest.controllerReportsActionFinishedForDownloadOnlyActionThatIsNotActive Did not receive the expected amount of events form class org.eclipse.hawkbit.repository.event.remote.entity.TargetUpdatedEvent Expected: 3 but was: 3
[ERROR]   ControllerManagementTest.updateTargetAttributesWithDifferentUpdateModes Did not receive the expected amount of events form class org.eclipse.hawkbit.repository.event.remote.entity.TargetCreatedEvent Expected: 1 but was: 1

Running the build on my local machine works fine with all tests passing. Any ideas?

Sergey-G-dev avatar Nov 05 '20 13:11 Sergey-G-dev

Running the build on my local machine works fine with all tests passing. Any ideas?

I triggered the build 5 times in a row (cf. PR builds) without error. I assume that this was some kind of flakiness on CircleCI side

schabdo avatar Jan 05 '21 15:01 schabdo

Hi @Sergey-G-dev ! Eventually I got the chance to take a look at your PR, sorry for the delay. I really like your idea of checking if there is an assignment available based on the Target change rather than querying all the targets periodically for each target filter. However, from my point of view, the current implementation is not quite there yet.

First of all, you trigger the assignment check only when targets update their attributes: DdiRootController#putConfigData -> controllerManagement.triggerDistributionSetAssignmentCheck, but what if a target does not send its attributes at all, or if my filter is based on targets' metadata only? I would rather react on TargetCreated/TargetUpdated events and perform the filter matching in the background thread of some autoAssignmentExecutor not to block the async communication. However this approach should also be carefully evaluated comparing the amount/frequency of TargetCreated/TargetUpdated events and filter matching performance/resource consumption, probably a cache should be introduced for the target filter queries to minimise the database calls.

Second notice concerns the introduction of many duplications under ControllerManagement and DeploymentManagement and re-implementing the logic of AutoAssignChecker/Scheduler. I would suggest to follow the separation of concerns pattern and just adapt/extend the classes that are already responsible for the auto-assignment as opposed to moving all of the logic into the Management layer. Moreover overloading should be used where possible for effectively the same methods with different signature (e.g. list/single arguments). We could discuss additional minor remarks after the concrete structure is agreed upon.

In case you have any questions/concerns feel free to contact me!

bogdan-bondar avatar Jan 19 '21 18:01 bogdan-bondar