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

manifest: Include systemd-resolved for F33 change #575

Closed
wants to merge 1 commit into from

Conversation

jdoss
Copy link
Contributor

@jdoss jdoss commented Aug 21, 2020

This PR adds the systemd-resolved packages into FCOS. It is going to be used by default in Fedora 33 per this change request: https://fedoraproject.org/wiki/Changes/systemd-resolved

Adding the systemd-resolved packages back in gets us ready for this change. It won't impact how DNS is configured or used and it let's users get ready for the change in Fedora 33.

@bgilbert
Copy link
Contributor

What sort of "getting ready" does this change enable? I'm not sure if it makes sense to add the service but not switch over to using it. We might make that switch in advance of Fedora 33, if that makes sense, or at the same time as Fedora 33.

@jdoss
Copy link
Contributor Author

jdoss commented Aug 21, 2020

You can switch to using systemd-resolved on Fedora Workstation right now if you want. It is a pretty easy change and I would like to start using on my FCOS deployments now to ensure I don't have any surprises later.

@cgwalters
Copy link
Member

Minor nit: our commit messages match Linux kernel style generally and usually have a "topic prefix" and no period at the end, and if you can succinctly describe why in the subject that's great too e.g.
manifest: Include systemd-resolved for F33 change

@cgwalters
Copy link
Member

Also I'd really like to see us get in the habit of modifying the test cases as part of commits. It's really easy, for this one something like

diff --git a/tests/kola/misc-ro b/tests/kola/misc-ro
index ab5a30e..3c58e7b 100755
--- a/tests/kola/misc-ro
+++ b/tests/kola/misc-ro
@@ -20,8 +20,11 @@ get_journal_msg_timestamp() {
         | jq -r --slurp '.[0]["__MONOTONIC_TIMESTAMP"]'
 }
 
-systemctl is-enabled logrotate.service
-ok logrotate
+for unit in logrotate systemd-resolved; do
+    if ! systemctl is-enabled ${unit}; then
+        fatal "Unit ${unit} should be enabled"
+    fi
+done
 
 # https://github.com/coreos/fedora-coreos-config/commit/2a5c2abc796ac645d705700bf445b50d4cda8f5f
 if ip link | grep -o -e " eth[0-9]:"; then

to start.

To run the test against your change, use e.g.
cosa build && kola run '*'misc-ro
(Plus optionally the other tests too of course but that will also happen as part of a PR)

@cgwalters
Copy link
Member

Another thing on this topic - as extensively discussed on the list, this is a fairly profound change to the default network stack that is almost certainly going to have fallout. Now, it should be as easy as providing an ignition config to disable the unit, but we probably also want to be sanity testing that.

@cgwalters
Copy link
Member

Example fallout: I bet this will break this code.

@jdoss jdoss changed the title Add systemd-resolved packages back into FCOS manifest: Include systemd-resolved for F33 change Aug 22, 2020
@jdoss jdoss force-pushed the Add_systemd-resolved_back branch from dd8952d to ff80b8c Compare August 22, 2020 00:51
@jdoss
Copy link
Contributor Author

jdoss commented Aug 22, 2020

@cgwalters Updated my PR with your suggestions. I don't have cosa or kola setup on my workstation. Aside from RTFM on those repos do you have any tips or documentation for getting this going easily for new contributors?

@cgwalters
Copy link
Member

And indeed the test failed:

Aug 22 01:10:22 localhost.localdomain kola-runext-misc-ro[1190]: static
Aug 22 01:10:22 localhost.localdomain kola-runext-misc-ro[1189]: + for unit in logrotate systemd-resolved
Aug 22 01:10:22 localhost.localdomain kola-runext-misc-ro[1189]: + systemctl is-enabled systemd-resolved
Aug 22 01:10:22 localhost.localdomain kola-runext-misc-ro[1191]: disabled
Aug 22 01:10:22 localhost.localdomain kola-runext-misc-ro[1189]: + fatal 'Unit systemd-resolved should be enabled'
Aug 22 01:10:22 localhost.localdomain kola-runext-misc-ro[1189]: + echo 'Unit systemd-resolved should be enabled'
Aug 22 01:10:22 localhost.localdomain kola-runext-misc-ro[1189]: Unit systemd-resolved should be enabled
Aug 22 01:10:22 localhost.localdomain kola-runext-misc-ro[1189]: + exit 1
Aug 22 01:10:22 localhost.localdomain systemd[1]: kola-runext.service: Main process exited, code=exited, status=1/FAILURE
Aug 22 01:10:22 localhost.localdomain systemd[1]: kola-runext.service: Failed with result 'exit-code'.

I suspect the reason is that it's not enabled in the Fedora base presets? I see https://src.fedoraproject.org/rpms/fedora-release/c/c7ef1abb1f1732a261d75a11c9d5d585f73f7069?branch=master

@dustymabe
Copy link
Member

I need to dig in to the systemd-resolved change as part of coreos/fedora-coreos-tracker#609 anyway. Let me dig into this to understand it and come back with an informed response.

@cgwalters
Copy link
Member

Hmm right, agree we should probably just take this on as part of the next/f33 branch. When you do that PR, maybe cherry-pick in this patch?

@travier
Copy link
Member

travier commented Aug 26, 2020

I think there are two points here:

  • Enabling systemd-resolved in the final root: I think we should try this in next as this is coming in Fedora 33 and that would bring all the benefits mentioned (but may trigger other bugs).
  • Enabling systemd-resolved in the initramfs to keep networking behavior consistent between the two. This might require more work and I don't know if it's part of the F33 changes.

@dustymabe
Copy link
Member

I think there are two points here:

  • Enabling systemd-resolved in the final root: I think we should try this in next as this is coming in Fedora 33 and that would bring all the benefits mentioned (but may trigger other bugs).

I cherry-picked this patch into the "switch to F33" PR: #640

  • Enabling systemd-resolved in the initramfs to keep networking behavior consistent between the two. This might require more work and I don't know if it's part of the F33 changes.

I'm not sure about that. We might need to check with the change owner. Though I imagine the initramfs doesn't matter too much as long as there is some basic DNS to grab a few files and then get to the real root.

@dustymabe
Copy link
Member

jlebon convinced me to apply most of my f33 changes to f32 but make them conditional (that way config bot can keep syncing between the two and we don't have to eventually do a large merge back from next-devel into testing-devel). I propose we merge this into testing-devel and then #646 to make the test conditional on the fedora major version.

@dustymabe
Copy link
Member

Just for clarity.. systemd-resolved is disabled by default in F32 and enabled by default in F33. We're not changing that with this PR, just now including the bits.

This PR adds the systemd-resolved packages into FCOS. It is going to be used by
default in Fedora 33 per this change request:
https://fedoraproject.org/wiki/Changes/systemd-resolved

Adding the systemd-resolved packages back in gets us ready for this change.
It won't impact how DNS is configured or used and it lets users get ready for
the change in Fedora 33.
@jdoss jdoss force-pushed the Add_systemd-resolved_back branch from ff80b8c to bb99d8d Compare September 30, 2020 19:56
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@dustymabe
Copy link
Member

The CI for this will fail (fixed by the commit in #646). If CI passes for #646 I'll merge this and that.

@dustymabe
Copy link
Member

This commit was included in #646. Thanks @jdoss. I was going to merge this one individually but was convinced otherwise to prevent webhooks from going out and doing pipeline builds for state that we knew would fail tests.

@dustymabe dustymabe closed this Sep 30, 2020
@jdoss jdoss deleted the Add_systemd-resolved_back branch September 30, 2020 21:37
c4rt0 pushed a commit to c4rt0/fedora-coreos-config that referenced this pull request Mar 27, 2023
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.

5 participants