fix: discard update action on notify only #1637
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
saranyailla
commented
Jul 10, 2024
| @@ -504,10 +505,12 @@ private void cancelCurrentDeployment() { | |||
| .isCancellable()) { | |||
| logger.atInfo().log("Deployment already finished processing or cannot be cancelled"); | |||
| } else { | |||
| boolean canCancelDeployment = context.get(UpdateSystemPolicyService.class).discardPendingUpdateAction( | |||
There was a problem hiding this comment.
Currently, this returns true for the deployments that do not have the update policy set to DeploymentComponentUpdatePolicyAction.NOTIFY_COMPONENTS which is wrong. We should discard an update only when the policy is set to notify.
saranyailla
commented
Jul 10, 2024
| logger.atError().setEventType("service-update-action-error").addKeyValue("action", todo.getKey()) | ||
| .setCause(t).log(); | ||
| } | ||
| final UpdateAction pendingUpdateAction = pendingActions.remove(deploymentId); |
There was a problem hiding this comment.
Fix: Pending actions that only correspond to the specified deployment id should be performed.
Fix and refactor: Rely on pendingActions map to check if the update action is in progress/completed or never added.
There was a problem hiding this comment.
Does this mean that a deferred deployment cannot be cancelled? Ideally, if a component is deferring a config merge due a deployment, this deployment should still be cancellable given that defer period is still ongoing.
There was a problem hiding this comment.
- Deferred deployments are still cancellable. That code path is not modified.
runUpdateActionsis run only after we waited enough for the deferred time.- While we're waiting for the deferred time, deployment pending actions can be discarded upon request.
saranyailla
commented
Jul 10, 2024
Comment on lines
152
to
159
|
|
||
| boolean ggcRestarting = false; | ||
| for (UpdateAction action : pendingActions.values()) { | ||
| if (action.isGgcRestart()) { | ||
| ggcRestarting = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix: Restarting GG feature flag should be set only for the update actions of a single deployment ID.
saranyailla
commented
Jul 10, 2024
|
|
||
| PreComponentUpdateEvent preComponentUpdateEvent = new PreComponentUpdateEvent(); | ||
| preComponentUpdateEvent.setIsGgcRestarting(ggcRestarting); | ||
| String deploymentId = pendingActions.values().stream().map(UpdateAction::getDeploymentId).findFirst().get(); |
There was a problem hiding this comment.
Fix: At this point, pendingActions can be empty and result in NPE. So, we send component updates only if there's an entry by invoking a consumer with ifPresent.
saranyailla
commented
Jul 10, 2024
| return false; | ||
| } | ||
| final UpdateAction pendingUpdateAction = pendingActions.get(tag); |
There was a problem hiding this comment.
Fix - race: A thread can reach this point before the update action is completed (but in progress) resulting in returning an incorrect value for discardPendingUpdateAction and sending two post update events (one via runUpdateActions and other via discardPendingUpdateAction). This is because 1/ runUpdateActionsremoves the action from the map only after it is completed 2/ No locking used while accessing/modifying the map .
Use remove to ensure that the updated state is reflected across threads.
alter-mage
reviewed
Jul 11, 2024
src/main/java/com/aws/greengrass/lifecyclemanager/UpdateSystemPolicyService.java
Outdated
Show resolved
Hide resolved
alter-mage
reviewed
Jul 11, 2024
src/main/java/com/aws/greengrass/lifecyclemanager/UpdateSystemPolicyService.java
Outdated
Show resolved
Hide resolved
saranyailla
force-pushed
the
update-services
branch
2 times, most recently
from
7eb23e6
to
6894e18
Compare
saranyailla
force-pushed
the
update-services
branch
from
6894e18
to
cda58eb
Compare
|
Binary incompatibility detected for commit cda58eb. com.aws.greengrass.deployment.DeploymentConfigMerger is binary incompatible and is source incompatible because of CONSTRUCTOR_REMOVED Produced by binaryCompatability.py |
|
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against cda58eb |
|
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against cda58eb |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Description of changes:
Why is this change necessary:
How was this change tested:
Any additional information or context required to review the change:
Documentation Checklist:
Compatibility Checklist:
any deprecated method or type.
Refer to Compatibility Guidelines for more information.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.