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

RPM: Update config file patching #2145

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Sep 3, 2024

rpm/update-config-files.sh has now been simplified and will now fail if a pattern is not found instead of inserting a rule by default.

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

ACK for the record (but I didn’t check that the result of the script is as expected).

@lsm5
Copy link
Member Author

lsm5 commented Sep 4, 2024

ACK for the record (but I didn’t check that the result of the script is as expected).

Thanks. I need to make some more changes here. I'll probably end up reworking the entire thing to be an actual patch instead of a shell script.

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@lsm5 lsm5 force-pushed the storage-options branch 10 times, most recently from 72b7cdc to 5176a65 Compare September 4, 2024 14:17
@lsm5 lsm5 changed the title RPM: Update storage.conf patching RPM: Update config file patching Sep 4, 2024
@lsm5
Copy link
Member Author

lsm5 commented Sep 4, 2024

@jnovy @Luap99 @edsantiago @mtrmac PTAL. I've removed the rule in ensure() that would just insert a line if a pattern isn't found.

I tried to change the build process with actual patches but dealing with %autosetup to account for patches from 3 different repos turned out to be too much of a PITA. I think the current setup should work well enough.

podman system tests on crun PR also pass with the copr builds generated here: containers/crun#1533 . Ignore the aarch64 test failures there. The x86_64 test jobs there have been tested with the x86_64 copr builds from this PR.

@lsm5 lsm5 marked this pull request as ready for review September 4, 2024 14:46
@lsm5
Copy link
Member Author

lsm5 commented Sep 4, 2024

ignore centos-stream 10 copr job failure here. That's blocked on https://pagure.io/releng/issue/12305

@jnovy
Copy link
Contributor

jnovy commented Sep 4, 2024

This breaks all the logic around being sure a particular option is set? Without this it doesn't make sense?

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Non-blocking: Now that you brought this up, I’d really prefer these to be a patch. The manual echo and sed “insert after this line” commands seem rather brittle.

If integrating that into %autosetup were too hard (?), even a patch command inside this shell script seems more attractive to me.

rpm/update-config-files.sh Show resolved Hide resolved
@@ -17,25 +17,19 @@ ensure() {
then
Copy link
Contributor

Choose a reason for hiding this comment

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

(Pre-existing: it’s idiomatic to use if grep -q, rather than redirect to /dev/null)

@@ -17,25 +17,19 @@ ensure() {
then
sed -i "/^#.*$2[[:blank:]].*=/a \
$2 = $3" $1
else
echo "$2 = $3" >> $1
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

The description says “fail if a pattern is not found”; how does that happen? I don’t see, in my tests, that if false … returns a false status.

@edsantiago
Copy link
Member

Sorry for taking so long. Can we assume python on the build host? If so, tomlkit preserves comments and seems to do a really nice job of intelligently editing toml. Could you do something like this?

#! /usr/bin/env python3

import tomlkit

def foo():
    path = "foo.conf"
    with open(path, "r") as fp:
        x = tomlkit.load(fp)

    fedora = 1
    if fedora:
        want2add = '["registry.fedoraproject.etc", "registry.access.rh.etc"]'
    else:
        want2add = '["registry.access.rh.etc", "registry.redhat.io", "etc"]'
    x.add(tomlkit.nl())
    x.add('unqualified-search-registries', want2add)

    # FIXME: save to tmp file, mv into place
    print(tomlkit.dumps(x))

if __name__ == '__main__':
    foo()

@mtrmac
Copy link
Contributor

mtrmac commented Sep 4, 2024

I think we can require Python as a build dependency — but I think we can’t require tomlkit: AFAICS it is not included in any RHEL product.

@edsantiago
Copy link
Member

Sigh...... you're right. Well, that was a nice waste of time.

@edsantiago
Copy link
Member

After taking a few steps back to reflect, I agree with @mtrmac. This needs to be done via patch or, if you can figure out all the Fedora-CentOS-RHEL logistics, %autosetup.

@mtrmac
Copy link
Contributor

mtrmac commented Sep 4, 2024

Maybe we should split this effort into two? We need an update for the c/storage changes to the shipped storage.conf. The other changes are desirable but probably not blocking the first part.

(I’m not saying it must be split, I’ll leave it entirely to the PR authors — just, as the one who brought up conversion to patches, I wanted to note that not all review comments are equally urgent.)

@jnovy
Copy link
Contributor

jnovy commented Sep 5, 2024

Non-blocking: Now that you brought this up, I’d really prefer these to be a patch. The manual echo and sed “insert after this line” commands seem rather brittle.

If integrating that into %autosetup were too hard (?), even a patch command inside this shell script seems more attractive to me.

The patch against each config file comes at high maintenance cost as the context changes way too often - almost during every vendored component update. A maintainer needs to manually regenerate patches during each update. Also for each version of RHEL/Fedora. Consider e.g. seccomp.json. The ensure concept - just to be sure X = Y is present in config file (even replacing X = Z to X = Y) turned out to be very robust and readable throughout the years of RHEL maintenance.

storage.conf has been updated upstream so the config files update script
needs to account for that.

Ref: containers/storage#2066

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5
Copy link
Member Author

lsm5 commented Sep 5, 2024

Thanks for the comments everyone. In order to get things unblocked and to keep PR diff to a minimum, I've only removed the problematic line in the latest version. Tests on crun PRs are successful with this change. PTAL again.

EDIT: the key values from the removed line are now set as default in the storage.conf update https://github.com/containers/storage/blob/main/storage.conf , so deletion of that line should suffice AFAICT.

I'll keep additional patching changes for later, if at all.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@Luap99
Copy link
Member

Luap99 commented Sep 5, 2024

There issues in my eyes, first the script patching has a silent failure mode where we generated broken config files. The script has no idea when the toml changes and if it is valid or not. Additionally there is no human review on the generated files whatsoever. Heck I cannot even find the generated file anywhere in dist-git because it was just generated on the fly in the build process so the only way to get the file is to download the build rpm which is rather late in the process to notice errors. Having patches that fail to apply on changes would mitigate this to some extend as the build would fail instead requiring someone to fix it manually.

Second, there is no or little testing on the released packages. I guess we talk about podman-next copr here so little testing there might be fine but this could as well break in normal fedora releases AFAICS. In the RHEL release process we have more testing so things get caught but this is also very late.
And if testing is done there it doesn't seem specifically test that the config settings are actually in use. Checking that we have the right short-name-mode, mount opts, log drivers, etc... is not really done. Podman/Buildah gating tests might assume some of this implicitly and break if the config files are not the right ones but this is much harder to notice. If these settings are important enough to get patched on the distro/package level to us then they must be important enough to also get explicitly tested.

@jnovy
Copy link
Contributor

jnovy commented Sep 5, 2024

LGTM

# regardless of distro
if [[ "$FEDORA" -gt 40 ]] || [[ "$RHEL" -ge 10 ]]; then
ensure pkg/config/containers.conf compression_format \"zstd:chunked\"
ensure storage.conf pull_options \{enable_partial_images\ =\ \"true\",\ use_hard_links\ =\ \"false\",\ ostree_repos=\"\",\ convert_images\ =\ \"false\"\}
# Leave composefs disabled
ensure storage.conf use_composefs \"false\"
Copy link
Member

Choose a reason for hiding this comment

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

IIUC use_composefs needs to fit inside an [overlay] block.

I don't think I'm prepared to review further changes of this sort. There are no safety checks, no visibility into the generated files, at least not until much later when downstream tests start failing. I will not block, but I can't approve. I'm sorry.

@lsm5
Copy link
Member Author

lsm5 commented Sep 5, 2024

Let's get this merged unless anyone is strongly opposed. The PR in its current form isn't making things worse. I concur that we need testing of built packages, something which I can address in followup PRs.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

openshift-ci bot commented Sep 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Sep 5, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit db9e6ce into containers:main Sep 5, 2024
16 checks passed
@mtrmac
Copy link
Contributor

mtrmac commented Sep 5, 2024

The patch against each config file comes at high maintenance cost as the context changes way too often - almost during every vendored component update. A maintainer needs to manually regenerate patches during each update.

I read that as “this needs human review very often, and using a patch ensures that human review happens”, 100% a benefit for things like the recent storage.conf reformatting.


Consider e.g. seccomp.json.

That one might be different if it is really machine-generated, and we can assume that the existing kill/socketcall entries will never be removed.

@lsm5 lsm5 deleted the storage-options branch December 17, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants