-
Notifications
You must be signed in to change notification settings - Fork 687
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
Updates developer docs for upgrade testing targets #3832
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3832 +/- ##
=======================================
Coverage 84.7% 84.7%
=======================================
Files 43 43
Lines 2765 2765
Branches 300 300
=======================================
Hits 2342 2342
Misses 355 355
Partials 68 68 Continue to review full report at Codecov.
|
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.
Looks good, with the exception of one point re: upgrades using apt-test.
docs/development/upgrade_testing.rst
Outdated
create the environment: | ||
|
||
.. code:: sh | ||
|
||
make build-debs | ||
molecule converge -s upgrade | ||
make upgrade-start |
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 step will fail unless there are previously-built Debian packages for Molecule to copy to the apt server and sign. The "Copy over debs" task succeeds even though build/ is empty, but "Sign release file" fails. Since the packages aren't actually needed when upgrading from apt-test, maybe the "Sign release file" task should just check for the existence of a Release file first and do nothing if it doesn't exist.
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.
Yeppppp @zenmonkeykstop is totally right here.. that logic be busted :| Ill try to push a fix for that real quick. I like how quickly the tides of this PR changes from nits+docs --> bugfixes. ❤️
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 step will fail unless there are previously-built Debian packages for Molecule to copy to the apt server
Just ran into this myself in a clean working directory. We can pilfer the find-debs logic used in the staging logic and fail fast if the build/
directory is empty. Will append a commit.
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.
Currently failing install in two diff scenarios:
1 - The change to upgrade-test-qa
target is breaking because you need to pass SD_UPGRADE_BASE
to any converge. I'll push a fix for that.. easy peasy
2 - The issue @zenmonkeykstop brings up seems to be an existing issue... this opens up a can of worms. I was mid opening a ticket for this but I realized the root of the issue is the divergence of workflow between the local dpkg
and remote qa
workflows is dramatically different.. maybe we should just create two sets of start make commands here?
upgrade_test_qa: ## Once an upgrade environment is running, force upgrade apt packages (from qa server) | ||
.PHONY: upgrade-test-qa | ||
upgrade-test-qa: ## Once an upgrade environment is running, force upgrade apt packages (from qa server) | ||
@QA_APTTEST=yes molecule converge -s upgrade -- --diff -t apt |
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.
Found a bug here.. fixing with an incoming commit
docs/development/upgrade_testing.rst
Outdated
create the environment: | ||
|
||
.. code:: sh | ||
|
||
make build-debs | ||
molecule converge -s upgrade | ||
make upgrade-start |
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.
Yeppppp @zenmonkeykstop is totally right here.. that logic be busted :| Ill try to push a fix for that real quick. I like how quickly the tides of this PR changes from nits+docs --> bugfixes. ❤️
i was about to push a fix but then realized this opens a discussion here that needs addressing... to break it down - there is a divergence between the The method now described in the docs opens up a bug as @zenmonkeykstop pointed out - specifically when the build dir is empty. So the quickest fix is to add a parallel make target something like I was about to push this fix: diff --git a/Makefile b/Makefile
index 6b6fd3b1d..a0ba33530 100644
--- a/Makefile
+++ b/Makefile
@@ -171,6 +171,10 @@ clean: ## DANGER! Purges all site-specific info and developer files from project
upgrade-start: ## Boot up an upgrade test base environment using libvirt
@SD_UPGRADE_BASE=$(STABLE_VER) molecule converge -s upgrade
+.PHONY: upgrade-start-qa
+upgrade-start-qa: ## Boot up an upgrade test base env using libvirt in remote apt mode
+ @SD_UPGRADE_BASE=$(STABLE_VER) QA_APTTEST=yes molecule converge -s upgrade
+
.PHONY: upgrade-destroy
upgrade-destroy: ## Destroy up an upgrade test base environment
@SD_UPGRADE_BASE=$(STABLE_VER) molecule destroy -s upgrade The issue with the currently documented change to the make target of |
Sorry to clarify - that bug was already present thanks to yours truly 👍 @zenmonkeykstop just pointed out a short-coming I made by not adding a parallel QA make target to address that bug. |
02a0574
to
4db5d39
Compare
4db5d39
to
6c837ff
Compare
Changes implemented via provided patch
Ready for review. Implemented the changes flagged by both @msheiny and @zenmonkeykstop . The most recent docs additions also close #3998. Note that I did not save a box for v0.10.0 of SD (see commit message for details), but we now have a 0.11.0 box live to aid in upgrade testing for 0.12.0. |
This is consistently failing for me with the 0.11.0 boxes, at the |
That's no fun. Thanks for report, @zenmonkeykstop — I will purge my local boxes and pull down fresh to see if I can reproduce. |
No problem - fwiw I updated the version in |
That's a bug, I had intended to remove the 0.10.0 metadata references (which would cause fetching 0.10.0 boxes to fail) since we missed building boxes for 0.10.0 during the release, and manually cobbling together a box for 0.10.0 bumped into the timebox I'd set. Will follow up excising that. |
Stylistic nit. All the Makefile targets use hyphens, rather than underscores, to the "upgrade" targets were inconsistent. Partly I want to avoid holding shift where possible, but more important, I don't want to have to remember when I do and do not have to use shift for autocomplete to work.
The Makefile logic wasn't automatically converting to the apt-test repo; developers still had to run an ad-hoc Molecule command from the docs in order to convert from local debs to the apt-test proxy. Let's make that automatic, as part of the "upgrade-test-qa" Makefile target.
We have convenient Makefile targets for the various upgrade testing scenario flows: * upgrade-start * upgrade-test-local * upgrade-test-qa Let's use those new targets in the docs where appropriate, to streamline the process for developers using the scenarios locally.
We missed updating the upgrade boxes during post-release procedure for both 0.10.0 and 0.11.0. Since 0.11.0 is still the most current release, it was rather straightforward to prepare a box. Catching up with 0.10.0 was trickier, however, since the box preparation logic assumes that whatever version is posted to the prod apt repo is the correct one to prepare for; no longer the case. As such, I'm omitting metadata for the 0.10.0 version. Since I did prepare and upload those boxes before realizing the 0.10/0.11 version discrepancy, here are the checksums of what's stored in S3: { "version": "0.10.0", "providers": [ { "name": "libvirt", "url": "https://s3.amazonaws.com/securedrop-vagrant/app-staging_0.10.0.box", "checksum_type": "sha256", "checksum": "2dda15d0f2557dd434dfba4cf0b3ab58b30666ac0277cd8d32b973d954b54264" } ] } { "version": "0.10.0", "providers": [ { "name": "libvirt", "url": "https://s3.amazonaws.com/securedrop-vagrant/mon-staging_0.10.0.box", "checksum_type": "sha256", "checksum": "6e281590a7dfd7753136b7c6505c8dd1ebc4d7f8207c890a80db4871d6ac08b7" } ] }
Conferred with @msheiny on the manual steps. Ideally we'd automate even these, via inspecting checksums and modifying the JSON file programatically, but the manual changes are only necessary after each SD release, which isn't a terrible burden right now.
If the upgrade scenario is used with the local debs strategy (i.e., QA_APT_TEST env var is not set), we should fail fast without the required local deb files. Previously, the scenario would proceed all the way to the local apt server configuration and only fail when attempting to sign the Release file with a test key. Now, the role will fail almost immediately without local debs present. Hat tip to @zenmonkeykstop for flagging during review.
Pointed out by @msheiny during review, so lifting the patch from PR discussion and implementing to resolve. The goal is to ensure that the "use apt-test repo" env var is declared during the "create" phase of the scenario, which is now handled.
9303a4d
to
cadd878
Compare
@zenmonkeykstop please try again. I've removed the metadata reference to the 0.10.0 box, since it had 0.11.0 installed, as you pointed out. I did a rebase, squash, and force push, because I'd already written a commit message indicating that 0.10.0 wasn't include—that message is now accurate! As for the 0.11.0 failure, I'm unable to reproduce that problem. After purging the 0.11.0 boxes from my host, I'm able to run
Note that you may need to |
I purged boxes as above and saw the same error, but deleting the images in question from /var/lib/libvirtd/images and restarting libvirtd did the trick! (There's almost certainly a cleaner way to do this with virsh.) So I'm inclined to chalk this up to getting a dodgy version of the 0.11.0 mon box at some point. With fresh boxes, local and apt-test upgrades work as expected. LGTM! |
Thanks for re-review, @zenmonkeykstop! My mistake on recommending the @kushaldas Can you take a spin through these docs and add your review? If you approve, as well, then it's good to go in. We may also be able to close #4025 in tandem, as well. |
While trying to execute the command:
It is still stuck at this error, so I am waiting to see what will happen next. |
The latest try failed at this output.
Also, I can only the @conorsch Please let me know if I am missing something before running |
@kushaldas The error message states |
|
What commit are you working from? The 0.11.0 metadata will only be present if you work from this branch. Post the sha sum of the commit you're on and I'll try to reproduce. |
@conorsch this is the latest commit. |
Finally some new errors @conorsch please have a look
I can see more update:
|
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.
Finally things are working for me after @emkll told me the magic line of destroying the scenarios. The PR looks good. Though, I would love to see a future PR (or a new commit here) explaining how to destroy any borked instance.
Tried both test-local and test-qa scenarios.
Good flag, @kushaldas. I'm going to merge this, please feel free to follow-up with a clarification fix adding the changes you propose! |
Status
Ready for review / Work in progress
Description of Changes
Follow-up to #3819. Changes proposed in this pull request:
make upgrade-test-qa
target to force use of apt-test proxyIt's now possible to run full upgrade testing, end to end, with two commands:
make upgrade-start
make upgrade-test-qa
Pushed up new debs to apt-test, built from 71ca2ed (the latest
develop
at time of writing).Also closes #3998, by documenting the maintenance procedures around preparing new boxes.
Testing
Read the new docs and review for clarity. @msheiny in particular should sanity check the new
make upgrade-test-qa
logic, but it seems reasonable to me.Deployment
Any special considerations for deployment? Consider both:
Checklist
If you made changes to the server application code:
make ci-lint
) and tests (make -C securedrop test
) pass in the development containerIf you made changes to
securedrop-admin
:make -C admin test
) pass in the admin development containerIf you made changes to the system configuration:
If you made non-trivial code changes:
If you made changes to documentation:
make docs-lint
) passed locally