Move provision-all to sdw-admin.py #1159
Merged
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
rocodes
reviewed
Aug 13, 2024
files/sdw-admin.py
Outdated
|
|
||
| print("Provisioning complete. Please reboot to complete the installation.") | ||
|
|
||
| def qubesctl_call(step_description: str, args: List[str]): |
There was a problem hiding this comment.
(You might want a skip_dom0 parameter set to True, that can optionally be overridden, but that fails "safe" and defaults to skipping dom0)
|
Thanks @deeplow - took a quick look through, have not tested it out since you mention WIP + draft mode, but very happy to do so if and when. A couple thoughts:
In case it's helpful: I also have some related work in progress from the retreat (my branch - very much WIP/probably not runnable yet) - there, I took the approach of "every qubesctl call is a subprocess call" just in the interests of focusing on the gui installer part, but it would be quite easy to integrate your changes there if and when. :) |
|
Thanks for taking a look. I was already planning to address all of those kinds of issues. But on the first commit / pass I just wanted to copy line by line almost literally to ensure I was not mistranslated some command.
I did briefly try that, but unfortunately QubesVM.shutdown() is missing a |
Feel free to copy what Kev+I did in https://github.com/freedomofpress/securedrop-workstation-ci/blob/218b571d3047eeee53e391245e4077b82bb6790c/dom0/runner.py#L71 |
deeplow
force-pushed
the
539-provision-all-py
branch
from
93d45fe to
628f714
Compare
Made some progress on this (I hope) by grouping together provisioning and configuration calls. Provisioning targets dom0 and configuration targets regular qubes. This way there's no way of mixing those up. On top of that that we call certain functions |
|
Just in case, I ran now |
Better group sdw-admin apply-related functions into:
- PROVISION: creating VMs and setting properties. These states
are usually fast to apply since they don't require starting
any VMs. Exclusively targets dom0.
- CONFIGURE: uses salt to configure individual VMs. Does not
target dom0.
Going forward, our aim is to reduce the ammount of configuration
code that we have and this gives us a visual indication of where
these salt calls are.
deeplow
force-pushed
the
539-provision-all-py
branch
from
628f714 to
c1fbe3c
Compare
Thanks. It turns out that even the upstream implementation of I don't like calling sub-processes either and I avoid using internal APIs, but I wonder if this is fair game: from qubesadmin.tools.qvm_shutdown import main as qvm_shutdown
qvm_shutdown("sd-app", "--wait")Semantically it should be the same as |
I do have some thoughts. I'd say unless we know exactly the circumstances where to conditionally avoid printing output, or unless I have some oversight on the security implications of |
|
I was trying to find a better approach to the sync-appmenus step. But I couldn't find a more suitable approach, at least for as long as we have separate provision and configuration steps (which is something we want). With that, I'm marking this as ready for review. |
cfm
previously approved these changes
Sep 11, 2024
|
(It also occurs to me that provisioning versus configuration is emerging as a clear architectural principle we should document, including in how it's implemented in this script. :-) |
|
@cfm thanks for the review. Added your line and waiting for re-approval. |
Co-authored-by: Cory Francis Myers <cfm@panix.com>
deeplow
force-pushed
the
539-provision-all-py
branch
from
eaa5015 to
507b903
Compare
|
(ammended commit to comply with lintter) |
cfm
approved these changes
Sep 12, 2024
There was a problem hiding this comment.
Thanks, @deeplow. Approving after 507b903 based on #1159 (review).
Since this is a high-impact change and the above test plan was unchecked (not sure if missed @cfm) or just forgotten to uncheck, I decided to do it again just for due diligence. I compared main to this PR. The diff is only:
These are all expected :) diff provision-all-bash.txt provision-all-python.txt |
45 tasks
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.
Status
Work in progress - this is just a literal conversion. I will review to ensure none of the semantics were lost and pythonize some of the functions, where appropriate.
Description of Changes
Fixes #539
Changes proposed in this pull request:
Testing
Successful case:
make devand compare the output tomake devfrom main. Only debugging information should differ (something similar to what I did here)make testFailure injection:
Deployment
Any special considerations for deployment?
Not that I can think of. If anything, it should improve deployment in case of failures. I didn't find any references to
provision-allother than the ones modified, not even in the docs. But it is always possible some developer's workflows will break if they usedprovision-all.Checklist
If you have made changes to the provisioning logic
make test) pass indom0If you have added or removed files
MANIFEST.inandrpm-build/SPECS/securedrop-workstation-dom0-config.specIf documentation is required