coreos / ignition

First boot installer and configuration tool
https://coreos.github.io/ignition/
Apache License 2.0
829 stars 245 forks source link

Ignition Kernel Argument Support #1168

Closed arithx closed 3 years ago

arithx commented 3 years ago

Overview

Ignition will add fields to support the adding or removing of kernel arguments. Additionally, a new stage will be introduced which by default is NOT enabled in the current stage chaining. During this new stage, Ignition will take the fields and directly pass them into a binary (the location of which is defined in internal/distro to allow overrides) provided by the distribution. Ignition will NOT verify that the kernel arguments have been set, only that the binary returned successfully. Additionally, the binary / wrapping systemd units (provided by the distribution) are responsible for performing a reboot. Ignition will not persistently cache the fetched config, so it will be re-fetched after a reboot.

Example Spec Changes

A new top level field kargs will be added to the Ignition spec which contains two fields, shouldExist & shouldNotExist which both accept lists of strings.

NOTE: The individual field names are subject to changes

{
  "ignition": { "version": "3.3.0-experimental" },
  "kargs": {
      "shouldExist": [
          "hugepagesz=1M hugepages=8 hugepagesz=2G hugepages=2",
          "foo=bar",
          "baz"
      ],
      "shouldNotExist": [
          "nosmt"
      ]
  }
}

Validation

Ignition will fail validation if duplicate entries are found in both the shouldExist & shouldNotExist arrays. It will compare both entire string and space-delimited splits of each string and reject matches. This is done to not require an add/remove specific ordering & to preserve the declarativity of the config.

Distributor Documentation

New distribution documentation will be added to Ignition to provide a sample script for how the binary could look, along with making recommendations about stage ordering (the recommended ordering will be sometime after the fetch stage and before the disks stage).

Binary Constraints

Ignition expects that the distribution provided binaries follow these constraints:

  1. Adds should be done in an append if missing manner
  2. Deletes should NOT fail if they are not present
  3. Rebooting the system if necessary
cgwalters commented 3 years ago

This seems to support both whitespace-separated strings as well as distinct entries in an array. What's the rationale for that? Is it saying that e.g. the arguments in a single string must be provided in exactly that order, but otherwise the arguments may be reordered?

cgwalters commented 3 years ago

Also I think the problem in https://github.com/openshift/os/issues/503 :

At minimum we'll need to coordinate the two so that there is a maximum of one reboot in the initrd (if any of FIPS or Ignition kernel arguments are set).

is a general one. A FCOS user might also want to reboot for some other reason - perhaps, similarly to the MCO/OpenShift4 they want to apply OS updates before some workloads can land on a machine.

So I think probably what we want is a declarative way to tell Ignition to not reboot in the initramfs. Perhaps to start it's just:

{
  "ignition": { "version": "3.3.0-experimental" },
  "kargs": {
      "reboot": false,
      "shouldExist": [
          "hugepagesz=1M hugepages=8 hugepagesz=2G hugepages=2",
          "foo=bar",
          "baz"
      ],
      "shouldNotExist": [
          "nosmt"
      ]
  }
}
bgilbert commented 3 years ago

This seems to support both whitespace-separated strings as well as distinct entries in an array. What's the rationale for that?

In general, users should specify each argument as a separate array entry. The space-separated case is to support specific parameters such as hugepages, where an individual argument might be repeated in combination with another argument. An argument-by-argument shouldExist wouldn't do the right thing in that case.

A FCOS user might also want to reboot for some other reason - perhaps, similarly to the MCO/OpenShift4 they want to apply OS updates before some workloads can land on a machine.

Hmm, do you have a specific use case in mind? In general, we go out of our way to make the first boot behave identically to the other boots, and we encourage FCOS users to directly launch the version of the OS they want to run. (Which is to say, I think we should continue to discourage an OpenShift-style multiple-reboot model in FCOS.)

jlebon commented 3 years ago

This isn't describing the API that the distribution binary needs to implement.

I think initially we were discussing about just using switches for this. But I'm thinking it'd be cleaner really if Ignition just reserializes the kargs object to JSON and passes it as its stdin. Then, the API is the spec and has the same compatibility constraints etc...

(It also has the effect of encouraging implementations to be written in !shell, which is not a bad idea. :) )

bgilbert commented 3 years ago

I think we should keep the API for the helper program as simple as possible, to reduce the integration effort needed by a distro. The initrd typically won't have jq or a language interpreter other than shell, and a bespoke binary complicates creation of the distro-specific Dracut module. So I'd argue for sticking with command-line arguments, and keeping the example program in shell.

jlebon commented 3 years ago

Starting to look at adding the necessary support for this in rdcore. In the interest of using wording that's implementation-agnostic and a 1:1 mapping to the spec, let's just do something like (using the same kargs as above):

$ <distro.kargCmd> \
    --should-exist "hugepagesz=1M hugepages=8 hugepagesz=2G hugepages=2" \
    --should-exist "foo=bar" \
    --should-exist "baz" \
    --should-not-exist "nosmt"

?

jlebon commented 3 years ago

rdcore kargs support: https://github.com/coreos/coreos-installer/pull/498

lucab commented 3 years ago

A bunch of design questions on this proposal.

It looks like the execution flow is not explicitely defined/linearized. How is the system expected to behave if there is a stub configuration with something like kargs.shouldExist = [ "ip=..." ] and ignition.config.replace.source = "https://..."?

Both shouldExist and shouldNotExist take a list of strings. Does this mean that they enforce ordering too? That is, would shouldExist: ["a", "b"] be ok with a b foo a cmdline?

Is this new stage expected to be aware of dracut injected args?

jlebon commented 3 years ago

A bunch of design questions on this proposal.

It looks like the execution flow is not explicitely defined/linearized. How is the system expected to behave if there is a stub configuration with something like kargs.shouldExist = [ "ip=..." ] and ignition.config.replace.source = "https://..."?

Do you mean whether this is something that could be used to break the network bootstrapping problem? I'm not sure if this is something we should pursue, because it's inconsistent with how Ignition processes its config otherwise (i.e. fetching everything first and merging/replacing as needed). So in the scenario you described, ignition-fetch would still as usual try to fetch the HTTPS config and replace it in before continuing. I'd like us to keep trying to move away from network kargs towards NM keyfiles and the --copy-network workflow instead (and keep polishing that UX, like https://github.com/openshift/enhancements/pull/492).

Both shouldExist and shouldNotExist take a list of strings. Does this mean that they enforce ordering too? That is, would shouldExist: ["a", "b"] be ok with a b foo a cmdline?

Ordering is not enforced, but you can specify multiple kargs in a single element using spaces if you want specific kargs to appear in a certain order (see https://github.com/coreos/ignition/issues/1168#issuecomment-784557866 above).

Is this new stage expected to be aware of dracut injected args?

Ignition itself doesn't really care about dracut args, so this is mostly down to the distro implementation. For FCOS/RHCOS, I don't think we should be aware of dracut args at all. Although we do make use of it at runtime for the conditional networking bits, we should strongly encourage components to not bake drop-ins there (see e.g. https://github.com/latchset/clevis/commit/c52caeb438edb54c4c0559dfb8a349ed1f14400a).