rpm-ostree icon indicating copy to clipboard operation
rpm-ostree copied to clipboard

kargs: do string modifications client-side and cleanup some implementation details

Open jlebon opened this issue 3 years ago • 1 comments

Today, the rpm-ostree kargs works is:

  • it reads in the default kargs from the daemon
  • if --editor is used, it uses that to generate the final set of kargs
  • it calls KernelArgs() which takes the default kernel args, as well as possible modifications via --append, --replace, --delete

There are two concerns with this:

  • Passing the existing kargs to KernelArgs() seems weird; the daemon should know what the kargs are. But the reason we do this is because we rely on it in the --editor case to replace the kargs entirely. So that D-Bus argument plays an awkward double-duty.
  • String manipulations happen daemon-side, even though we could easily do it client-side. E.g. right now --delete nonexistent will trigger a full transaction just to error out.

It seems like what we should do instead is something like:

  • read default kargs from daemon
  • if --editor is used, generate final set of kargs
  • otherwise, perform modifications locally based on --append etc. to generate final set of kargs
  • call KernelArgs() with the final set of kargs

That way, --editor and e.g. --append, etc. work the same way.

To address race conditions, we could have a key in the options GVariant called e.g. previous_kernel_args which works like git's --force-with-lease: the daemon errors out if the kernel arguments don't match before applying the new kargs.

jlebon avatar Mar 30 '21 14:03 jlebon

The regression at https://github.com/coreos/rpm-ostree/issues/3182 actually happened because of the split logic for --editor which is described here. Thus a bugfix will intersect this too.

The current method already takes a existing_kernel_args which I think we could keep using for the race-mitigating logic proposed above. An immediate step towards fixing --editor would be to make it using a new final-kernel-args entry in the options, so that the client passes both the previous and the new kargs (as two strings) to the daemon. Later on, the non-editor flow can be updated too to directly perform the changes client-side and pass only the result to the daemon through final-kernel-args.

lucab avatar Nov 08 '21 17:11 lucab