feat(runtime): support multiple installation sources for container runtimes#637
Conversation
Pull Request Test Coverage Report for Build 21950537158Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR extends the container runtime provisioning capabilities to support multiple installation sources (package, git, latest) for containerd, Docker, and CRI-O. This is part of epic #567 Phase 2 and follows the established multi-source pattern introduced for the NVIDIA Container Toolkit in a previous PR (#635). The changes maintain full backward compatibility with the existing Version field while introducing new structured source specifications.
Changes:
- Added API types for runtime source selection:
RuntimeSourceenum andRuntimePackageSpec,RuntimeGitSpec,RuntimeLatestSpecconfiguration structs - Implemented git-based installation templates for all three runtimes with proper build dependencies, source compilation, and systemd service creation
- Added "latest" branch tracking support for containerd (Docker and CRI-O intentionally excluded per design)
- Refactored constructor functions to return errors and handle multiple source types, with git ref resolution integrated into the provisioner dependency graph
- Added comprehensive validation logic and test coverage for all source combinations
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| api/holodeck/v1alpha1/types.go | Defines new RuntimeSource enum and spec types for multi-source runtime installation |
| api/holodeck/v1alpha1/validation.go | Adds ContainerRuntime.Validate() method to validate source configurations |
| api/holodeck/v1alpha1/validation_test.go | Comprehensive test coverage for all valid and invalid source combinations |
| pkg/provisioner/templates/containerd.go | Adds git and latest source templates, refactors NewContainerd to return errors |
| pkg/provisioner/templates/containerd_test.go | Updated tests for new sources including git and latest templates |
| pkg/provisioner/templates/docker.go | Adds git source template with moby build support, refactors NewDocker |
| pkg/provisioner/templates/docker_test.go | Updated tests covering package and git sources |
| pkg/provisioner/templates/crio.go | Adds git source template with CRI-O build support, refactors NewCriO |
| pkg/provisioner/templates/crio_test.go | Updated tests covering package and git sources |
| pkg/provisioner/dependency.go | Integrates git ref resolution for all three runtimes using gitref.NewGitHubResolver |
| { | ||
| name: "Latest source - default", | ||
| cr: ContainerRuntime{ | ||
| Install: true, | ||
| Name: ContainerRuntimeContainerd, | ||
| Source: RuntimeSourceLatest, | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "Latest source - with config", | ||
| cr: ContainerRuntime{ | ||
| Install: true, | ||
| Name: ContainerRuntimeContainerd, | ||
| Source: RuntimeSourceLatest, | ||
| Latest: &RuntimeLatestSpec{ | ||
| Track: "release/1.7", | ||
| }, | ||
| }, | ||
| wantErr: false, | ||
| }, |
There was a problem hiding this comment.
The validation tests for ContainerRuntime only test the "latest" source with containerd. There should be additional test cases that verify Docker and CRI-O reject the "latest" source (once the validation logic is updated to enforce this restriction). Add test cases like "Latest source - Docker (should fail)" and "Latest source - CRI-O (should fail)" with wantErr: true.
|
|
||
| WORK_DIR=$(mktemp -d) | ||
| trap 'rm -rf "$WORK_DIR"' EXIT | ||
|
|
There was a problem hiding this comment.
The dockerGitTemplate doesn't validate that GIT_REPO is non-empty before using it in the git clone command (line 281). While NewDocker sets a default repository, defensive programming suggests validating critical variables in shell scripts. Add a check similar to containerd.go line 428: if [[ -z "${GIT_REPO}" ]]; then holodeck_log "ERROR" "$COMPONENT" "GIT_REPO is empty"; exit 1; fi before the git clone command.
| if [[ -z "${GIT_REPO}" ]]; then | |
| holodeck_log "ERROR" "$COMPONENT" "GIT_REPO is empty" | |
| exit 1 | |
| fi |
|
|
||
| WORK_DIR=$(mktemp -d) | ||
| trap 'rm -rf "$WORK_DIR"' EXIT | ||
|
|
There was a problem hiding this comment.
The crioGitTemplate doesn't validate that GIT_REPO is non-empty before using it in the git clone command (line 168). While NewCriO sets a default repository, defensive programming suggests validating critical variables in shell scripts. Add a check similar to containerd.go line 428: if [[ -z "${GIT_REPO}" ]]; then holodeck_log "ERROR" "$COMPONENT" "GIT_REPO is empty"; exit 1; fi before the git clone command.
| if [[ -z "${GIT_REPO}" ]]; then | |
| holodeck_log "ERROR" "$COMPONENT" "GIT_REPO is empty" | |
| exit 1 | |
| fi |
| // Latest source is valid with or without explicit config | ||
| return nil |
There was a problem hiding this comment.
The validation accepts RuntimeSourceLatest for all container runtimes, but Docker and CRI-O don't actually implement support for the "latest" source. According to the PR description table, only containerd supports "latest" (Docker and CRI-O are marked "N/A"). The validation should reject "latest" source for Docker and CRI-O to prevent users from specifying an unsupported configuration. Consider adding runtime-specific validation that checks cr.Name and returns an error if source is "latest" but name is "docker" or "crio".
| // Latest source is valid with or without explicit config | |
| return nil | |
| // Latest source is only supported for containerd | |
| switch cr.Name { | |
| case "containerd": | |
| // Latest source is valid with or without explicit config for containerd | |
| return nil | |
| case "docker", "crio": | |
| return fmt.Errorf("container runtime source 'latest' is not supported for %s", cr.Name) | |
| default: | |
| return fmt.Errorf("container runtime source 'latest' is only supported for containerd") | |
| } |
| d.GitRef = cr.Git.Ref | ||
| if d.GitRepo == "" { | ||
| d.GitRepo = "https://github.com/moby/moby.git" | ||
| } |
There was a problem hiding this comment.
NewDocker doesn't handle RuntimeSourceLatest case in the switch statement. If a user specifies source=latest for Docker (which is not supported per the PR description), the function will silently return with only Source set but no error. This could lead to confusing behavior. Add a case for "latest" that returns an error explaining that Docker doesn't support latest source tracking, or add a default case that handles unknown sources with an error.
| } | |
| } | |
| case "latest": | |
| return nil, fmt.Errorf("docker does not support latest source tracking; use source=package with version=latest instead") | |
| default: | |
| return nil, fmt.Errorf("unknown docker source: %s", d.Source) |
| c.GitRef = cr.Git.Ref | ||
| if c.GitRepo == "" { | ||
| c.GitRepo = "https://github.com/cri-o/cri-o.git" | ||
| } |
There was a problem hiding this comment.
NewCriO doesn't handle RuntimeSourceLatest case in the switch statement. If a user specifies source=latest for CRI-O (which is not supported per the PR description), the function will silently return with only Source set but no error. This could lead to confusing behavior. Add a case for "latest" that returns an error explaining that CRI-O doesn't support latest source tracking, or add a default case that handles unknown sources with an error.
| } | |
| } | |
| case "latest": | |
| return nil, fmt.Errorf("crio does not support latest source tracking") |
| CRI_DOCKERD_VERSION="0.3.17" | ||
| if [[ ! -f /usr/local/bin/cri-dockerd ]]; then | ||
| CRI_DOCKERD_URL="https://github.com/Mirantis/cri-dockerd/releases/download/v${CRI_DOCKERD_VERSION}/cri-dockerd-${CRI_DOCKERD_VERSION}.${GO_ARCH}.tgz" | ||
| curl -L "${CRI_DOCKERD_URL}" | sudo tar xzv -C /usr/local/bin --strip-components=1 |
There was a problem hiding this comment.
The curl command for downloading cri-dockerd lacks error handling. If the download fails, the pipeline will silently continue and tar will fail with a potentially confusing error. Add error handling by checking the curl exit status or using curl's -f flag which is already present via -fsSL in other parts of the template. Consider wrapping this in a conditional check or adding explicit error handling similar to the git clone operations above.
| curl -L "${CRI_DOCKERD_URL}" | sudo tar xzv -C /usr/local/bin --strip-components=1 | |
| if ! curl -fsSL "${CRI_DOCKERD_URL}" | sudo tar xzv -C /usr/local/bin --strip-components=1; then | |
| holodeck_error 12 "$COMPONENT" "Failed to download or extract cri-dockerd" \ | |
| "Check network connectivity and verify URL: ${CRI_DOCKERD_URL}" | |
| fi |
| CRI_DOCKERD_URL="https://github.com/Mirantis/cri-dockerd/releases/download/v${CRI_DOCKERD_VERSION}/cri-dockerd-${CRI_DOCKERD_VERSION}.${GO_ARCH}.tgz" | ||
| curl -L "${CRI_DOCKERD_URL}" | sudo tar xzv -C /usr/local/bin --strip-components=1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
The dockerGitTemplate installs cri-dockerd binary but doesn't create the required systemd service files (cri-docker.service and cri-docker.socket) that are created in the dockerPackageTemplate (lines 164-210). This means Kubernetes won't be able to use Docker as a container runtime after git-based installation. Add the systemd service file creation logic from dockerPackageTemplate to dockerGitTemplate after installing the cri-dockerd binary.
| # Create systemd units for cri-dockerd if they do not exist | |
| if [[ ! -f /etc/systemd/system/cri-docker.service ]]; then | |
| sudo tee /etc/systemd/system/cri-docker.service >/dev/null <<'EOF' | |
| [Unit] | |
| Description=CRI interface for Docker Application Container Engine | |
| Documentation=https://docs.mirantis.com | |
| After=network-online.target docker.service | |
| Wants=network-online.target | |
| [Service] | |
| Type=notify | |
| ExecStart=/usr/local/bin/cri-dockerd --container-runtime-endpoint fd:// | |
| ExecReload=/bin/kill -s HUP $MAINPID | |
| KillMode=process | |
| Restart=always | |
| RestartSec=5s | |
| [Install] | |
| WantedBy=multi-user.target | |
| EOF | |
| fi | |
| if [[ ! -f /etc/systemd/system/cri-docker.socket ]]; then | |
| sudo tee /etc/systemd/system/cri-docker.socket >/dev/null <<'EOF' | |
| [Unit] | |
| Description=CRI Docker Socket for the API | |
| PartOf=cri-docker.service | |
| [Socket] | |
| ListenStream=/var/run/cri-dockerd.sock | |
| SocketMode=0660 | |
| SocketUser=root | |
| SocketGroup=docker | |
| [Install] | |
| WantedBy=sockets.target | |
| EOF | |
| fi | |
| sudo systemctl daemon-reload | |
| sudo systemctl enable --now cri-docker.service cri-docker.socket |
ArangoGutierrez
force-pushed
the
feat/issue-567-runtime-sources
branch
from
19cf847 to
dc92075
Compare
…ntimes Extend ContainerRuntime to install containerd, Docker, and CRI-O from distribution packages (default), git source builds, or by tracking a moving branch. Follows the existing CTK/Kubernetes multi-source pattern with full backward compatibility. - containerd: package (v1.x/v2.x), git, latest - Docker/moby: package, git (with cri-dockerd) - CRI-O: package, git Closes: NVIDIA#567 (Phase 2 — runtime sources) Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Resolves gocritic ifElseChain lint warnings in containerd and docker templates. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
ArangoGutierrez
force-pushed
the
feat/issue-567-runtime-sources
branch
from
dc92075 to
b4e787a
Compare
2 participants
Summary
ContainerRuntimeto install containerd, Docker, and CRI-O from distribution packages (default), git source builds, or by tracking a moving branchContext
Part of #567 (Phase 2 — Runtime Sources). Depends on #635 (provenance) for
BuildComponentsStatusawareness, but can be merged independently.New API Types
RuntimeSourceenum:package,git,latestRuntimePackageSpec— version pinningRuntimeGitSpec— repo + refRuntimeLatestSpec— branch tracking + repo overrideTemplates (3 runtimes x 3 sources)
Wiring
dependency.go: git ref resolution for all three runtimesTest plan
go build ./...passesgo vet ./...passes