Skips running the updater if last update was good, a reboot isn't needed, and a config-specified delta isn't defined. #450
Conversation
launcher/sdw-launcher.py
Outdated
|
|
||
| DEFAULT_HOME = os.path.join(os.path.expanduser("~"), ".securedrop_launcher") | ||
| logger = "" |
|
Now that's what I call a test plan! Stepping through on review, will update as I go... |
config.json.example
Outdated
| @@ -4,7 +4,8 @@ | |||
| "hostname": "avgfxawdn6c3coe3.onion", | |||
| "key": "Il8Xas7uf6rjtc0LxYwhrx" | |||
| }, | |||
| "environment": "prod", | |||
| "environment": "prod", | |||
| "update_skip_delta": 28800, | |||
There was a problem hiding this comment.
What's the rationale to have this value user-facing? Perhaps we can override in config.json, but I think it would be more prudent to have the default be specified in Updater.py. Suppose that we want to change this default to a shorter period in the future, this would require an admin to configure their config.json and not be applied automatically.
If we ultimately decide that this config is necessary and useful, we should also validate the config.json value in https://github.com/freedomofpress/securedrop-workstation/blob/master/scripts/validate_config.py
There was a problem hiding this comment.
The rationale for doing so is to make it tweakable, per-instance and without a code push. The default is set in sdw-launcher.py, and is 0 sec, as the safest approach to updates is to run the update check by default. But I get your point that we might want to make it optional, and enforce a reasonable default in its absence. Happy to make the change if others agree. Will also add validation if we decide to keep it.
|
@conorsch - heads-up that as per offline chat, I removed the config.json option and associated build and salt configs. The only test plan change is to the last section - running the launcher script without any arguments will now cause it to test for a default update interval of 8 hours. |
There was a problem hiding this comment.
Ran through the detailed test plan. Mostly solid, except for the final section:
- run the command /opt/securedrop/launcher/sdw-launcher.py --skip-delta=0
- 🔴 confirm that the updater runs directly
- once the update completes, click Continue, then close the client
- in the Terminal, run the command /opt/securedrop/launcher/sdw-launcher.py --skip-delta=600
- 🔴 confirm that the client starts without the updater running
@zenmonkeykstop Can you confirm that test plan is accurate? For instance, if I set --skip-delta=1 rather than --skip-delta=0, then the updater runs. Judging by the conditional in the code, that makes sense to me.
Also, not a problem with the diff you're presenting here, but we should consider folding the Salt tasks for the launcher into the jinja conditional block for "dev" environment. Otherwise, we're managing those files both via RPM and Salt.
Given the potential conflicts with the logging config mentioned by @eloquence, perhaps we should merge #445 if it's ready to go.
launcher/tests/test_updater.py
Outdated
| (UpdateStatus.REBOOT_REQUIRED, True, False), | ||
| (UpdateStatus.UPDATES_FAILED, True, False) | ||
| ]) | ||
| # @mock.patch("Updater.last_required_reboot_performed", return_value=True) |
There was a problem hiding this comment.
Should comment on L855 be removed, or active?
There was a problem hiding this comment.
Removed, mocked in body of test. done.
launcher/sdw_updater_gui/Updater.py
Outdated
| sdlog.info("Update interval not expired, launching client.") | ||
| return False | ||
| else: | ||
| sdlog.info("Updates or reboot required, launching updater.") |
There was a problem hiding this comment.
As shown in the test plan, conditions can lead to "Updates or reboot required, launching updater". It might be helpful (to admins & support) to log the actual status code, and possibly timestamp.
There was a problem hiding this comment.
logger is configured to prepend a timestamp automatically. Added status code in message.
zenmonkeykstop
force-pushed
the
402-skip-updates-sometimes
branch
from
73808e1 to
c3cbec3
Compare
|
@conorsch the argument handling that caused the problem you spotted has been fixed. The test plan should now describe expected behavior. |
|
From reading the code, it looks like the |
|
@eloquence correct. The update time doesn't change, because the the updater hasn't run, and the status doesn't change until the updater runs next. if the |
I don't think this is the behavior we want. A stale I would propose the following:
Does that make sense, @zenmonkeykstop, @emkll & others? |
|
Seems reasonable @eloquence. Only worry is that it might take a bit of refactoring of the updater to to make that functionality available to the launcher script. Narrator: it did not |
zenmonkeykstop
force-pushed
the
402-skip-updates-sometimes
branch
from
e2fda14 to
bd979a6
Compare
|
@eloquence your requested changes are in. Minor changes to the test plan as some log messages have changed but otherwise unaffected. |
|
@zenmonkeykstop Thank you! :) To be clear, it was just a suggestion, and I defer to @emkll on whether this is the right approach. Thanks for updating the test plan as well! |
|
Yup, if he dissents we can back it out, but it makes sense to me. |
|
@zenmonkeykstop #445 is merged, which has introduced conflicts here. Mind rebasing and pinging for re-review? |
Also, please bump the |
zenmonkeykstop
force-pushed
the
402-skip-updates-sometimes
branch
from
bd979a6 to
25df3de
Compare
|
rebase and a quick spin through the test plan in progress. |
|
Ready for re-review! |
|
Since I'm fairly familiar with this code and the expected behavior, I'll take a spin through this as well (can't approve, but can give a functional sign-off from my end). |
| @@ -249,7 +249,7 @@ def run(self): | |||
| # write the flags to disk | |||
| run_results = Updater.overall_update_status(results) | |||
| Updater._write_updates_status_flag_to_disk(run_results) | |||
| if run_results == UpdateStatus.UPDATES_OK: | |||
| if run_results in {UpdateStatus.UPDATES_OK, UpdateStatus.REBOOT_REQUIRED}: | |||
There was a problem hiding this comment.
I would recommend adding a code comment above this line, explaining the behavior.
| @@ -94,6 +94,9 @@ find /srv/salt -maxdepth 1 -type f -iname '*.top' \ | |||
| | xargs qubesctl top.enable > /dev/null | |||
|
|
|||
| %changelog | |||
| * Fri Feb 14 2020 Kevin O Gorman <[email protected]> - 0.1.4 | |||
There was a problem hiding this comment.
Per @conorsch the recommendation for future changelog entries is to use "SecureDrop Team <[email protected]>" consistently
There was a problem hiding this comment.
Citation: freedomofpress/securedrop-builder#108
| ], | ||
| ) | ||
| @mock.patch("Updater._write_updates_status_flag_to_disk") | ||
| def test_should_run_updater_status_interval_expired( |
There was a problem hiding this comment.
This and test_should_run_updater_status_interval_not_expired are your most complex tests, and I think they would benefit from a few code comments/test descriptions to explain the test behavior. That said, after staring at them for 15 minutes or so, the behavior seems reasonable to me.
My personal intuition would be to put the expected value rightmost in each list, to make the lists a bit more readable (kind of like a truth table), but I don't know if there's a convention you're following here that goes another way.
|
Testing now. :) So we understand more clearly in future what was merged, could you update the PR description to reflect the current behavior and default interval (post |
launcher/sdw_updater_gui/Updater.py
Outdated
| return True | ||
| else: | ||
| sdlog.info( | ||
| "Update status is {}, launching updater.".format( |
There was a problem hiding this comment.
This results in log lines like "Update status is 1, launching updater" which is of course true, but is only useful to anyone familiar with the implementation internals -- I think for states we know the updater can enter, we should have corresponding human-readable log lines.
launcher/sdw-launcher.py
Outdated
| import logging | ||
| import sys | ||
| import argparse | ||
|
|
||
| DEFAULT_INTERVAL = 28800 |
There was a problem hiding this comment.
I always make a habit of annotating seconds values, i.e. # 8 hours in this case
I did not get the option to launch the client in this scenario, even though no updates were applied. Instead it prompted me to reboot. I think that is the expected behavior currently. I'm wondering if we should even check for updates if the user runs the updater before rebooting. It seems more reasonable to immediately advance to the "Reboot required" screen. I don't think that needs to be done in the same PR, however, as we might want to think through the UX for that case a bit more (e.g., maybe the screen we show in that case should be a bit different). |
|
Ypu that is the desired behaviour, will update test plan. As for the second part, I agree that it's desirable, but also that it's probably out of scope. |
launcher/sdw_updater_gui/Updater.py
Outdated
| @@ -430,6 +430,65 @@ def _safely_start_vm(vm): | |||
| sdlog.error(str(e)) | |||
|
|
|||
|
|
|||
| def should_launch_updater(interval): | |||
| sdlog.info("Starting SecureDrop Launcher") | |||
There was a problem hiding this comment.
This results in a duplicate log line (since the same message is logged from main), is that intentional?
There was a problem hiding this comment.
nope, merge cruft. will delete.
|
Excellent test plan, and everything working as expected (I ticked the missing boxes); all my comments are minor except for that behavior issue re: reboot case, which I'll file a new issue for. Given that we're also altering the
Note that for the purposes of this test, no actual updates were available, and I modified |
|
Updated the unit tests to also check for the sdw-update-status file being written (as there's precisely one case (REBOOT_REQUIRED + reboot performed) where that happens, so there's that. The code writing the sdw-last-updated flags is in UpdaterApp.py, which currently does not have a coverage target being all UIy and threaded, tho @emkll has expressed an interest in getting test coverage there as well. If that gets some testing love in the future, that would be the place to test. |
|
|
||
| args = parse_argv(argv) | ||
|
|
||
| try: |
There was a problem hiding this comment.
Perhaps setting the default in argparse will simplify the logic here: https://docs.python.org/3.7/library/argparse.html#default
There was a problem hiding this comment.
Went through the comprehensive test plan, functional testing looks good to me. Two high level comments that shouldn't block merge:
--skip-deltaparameter to override the interval may not be immediately necessary, increasing the complexity of untested code paths (see inline for potential optimization).- existing tests were rewritten/modified for style reasons (they were formatted by
black, but fine to change sinceblack --checkis not complaining)
Since @conorsch previously changes, will let him take a final look prior to merge.
Other tests
- invalid/unparseble
sdw-update-statusfile starts the updater - updater.py test coverage is 100%
Test plan from PR
testing initial and second run
- double-click the SecureDrop shortcut
- confirm that the updater is launched
- confirm that the logfile at
~/.securedrop_launcher/logs/launcher.logcontains the line:
INFO: Update status not available, launching updater. - complete the update check, applying updates and rebooting if necessary. If a reboot is not necessary, click Continue to launch the client, then close the client
- double-click the SecureDrop shortcut
- confirm that the client is launched directly without the updater starting
- confirm that the logfile above contains the line:
INFO: Updates OK and interval not expired, launching client. - close the client
testing updates_required status
- update the dom0 JSON file
~/.securedrop_launcher/sdw-update-status, changing the value of thestatusfield to"1". - double-click the SecureDrop shortcut
- confirm that the updater is launched
- confirm that the logfile at
~/.securedrop_launcher/logs/launcher.logcontains the line:
INFO: Update status is 1, launching updater. - after updates are complete, click Continue, then close the client
testing reboot_required status (without reboot having been performed)
- update the dom0 JSON file
~/.securedrop_launcher/sdw-update-status, changing the value of thestatusfield to"2". - double-click the SecureDrop shortcut
- confirm that the updater is launched
- confirm that the logfile at
~/.securedrop_launcher/logs/launcher.logcontains the line:
INFO: Required reboot pending, launching updater - after updates are complete, click Continue, then close the client
testing updates_failed status
- update the dom0 JSON file
~/.securedrop_launcher/sdw-update-status, changing the value of thestatusfield to"3". - double-click the SecureDrop shortcut
- confirm that the updater is launched
- confirm that the logfile at
~/.securedrop_launcher/logs/launcher.logcontains the line:
INFO: Update status is 3, launching updater. - after updates are complete, click Reboot
testing reboot_required status (with required reboot)
- update the dom0 JSON file
~/.securedrop_launcher/sdw-update-status, changing the value of thestatusfield to"2". - reboot Qubes.
- double-click the SecureDrop shortcut
- confirm that the client is launched
- confirm that the logfile at
~/.securedrop_launcher/logs/launcher.logcontains the line:
INFO: Required reboot performed, updating status and launching client.. - confirm that the
sdw-update-statusfile has been updated with status0and a new timestamp - close the client
testing interval expiry
- open a dom0 Terminal
- run the command
/opt/securedrop/launcher/sdw-launcher.py- confirm that the client starts without the updater running
- run the command
/opt/securedrop/launcher/sdw-launcher.py --skip-delta=0- confirm that the updater runs directly
- once the update completes, click Continue, then close the client
- in the Terminal, run the command
/opt/securedrop/launcher/sdw-launcher.py --skip-delta=600- confirm that the client starts without the updater running
- wait a little over 10 seconds, then, in the Terminal, run the command
/opt/securedrop/launcher/sdw-launcher.py --skip-delta=10- confirm that the updater runs.
|
Latest diff is looking good. @emkll has given a detailed functional review. There are a few opportunities to prune codepaths, but these files are quite hot now given the approach of the pilot, so merging to keep moving. |
Skips running the updater if last update was good, a reboot isn't needed, and a config-specified delta isn't defined.
Status
Ready for review
Description of Changes
Fixes #402.
--skip-delta=<update_skip_delta>--skip-deltaparameter above. If the last update was successful and less thanupdate_skip_deltaseconds ago, then the launcher runs the SecureDrop client directly. Otherwise, it runs the updater. if--skip-deltais omitted, a default interval of 8 hours is assumed. If--skip-deltais 0, the updater always runs.Testing
On this branch:
update_skip_deltafield in config.json)/opt/securedrop/launcher/sdw-launcher.py --skip-delta=28800~/.securedrop_launcherif it existstesting initial and second run
~/.securedrop_launcher/logs/launcher.logcontains the line:testing updates_required status
~/.securedrop_launcher/sdw-update-status, changing the value of thestatusfield to"1".~/.securedrop_launcher/logs/launcher.logcontains the line:testing reboot_required status (without reboot having been performed)
~/.securedrop_launcher/sdw-update-status, changing the value of thestatusfield to"2".~/.securedrop_launcher/logs/launcher.logcontains the line:testing updates_failed status
~/.securedrop_launcher/sdw-update-status, changing the value of thestatusfield to"3".~/.securedrop_launcher/logs/launcher.logcontains the line:testing reboot_required status (with required reboot)
~/.securedrop_launcher/sdw-update-status, changing the value of thestatusfield to"2".~/.securedrop_launcher/logs/launcher.logcontains the line:sdw-update-statusfile has been updated with status0and a new timestamptesting interval expiry
/opt/securedrop/launcher/sdw-launcher.py/opt/securedrop/launcher/sdw-launcher.py --skip-delta=0/opt/securedrop/launcher/sdw-launcher.py --skip-delta=600/opt/securedrop/launcher/sdw-launcher.py --skip-delta=10Checklist
If you have made changes to the provisioning logic
Linting (
make flake8) and tests (make test) pass in dom0 of a Qubes installmake testfails an unrelated qubes-rpc test due to system setupI have added/removed files, and have updated packaging logic in MANIFEST.in and rpm-build/SPECS/securedrop-workstation-dom0-config.spec