-
Notifications
You must be signed in to change notification settings - Fork 261
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
core: validate transfer phase #92
Conversation
...les/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/ImageTransferUpdater.java
Outdated
Show resolved
Hide resolved
validTransitions.put(FINALIZING_SUCCESS, EnumSet.of(FINALIZING_FAILURE, FINISHED_SUCCESS)); | ||
validTransitions.put(FINALIZING_FAILURE, EnumSet.of(FINISHED_FAILURE)); | ||
validTransitions.put(FINALIZING_CLEANUP, EnumSet.of(FINISHED_CLEANUP)); | ||
validTransitions.put(FINISHED_CLEANUP, EnumSet.of(FINISHED_FAILURE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I'm not sure the valid transitions are correct, but the validator is nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I "guessed" this based on the code, I hope eventually the spec document will provide a clear understanding of which transitions are valid. For now, I guess it would be good enough to block transitions to FINALIZING_SUCCESS when a user incorrectly finalizes a transfer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is very risky - if you got the valid transitions wrong, this will break
valid code in engine trying to change the phase.
Another issue is failing user request which should succeed. For example:
More than one finalize:
- user ask to finalize succeeds, start finalize flow
- user ask to finalize again - fail
More than one cancel:
- user ask to cancel succeeds, start finalize flow
- user ask to cancel again - fail
The system will be much easier to use if calling finalize() or cacnel() more than
once is allowed and does not change anything.
So this cannot be done using this validation. I think adding flags like:
- finalized
- canceled
When we handle finalize/cancel request we can consider the flags before we
modify the phase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, this is a draft to showcase a possible direction for validating, and I am in no rush to merge this. I still need to add unit tests and test this extensively, but even before that, we need to complete the spec, at least transitions part.
Multiple user requests will not fail (same goes for internal phase updates), but won't change the phase. But I am not sure if that is correct all the time, maybe in some cases the entire entity update should be discarded...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problematic flow I see is this:
- User ask to finalize
- System change phase to finalizing success
- System change state to the next phase
- User ask to finalize
- System changes state to finalizing success
- System remain stuck in finalizing success
And same for cancel.
I could reproduce this issue with virt-v2v:
- virt-v2v ask to finalize without closing imageio socket
- engine waits for imageio to remove the ticket
- imageio times out, because of the open socket
- virt-v2v request times out before engine replies
- engine wrongly advance to the next phase after imageio timeout
- virt-v2v cancel the transfer because of the finalize timeout
- engine get stuck in finalizing failure
This is a combinations of bugs in virt-v2v, imageio and engine
that cannot happen now because virt-v2v does close the socket now
before calling finalize, and imageio does not time out when there
are not active requests, and will remove the ticket immediately, but
this can happen with another clients for other reasons.
To fix these issues, we must ensure that requests to finalize
or cancel are handled exactly once.
.../src/main/java/org/ovirt/engine/core/common/businessentities/storage/ImageTransferPhase.java
Show resolved
Hide resolved
bfc2f55
to
25ca005
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice approach
validTransitions.put(PAUSED_USER, EnumSet.of(RESUMING, CANCELLED_USER, CANCELLED_SYSTEM)); | ||
validTransitions.put(CANCELLED_USER, EnumSet.of(FINALIZING_CLEANUP)); | ||
validTransitions.put(CANCELLED_SYSTEM, EnumSet.of(FINALIZING_FAILURE)); | ||
validTransitions.put(FINALIZING_SUCCESS, EnumSet.of(FINALIZING_FAILURE, FINISHED_SUCCESS)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the possible transition from FINALIZING_SUCCESS to FINALIZING_FAILURE seems suspicious but yeah, as Nir commented below - I take your word on those transitions :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's based on the code :)
Lines 858 to 863 in 92824ff
if (isImageVerified) { | |
log.info("Image transfer '{}' was successful for {}", getCommandId(), getTransferDescription()); | |
setCommandStatus(CommandStatus.SUCCEEDED); | |
} else { | |
updateEntityPhase(ImageTransferPhase.FINALIZING_FAILURE); | |
} |
That said, the explicit transitions could very well be wrong
1bc13fc
to
7f9a793
Compare
e4a7fc3
to
6c0fd3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, minor comment
@@ -26,17 +26,39 @@ public enum ImageTransferPhase implements Identifiable { | |||
private String description; | |||
private static final Map<Integer, ImageTransferPhase> valueToPhase = new HashMap<>(); | |||
|
|||
// TODO: revisit this in the future to validate all transition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO: revisit this in the future to validate all transition | |
// TODO: revisit this in the future to validate all transitions |
6c0fd3d
to
3da0897
Compare
Do not update the phase if the incoming phase is invalid. This only affects external phase updates, when the user attempts to cancel or finalize a transfer. Bug-Url: bugzilla.redhat.com/2092816
3da0897
to
88d666f
Compare
/ost |
Do not update the phase if the incoming phase is invalid
Bug-Url: https://bugzilla.redhat.com/2092816