Implement Abort() API call #1122
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1122 +/- ##
==========================================
- Coverage 77.82% 77.75% -0.07%
==========================================
Files 167 169 +2
Lines 10001 10004 +3
==========================================
- Hits 7783 7779 -4
- Misses 2218 2225 +7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Did a more thorough review today. Mostly looks fine. So the point of the token is to have one object to represent the API state, right? For some reason it took me a minute to make sense out of that, but it seems fine to me now. I still haven't played around with this, but the easiest way to do that is with hmi_stub, which is lacking the Abort function. Once that is there, I'd like to try it out and make sure it behaves as expected.
| * but the Aktualizr will remain in the paused state. To continue execution | ||
| * at some later point one needs to call Resume(). | ||
| */ | ||
| void Abort(); |
There was a problem hiding this comment.
Is it worth mentioning that the only command that can be interrupted while in progress is downloading?
There was a problem hiding this comment.
I'm not so sure if it's worth mentioning here. It's unlikely, but what if we decide to abort e.g. CheckForUpdates at some later point?
There was a problem hiding this comment.
It's not strictly necessary, but when I read "Requests the currently running command to abort", that leads me to assume that all commands can be aborted while in progress, and that isn't true. If it later becomes true, or even if just one more command is abortable, we can change the doc here again to reflect that as necessary.
There was a problem hiding this comment.
Agreed, sounds confusing. I'll rephrase it.
|
Thanks for the review, @patrickvacek! I'll fix the mentioned issues soon. |
Great, that was my understanding. That is perfect. The abstraction was confusing to me at first, but it is obviously superior to me now that I understand it better. |
eu-siemann
force-pushed
the
feat/abort
branch
4 times, most recently
from
23a7b00 to
a0be831
Compare
Squashed based on commit: 9076faf advancedtelematic#1122 Signed-off-by: Eugene Smirnov <[email protected]>
eu-siemann
force-pushed
the
feat/abort
branch
6 times, most recently
from
9715ed2 to
402be5d
Compare
| self.send_response_only(200) | ||
| else: | ||
| self.send_response_only(404) | ||
| self.end_headers() |
There was a problem hiding this comment.
do_HEAD looks to be duplicated.
| try: | ||
| httpd.serve_forever() | ||
| except KeyboardInterrupt as k: | ||
| with ExitStack() as stack: |
There was a problem hiding this comment.
What benefit does the ExitStack and sig_handler provide? Just curious, not doubting. :)
There was a problem hiding this comment.
sys.exit(0) in signal handler will raise a SystemExit exception, which will unwind the ExitStack cleaning up the tmp directory
Signed-off-by: Eugene Smirnov <[email protected]>
Signed-off-by: Eugene Smirnov <[email protected]>
Signed-off-by: Eugene Smirnov <[email protected]>
Signed-off-by: Eugene Smirnov <[email protected]>
Signed-off-by: Eugene Smirnov <[email protected]>
Signed-off-by: Eugene Smirnov <[email protected]>
Signed-off-by: Eugene Smirnov <[email protected]>
Signed-off-by: Eugene Smirnov <[email protected]>
The main idea of this PR is to provide an ability to abort ongoing downloads and reset the queue to an empty state. It also removes Shutdown() call, which was bringing an Aktualizr object to an unusable state and fixes the issue with a possible race condition between Pause()/Resume() calls.
The PR still lacks tests for the Abort() functionality and more proper handling of an abort event in fetcher and uptane client.
At least it doesn't seem to break existing functionality and I feel that it's already getting quite big,
so it would be great if you could have a look at it next week.