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

units: Sort systemd presets in alphabetical order in 20-ignition.preset file #1389

Merged
merged 2 commits into from
Jun 3, 2022

Conversation

marmijo
Copy link
Member

@marmijo marmijo commented Jun 1, 2022

The 20-ignition.preset file is used in CoreOS layering. The contents of the file will be sorted in alphabetical order by unit name to ensure the file is written in a consistent order across multiple runs.

Fixes #1339

@marmijo marmijo requested a review from bgilbert June 1, 2022 15:50
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.

For the record: let's add a positive blackbox test for this. We'll know we've regressed if it sometimes fails.

For the commit message, we usually add a context prefix. In this case, something like "files:" is probably fine, since this change is scoped to the files stage. Also, while it's good to link to the bug, we try to write the commit message with enough detail that the change is clear from the message alone.

internal/exec/stages/files/units.go Outdated Show resolved Hide resolved
internal/exec/stages/files/units.go Outdated Show resolved Hide resolved
@marmijo marmijo force-pushed the marmijo_issue_1339 branch 5 times, most recently from d005ee8 to 7fc221e Compare June 2, 2022 18:27
@marmijo marmijo changed the title sort unit presets in alphabetical order per #1339 units: Sort systemd presets in alphabetical order in 20-ignition.preset file Jun 2, 2022
@marmijo marmijo marked this pull request as ready for review June 2, 2022 18:52
@marmijo marmijo requested a review from bgilbert June 2, 2022 19:14
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.

Looks good!

tests/positive/files/units.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
tests/positive/files/units.go Show resolved Hide resolved
@bgilbert
Copy link
Contributor

bgilbert commented Jun 3, 2022

The first paragraph of the commit message is still a bit vague about why this matters. And I think I misled you a bit wrt CoreOS layering; that's a downstream FCOS/RHCOS concept that isn't directly relevant to Ignition upstream. It probably makes more sense to refer to ignition-apply, which is the Ignition entrypoint used by the layering code.

I'm sorry to say I took the easy way out here, and rewrote the paragraph. Feel free to use this verbatim or rework to taste:

The lines of the 20-ignition.preset file were being written in arbitrary
order.  This shouldn't affect its semantics, but could cause ignition-apply
to produce different filesystem trees from identical configs, breaking
reproducible container builds.  Sort the list of units before writing it
out.

The lines of the 20-ignition.preset file were being written in arbitrary
order.  This shouldn't affect its semantics, but could cause ignition-apply
to produce different filesystem trees from identical configs, breaking
reproducible container builds.  Sort the list of units before writing it
out.

Fixes coreos#1339
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.

🎉

@marmijo marmijo merged commit c41589b into coreos:main Jun 3, 2022
@marmijo marmijo deleted the marmijo_issue_1339 branch June 3, 2022 19:35
mkenigs added a commit to mkenigs/machine-config-operator that referenced this pull request Jun 12, 2022
Ignition would order the same content in 20-ignition.preset differently,
so it had to be sorted manually, but that was fixed in
coreos/ignition#1389
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.

/etc/systemd/system-preset/20-ignition.preset is not reproducible
2 participants