kargs delete-if-present / append-if-missing / replace failures

[champtar]

Host system details

Alma 9.3 base OS

# rpm -q rpm-ostree
rpm-ostree-2023.7-1.el9.x86_64

Expected vs actual behavior

I have/had crashkernel=auto and want to go to crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M using idempotent commands

# cat /proc/cmdline 
... crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M ...
# rpm-ostree kargs --unchanged-exit-77 --replace=crashkernel=auto=1G-4G:192M,4G-64G:256M,64G-:512M
error: No karg 'crashkernel=auto' found
# rpm-ostree kargs --unchanged-exit-77 --delete-if-present=crashkernel=auto
error: No karg 'crashkernel=auto' found
# rpm-ostree kargs --unchanged-exit-77 --delete-if-present=crashkernel=auto --append-if-missing=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M
error: No karg 'crashkernel=auto' found

delete-if-present fails if the key exists but the key=value doesn't match

Haven't tested on Fedora CoreOS / CentOS stream

Would you like to work on the issue?
no

[champtar]

I think this is a regression caused by #4161
https://github.com/coreos/rpm-ostree/pull/4161/files#diff-644b0b7460c86d00842e2874a1b0d814450847b4f08e3d5ed95021044c6ab908L2711-R2711
We were using g_strv_contains, so looking for the full key=value, but we are now using ostree_kernel_args_contains (https://github.com/ostreedev/ostree/blob/51a34a4030f91a1fc6c2e6cfa41bc6e2cef395d0/src/libostree/ostree-kernel-args.c#L804-L823) which only look at the key part
Ping @Razaloc

[Razaloc]

I've created a branch with the suggested rollback changes but I think it all still needs further testing. I'm going to try and test it, both the main and my branch, just in case I can reproduce the problem and see if it disappear.

[HuijingHei]

Not sure if my understanding is correct. If you want to update kernel argument from crashkernel=auto to crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M, how about using
rpm-ostree kargs --unchanged-exit-77 --replace=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M ?

And # rpm-ostree kargs --unchanged-exit-77 --delete-if-present=crashkernel=auto --append-if-missing=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M also works in my testing.

[root@cosa-devsh ~]# cat /proc/cmdline 
... crashkernel=auto
[root@cosa-devsh ~]# rpm-ostree kargs --unchanged-exit-77 --replace=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M
Staging deployment... done
Changes queued for next boot. Run "systemctl reboot" to start a reboot

[champtar]

@HuijingHei I want idempotent commands that I can run at every boot in a systemd unit, my issues are once crashkernel is not auto anymore or if it's missing

using replace fails if crashkernel is missing

rpm-ostree kargs --delete-if-present=crashkernel=auto
rpm-ostree kargs --delete-if-present=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M
# test
rpm-ostree kargs --unchanged-exit-77 --replace=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M

Delete + Append also fails if crashkernel is missing

rpm-ostree kargs --delete-if-present=crashkernel=auto
rpm-ostree kargs --delete-if-present=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M
# test (error)
rpm-ostree kargs --unchanged-exit-77 --delete-if-present=crashkernel=auto --append-if-missing=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M

Delete + append is completely broken

rpm-ostree kargs --delete-if-present=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M
rpm-ostree kargs --append-if-missing=crashkernel=auto
# check crashkernel=auto is present
rpm-ostree kargs

# test 1
rpm-ostree kargs --unchanged-exit-77 --delete-if-present=crashkernel=auto --append-if-missing=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M
# no error, but crashkernel is now missing
rpm-ostree kargs

# test 2 (error)
rpm-ostree kargs --unchanged-exit-77 --delete-if-present=crashkernel=auto --append-if-missing=crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M

[HuijingHei]

I think this is a regression caused by #4161 https://github.com/coreos/rpm-ostree/pull/4161/files#diff-644b0b7460c86d00842e2874a1b0d814450847b4f08e3d5ed95021044c6ab908L2711-R2711 We were using g_strv_contains, so looking for the full key=value, but we are now using ostree_kernel_args_contains (https://github.com/ostreedev/ostree/blob/51a34a4030f91a1fc6c2e6cfa41bc6e2cef395d0/src/libostree/ostree-kernel-args.c#L804-L823) which only look at the key part

Thanks @champtar for the reporting.

Agree with you, ostree_kernel_args_contains() only looks at the key part, this makes delete-if-present failed when there is existing key. append-if-missing will hit the same problem. We will revert part of that patch.

]# rpm-ostree kargs --append=crashkernel=1G
]# rpm-ostree kargs
... crashkernel=1G
]# rpm-ostree kargs --unchanged-exit-77 --delete-if-present=crashkernel=auto
error: No karg 'crashkernel=auto' found
]# rpm-ostree kargs --unchanged-exit-77 --append-if-missing=crashkernel=auto
No changes.

We will not check the existing parameter for replace, the same as delete and append

[root@cosa-devsh ~]# rpm-ostree cleanup -p
[root@cosa-devsh ~]# rpm-ostree kargs --unchanged-exit-77 --replace=crashkernel=auto
error: No key 'crashkernel' found
[root@cosa-devsh ~]# rpm-ostree kargs --unchanged-exit-77 --delete=crashkernel=auto
error: No key 'crashkernel' found

[HuijingHei]

But refer to #4161, for corner case where --append foo --append-if-missing foo would append foo twice, maybe consider a safe way to fix it.

[cgwalters]

One thing I'm thinking here is we really want to go to a declarative-with-dropins model for kernel arguments. What we have today with imperative stateful mutation is just going to be seriously prone to these corner cases.

The declarative model would also mesh much better with container builds.