Skip to content
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

Bump supported Fedora release to 37 #859

Merged
merged 6 commits into from
Mar 30, 2023
Merged

Bump supported Fedora release to 37 #859

merged 6 commits into from
Mar 30, 2023

Conversation

eaon
Copy link
Contributor

@eaon eaon commented Feb 27, 2023

Description of Changes

Fedora 37 will be shipped with Qubes OS R4.1.2. As soon as Qubes R4.1.2 has been released, this should probably be merged so that the next release doesn't download Fedora 36 unnecessarily.

Fixes #841

Testing

  • There are no references to Fedora 36 left in the repository
  • sd-fedora-37-dvm is used in place of sd-fedora-dvm through the repo (except for the salt state removing the latter)
  • Check out this branch in your sd-dev vm and ensure you set the SDW environment variables. Ensure SD VMs (lookin at you, sd-whonix) are all shut down (otherwise, during make test, you could encounter tests that report that VMs aren't fully updated). In dom0 make clone then make dev
    • make dev completes without error
    • sd-fedora-37-dvm exists and is based on fedora-37
    • sd-fedora-dvm is removed
    • make test passes in dom0

Note that removing the f36 template/f36-dvm template is not part of the template update process.

@eaon eaon marked this pull request as draft February 27, 2023 18:56
@eaon eaon self-assigned this Mar 1, 2023
@eloquence
Copy link
Member

(I've taken the liberty to push a small commit to update the dom0 tests to the new template version.)

@eaon eaon marked this pull request as ready for review March 7, 2023 13:16
@eaon
Copy link
Contributor Author

eaon commented Mar 7, 2023

The Fedora 37 template is available now so technically this is already ready for review.

@eloquence
Copy link
Member

I consistently get the following error when attempting to migrate from fedora-36:

qubesadmin.exc.QubesVMNotHaltedError: Cannot change template while there are running DispVMs based on this DVM template

To test the migration, I built an RPM from the branch, installed it in dom0, and then ran the updater with /opt/securedrop/launcher/sdw-launcher.py, including once after a clean reboot. (I wanted to see the migration behavior a user would see.)

eaon and others added 2 commits March 14, 2023 18:11
Fedora 37 template was released on 2023-03-03, and will be shipped by
default by Qubes OS R4.1.2.

For upgrades, we also need to ensure we're not changing the template
for sd-fedora-dvm before the appropriate time.
@eaon eaon marked this pull request as ready for review March 14, 2023 17:13
@eaon eaon requested review from eloquence and rocodes March 14, 2023 17:13
Copy link
Member

@eloquence eloquence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working through this! Unfortunately I'm still hitting an error during the migration. That said, it looks like the migration did complete -- the non-disposable sys- templates are using fedora-37, and sd-fedora-dvm uses the new template. 🎉

This Gist has the Salt error I'm getting: https://gist.github.com/eloquence/e5bca6fcd12ed6d7b7364c180bfe8bac

You may have an immediate hypothesis on what's causing this, but I'll leave one note at the line level on a part of the logic that's confusing to me.

sd-{{ sys_vm }}-fedora-version-update:
qvm.vm:
- name: {{ sys_vm }}
- prefs:
- template: {{ sd_supported_fedora_template }}
- require:
- cmd: sd-{{ sys_vm }}-fedora-version-halt-wait
{% if sd_supported_fedora_template.endswith("-dvm") %}
{% if not dvm_needs_updating and sd_supported_fedora_template.endswith("-dvm") %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by the not dvm_needs_updating addition here -- what's the purpose of this additional check?

That said, what appears to be happening on my system is that this evaluates to true, but the create- block itself is never defined, because the VM is already present and the {% if required_dispvm not in existing_vms %} block evaluates to false.

@rocodes rocodes assigned rocodes and unassigned rocodes Mar 15, 2023
@eaon
Copy link
Contributor Author

eaon commented Mar 20, 2023

I'm assuming you ran everything again without doing any manual Fedora 37 related cleaning operations, correct? If that's the case, your machine is in a state that the current patch would not get to by itself, which is also why it's not covered. That being said, I think we shouldn't assume that SDW is used in a completely isolated environment, power users may have done some manual template updating, so I'll take another look.

Just to spell out the permutations of cases we would run into:

  • Fresh install: SDW is not installed, neither fedora-37 nor sd-fedora-dvm exist
  • Regular upgrade: SDW is installed, fedora-37 does not exist, sd-fedora-dvm needs updating
  • Manual intervention or previous broken run: SDW is installed, fedora-37 exists, sd-fedora-dvm needs updating
  • All up-to-date: SDW is installed, fedora-37 exists, sd-fedora-dvm does not need updating

@eloquence
Copy link
Member

Yes, that's correct, I did not manually remove fedora-37 before trying again. What's odd about my run, though, is that it seemed to eventually fit the "All up-to-date" description, but still kept failing with the error above.

The only other state I can think of is with Qubes 4.1.2, which was released last week. A fresh install on 4.1.2 would be:

  • Fresh install: SDW is not installed, fedora-37 exists but sd-fedora-dvm does not.

@rocodes
Copy link
Contributor

rocodes commented Mar 21, 2023

(Stepping through now to put some more eyes on this, will first try with no fedora-37 preinstall aka "regular update" scenario, then with fedora-37 template preinstalled aka "manual intervention or previous broken run")

Shout out to @gonzalo-bulnes for bringing this up as a potential
solution
@eaon
Copy link
Contributor Author

eaon commented Mar 22, 2023

I pushed some changes after a note from @gonzalo-bulnes came to mind again.

I have not tested this, but I think this should cover all cases including the one @eloquence pointed out.

This is all significantly more complex than https://github.com/freedomofpress/securedrop-workstation/pull/802/files in part due what I've introduced with #764 and #784. I consider the numbered approach now taken by this PR a workaround as removing and recreating an AppVM with the same contents wastes time and resources. However, this is a restriction of our deployment strategy which is IMO in dire need of fundamental restructuring. More notes on the direction I think would remove classes of pain-points is documented in https://github.com/freedomofpress/securedrop-engineering/pull/22

@rocodes rocodes self-assigned this Mar 27, 2023
@zenmonkeykstop
Copy link
Contributor

zenmonkeykstop commented Mar 27, 2023

Given 4.1.2 is out and recommended, the fresh install case should actually be "SDW not installed, f37 exists, sd-fedora-dvm doesn't exist." For update cases we still have to allow for the 4.1.1 cases.

@eloquence
Copy link
Member

eloquence commented Mar 28, 2023

On my system which has fedora-37 already installed, I am still encountering the error below. Note that fedora-37-dvm also exists on this system. This is why I think this Jinja check fails to include the create-fedora-37 step, which is then later required.

I'm going to try removing the fedora-37-dvm AppVM to see if that causes a successful run.

----------
          ID: sd-sys-firewall-fedora-version-update
    Function: qvm.vm
        Name: sys-firewall
      Result: False
     Comment: The following requisites were not found:
                                 require:
                                     qvm: create-fedora-37-dvm
     Started: 18:33:02.926407
    Duration: 0.002 ms
     Changes:   
----------
          ID: sd-sys-firewall-fedora-version-start
    Function: qvm.start
        Name: sys-firewall
      Result: False
     Comment: One or more requisite failed: sd-sys-vms.sd-sys-firewall-fedora-version-update
     Started: 18:33:02.926714
    Duration: 0.003 ms
     Changes:   

@eloquence
Copy link
Member

eloquence commented Mar 29, 2023

Yes, deleting fedora-37-dvm resulted in a successful run with all expected updates completed. 🎉

Repeated runs after that also complete without errors.

I think this is an edge case that resulted from a partially completed upgrade. I don't think you would encounter it on a Qubes 4.1.2 system with fedora-37-dvm already created, because such a system would also have sys-firewall already set to fedora-37.

If we haven't tested that already, I think that would be useful to verify. Assuming that is the case, I think addressing this issue would be mostly a matter of overall resilience.

@rocodes
Copy link
Contributor

rocodes commented Mar 29, 2023

Thanks for the sleuthing - you're right, I thought initially you were encountering the issue after only having downloaded the f37 template (as opposed to having set up the dvm as well).

This is fixable, thanks for flagging.

@rocodes
Copy link
Contributor

rocodes commented Mar 29, 2023

(A little bit annoying to remove the jinja optimization that was in there, but for now it's fine. We'll want to revisit our overall strategy here, as was suggested earlier)

@rocodes rocodes requested review from eloquence and a team March 29, 2023 21:12
@rocodes
Copy link
Contributor

rocodes commented Mar 29, 2023

I'm stepping through one last time for good measure on a brand new 4.1.2 install. If someone wants to review on an existing 4.1 workstation by following the original test plan, that would be great (since I'm the last one to push some code I shouldn't review).

Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without any fedora-37 yet installed, and with the caveat that this test plan wasn't written when I started the make clone && sdw-admin --apply sequence :-):

  • Check out this branch in your sd-dev vm and ensure you set the SDW environment variables. Ensure SD VMs (lookin at you, sd-whonix) are all shut down (otherwise, during make test, you could encounter tests that report that VMs aren't fully updated). In dom0 make clone then make dev sdw-admin --apply
    • make dev sdw-admin-apply completes without error
    • sd-fedora-37-dvm exists and is based on fedora-37
    • sd-fedora-dvm is removed
    • make test passes in dom0
      • ...modulo qrexec policy changes I've made because this is my lowercase-W workstation too... :-)
      • ...with one meaningful exception:

EXPECTED_KERNEL_VERSION = "5.15.41-grsec-workstation"

[user@dom0 securedrop-workstation]$ qvm-run --pass-io sd-app uname -r
5.15.89-grsec-workstation

@rocodes rocodes dismissed eloquence’s stale review March 30, 2023 00:32

later changes address this issue

@rocodes
Copy link
Contributor

rocodes commented Mar 30, 2023

@cfm I still have the 5.15.41 grsec kernel when I've only ever built from dev on the SDW laptop. I'm guessing you had this machine on staging or prod before, since we don't do nightly kernel builds - that would explain the test failure.

Thank you for the review :) I will finish clean install QA then merge.

Copy link
Member

@eloquence eloquence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't do a full re-run, but did do another make dev with latest changes and all LGTM, so adding another +1 for good measure :)

@rocodes
Copy link
Contributor

rocodes commented Mar 30, 2023

Thanks both. Tested on fresh 4.1.2 install; fedora 37 template handled just fine, although the first time, the installation errored out because our own template (workstation-bullseye) was not successfully downloaded. It was successful on a second run. Will keep an eye out for that issue in QA, but I think we're OK to proceed here.

@rocodes rocodes merged commit 67a125e into main Mar 30, 2023
@legoktm legoktm deleted the fedora-37 branch May 28, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to Fedora 37
5 participants