Topic/docs completions config updates for runtime pull image

[fidencio]

What type of PR is this?

What this PR does / why we need it:

The runtime_pull_image configuration option was introduced as part of fff6493, but missed documentation, completion, and adding it as part of the configuration template.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Add missing information about the `runtime_pull_image` to the documentation, completion, and default crio configuration.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fidencio

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:

• OWNERS [fidencio]

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

[fidencio]

/cc @littlejawa

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 49.67%. Comparing base (de1e328) to head (e25ee07).
Report is 9 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8110   +/-   ##
=======================================
  Coverage   49.67%   49.67%           
=======================================
  Files         153      153           
  Lines       16826    16829    +3     
=======================================
+ Hits         8358     8360    +2     
- Misses       7424     7426    +2     
+ Partials     1044     1043    -1     

[littlejawa]

Hey @fidencio,
Thanks for filling a hole here :-)

Now I have a concern: this is not just documenting "runtime_pull_image" and adding it to the template config, it also adds it to the "--runtimes" parameter, which I didn't do in the original PR.

The parsing for it is not done, so if you do it, you need to change the following to actually parse the new parameter within the string :

cri-o/internal/criocli/criocli.go

Lines 124 to 169 in d5666af

	if ctx.IsSet("runtimes") {
	runtimes := StringSliceTrySplit(ctx, "runtimes")
	for _, r := range runtimes {
	fields := strings.Split(r, ":")
	runtimeType := libconfig.DefaultRuntimeType
	privilegedWithoutHostDevices := false
	runtimeConfigPath := ""
	var (
	containerMinMemory string
	err error
	)
	switch len(fields) {
	case 7:
	containerMinMemory = fields[6]
	_, err = units.RAMInBytes(containerMinMemory)
	if err != nil {
	return fmt.Errorf("invalid value %q for --runtimes:container_min_memory: %w", containerMinMemory, err)
	}
	fallthrough
	case 6:
	runtimeConfigPath = fields[5]
	fallthrough
	case 5:
	if fields[4] == "true" {
	privilegedWithoutHostDevices = true
	}
	fallthrough
	case 4:
	runtimeType = fields[3]
	fallthrough
	case 3:
	config.Runtimes[fields[0]] = &libconfig.RuntimeHandler{
	RuntimePath: fields[1],
	RuntimeRoot: fields[2],
	RuntimeType: runtimeType,
	PrivilegedWithoutHostDevices: privilegedWithoutHostDevices,
	RuntimeConfigPath: runtimeConfigPath,
	ContainerMinMemory: containerMinMemory,
	}
	default:
	return fmt.Errorf("invalid format for --runtimes: %q", r)
	}
	}
	}

And then also

cri-o/internal/criocli/criocli.go

Line 654 in d5666af

Usage: "OCI runtimes, format is 'runtime_name:runtime_path:runtime_root:runtime_type:privileged_without_host_devices:runtime_config_path:container_min_memory'.",

to keep things consistant.

And then the crio CI scripts using it should be adapted in the same PR, otherwise some of them might (silently) fail.
See:

• cri-o/contrib/test/ci/vars.yml

  Line 79 in d5666af

  CONTAINER_RUNTIMES: "containerd-shim-kata-v2:/opt/kata/bin/containerd-shim-kata-v2:/run/vc:vm:true:/opt/kata/share/defaults/kata-containers/configuration.toml"

• cri-o/test/helpers.bash

  Line 162 in d5666af

  export CONTAINER_RUNTIMES=${CONTAINER_RUNTIMES:-$CONTAINER_DEFAULT_RUNTIME:$RUNTIME_BINARY_PATH:$RUNTIME_ROOT:$RUNTIME_TYPE:$PRIVILEGED_WITHOUT_HOST_DEVICES:$RUNTIME_CONFIG_PATH}

Now I have another concern: changing the "--runtimes" string the way you do, the order of parameters is modified. It means that all users of this parameter will see their configuration broken next time they update crio.
Or worse: it will silently work with wrong parameters.
If we add something in this string, I'm afraid it needs to be at the end of it, for backward compatibility...

[haircommander]

it doesn't look like this was wired into the cli flag so I don't think this is accurate

[openshift-ci]

@fidencio: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-fedora-integration	e25ee07	link	true	/test ci-fedora-integration
ci/prow/ci-cgroupv2-integration	e25ee07	link	true	/test ci-cgroupv2-integration
ci/prow/ci-fedora-kata	e25ee07	link	true	/test ci-fedora-kata
ci/prow/e2e-gcp-ovn	e25ee07	link	true	/test e2e-gcp-ovn

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[littlejawa]

Actually, I think we should keep it "false" here (or not set it at all as it is the default)
Our testing is done on kata alone - not peer-pods nor CoCo. We don't want to enable that feature systematically.

[littlejawa]

There is still one occurrence of "usage" string not updated :

cri-o/internal/criocli/criocli.go

Line 654 in d5666af

Usage: "OCI runtimes, format is 'runtime_name:runtime_path:runtime_root:runtime_type:privileged_without_host_devices:runtime_config_path:container_min_memory'.",

Otherwise lgtm

Thanks!