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.

Sign up for GitHub

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

Jump to bottom

internal/exec: delete enablement symlinks when disabling unit #1352

Merged
merged 1 commit into from
May 6, 2022
Merged

internal/exec: delete enablement symlinks when disabling unit #1352

merged 1 commit into from
May 6, 2022

Conversation

sohankunkerkar
Copy link
Member

We need to delete any enablement symlinks for a unit before sending it to a preset directive. This will help to disable that unit completely. This is a short-term solution until the upstream systemd PR (systemd/systemd#15205) gets accepted.

Fixes coreos/fedora-coreos-tracker#392

jlebon
jlebon reviewed Apr 26, 2022
View reviewed changes
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Let's add a test for this? Either in the blackbox testsuite, or as an external kola test.

internal/exec/util/unit.go Outdated Show resolved Hide resolved
internal/exec/util/unit.go Outdated Show resolved Hide resolved
@sohankunkerkar
Copy link
Member Author

Let's add a test for this? Either in the blackbox testsuite, or as an external kola test.

Yeah, I was thinking about writing an external kola test, however, the blackbox test makes more sense here.

@jlebon jlebon mentioned this pull request Apr 27, 2022
Merged
@sohankunkerkar sohankunkerkar force-pushed the remove-symlink branch 4 times, most recently from d54bda3 to 4aa6d50 Compare May 3, 2022 17:40
@sohankunkerkar sohankunkerkar requested a review from jlebon May 3, 2022 17:40
jlebon
jlebon reviewed May 3, 2022
View reviewed changes
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Some comments, but mostly looks sane. Seems like we could fold the second commit into the first.

internal/exec/util/unit.go Outdated Show resolved Hide resolved
tests/positive/files/units.go Outdated Show resolved Hide resolved
tests/positive/files/units.go Outdated Show resolved Hide resolved
@sohankunkerkar
Copy link
Member Author

Some comments, but mostly looks sane. Seems like we could fold the second commit into the first.

Done!

@sohankunkerkar sohankunkerkar force-pushed the remove-symlink branch 2 times, most recently from 7f2d394 to 0f12c38 Compare May 3, 2022 22:03
jlebon
jlebon approved these changes May 4, 2022
View reviewed changes
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM

For the commit title, maybe something like:

stages/files: delete enablement symlinks when disabling unit

?

And in the body, it's good that we have links to the related issues and PRs, but let's add a bit more explanation in the commit message itself.

@sohankunkerkar
Copy link
Member Author

stages/files

I don't think we're adding any changes to this folder

@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented May 4, 2022 •
edited
Loading

Primarily, there're two commits bc this patch affects two folders i.e. internal/{exec/distro} and tests/positive

@bgilbert bgilbert changed the title internal/*: remove enablement symlinks for a unit before disabling internal/exec: delete enablement symlinks when disabling unit May 6, 2022
bgilbert
bgilbert reviewed May 6, 2022
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

Changes look good; just a few nits. The commit message could still use a bit more detail; it doesn't say why we need to do this.

internal/distro/distro.go Outdated Show resolved Hide resolved
internal/distro/distro.go Outdated Show resolved Hide resolved
internal/exec/util/unit.go Outdated Show resolved Hide resolved
tests/positive/files/units.go Outdated Show resolved Hide resolved
@sohankunkerkar sohankunkerkar force-pushed the remove-symlink branch 4 times, most recently from a38cc75 to 8585c47 Compare May 6, 2022 15:10
@sohankunkerkar
internal/exec: delete enablement symlinks when disabling unit
397aa6a 
For services where FCOS ships a symlink in /etc, if the user tries
to disable the service via Ignition, systemd ignores the disablement
directive in the preset. Avoid this behavior by deleting the enablement
symlinks when disabling a unit, but continue to record the disablement
in the preset file. This is a short-term solution until the upstream
systemd PR (systemd/systemd#15205) is merged and widely deployed.

Fixes coreos/fedora-coreos-tracker#392
bgilbert
bgilbert approved these changes May 6, 2022
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

👍

@sohankunkerkar sohankunkerkar merged commit 12a4b10 into coreos:main May 6, 2022
@sohankunkerkar sohankunkerkar deleted the remove-symlink branch May 6, 2022 16:10
@jlebon jlebon mentioned this pull request Jul 6, 2022
Merged
@jlebon jlebon mentioned this pull request Aug 9, 2022
Open
@bgilbert bgilbert mentioned this pull request Apr 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgilbert bgilbert bgilbert approved these changes

@jlebon jlebon jlebon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to disable zincati.service using Ignition
3 participants
@sohankunkerkar @bgilbert @jlebon