Revert "OCPBUGS-83492: Allow :ref suffix in additionalLayerStores path for stargz-store to support lazy image pulling"

[BhargaviGudi]

This reverts commit 47e56c7 (PR #2806)
Reason for Revert:
The :ref suffix should not be exposed in the user-facing API. Instead, the Machine Config Operator (MCO) should handle this implementation detail automatically.

Correct Approach:
The :ref suffix is an implementation detail of how stargz-store integrates with CRI-O's additionalLayerStores. This should be handled automatically by the MCO during the translation phase from ContainerRuntimeConfig CR to the node's storage.conf file

Related

• Original PR: OCPBUGS-83492: Allow :ref suffix in additionalLayerStores path for stargz-store to support lazy image pulling #2806
• Related Bug: OCPBUGS-83492
• MCO should handle this in: OCPBUGS-83492: Auto-append :ref suffix to additionalLayerStores in storage.conf machine-config-operator#5888

[openshift-merge-bot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

[openshift-ci]

Hello @BhargaviGudi! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

[openshift-ci-robot]

@BhargaviGudi: This pull request references Jira Issue OCPBUGS-83492, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (bgudi@redhat.com), skipping review request.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

This reverts commit 47e56c7 (PR #2806)
Reason for Revert:
The :ref suffix should not be exposed in the user-facing API. Instead, the Machine Config Operator (MCO) should handle this implementation detail automatically.

Correct Approach:
The :ref suffix is an implementation detail of how stargz-store integrates with CRI-O's additionalLayerStores. This should be handled automatically by the MCO during the translation phase from ContainerRuntimeConfig CR to the node's storage.conf file

Related

• Original PR: OCPBUGS-83492: Allow :ref suffix in additionalLayerStores path for stargz-store to support lazy image pulling #2806
• Related Bug: OCPBUGS-83492
• MCO should handle this in: OCPBUGS-83492: Allow :ref suffix in additionalLayerStores controller validation machine-config-operator#5858

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 openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request removes support for an optional :ref suffix in additional layer store paths. The LayerStorePath type in machineconfiguration/v1/types.go is removed and replaced with the existing StorePath type. The AdditionalLayerStore.Path field type is updated accordingly. Test cases covering :ref suffix validation scenarios are removed, and error messages are adjusted to no longer reference the suffix. CRD validation schemas across multiple feature gate levels (CustomNoUpgrade, DevPreviewNoUpgrade, TechPreviewNoUpgrade) are tightened to enforce absolute paths containing only alphanumeric characters, forward slashes, dots, underscores, and hyphens.

🚥 Pre-merge checks | ✅ 12

✅ Passed checks (12 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: reverting a commit that added :ref suffix support to the API, which is the primary objective of this pull request.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The PR modifies YAML-based test files and CRD definitions without any Ginkgo tests, making the Ginkgo naming convention check inapplicable.
Test Structure And Quality	✅ Passed	Test structure meets all quality requirements: single responsibility, automatic setup/cleanup, appropriate timeouts, meaningful assertions, and consistency with codebase patterns.
Microshift Test Compatibility	✅ Passed	This PR does not add any new Ginkgo e2e tests; it is a revert modifying CRD schema validation rules and Go type definitions without new Ginkgo test blocks.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests added; PR contains only type definitions, CRD schemas, and YAML validation configs.
Topology-Aware Scheduling Compatibility	✅ Passed	PR makes only schema-level changes to ContainerRuntimeConfig CRD without introducing deployment manifests, operator controllers, or topology-aware scheduling constraints.
Ote Binary Stdout Contract	✅ Passed	The pull request adds test infrastructure files that properly follow the OTE binary stdout contract by routing all logging through Ginkgo's GinkgoWriter instead of stdout, with no direct stdout writes in module-level or suite-level code.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PR does not add any new Ginkgo e2e tests; changes consist of schema validation and type definition updates with CRD validation test data only.
Description check	✅ Passed	The pull request description clearly explains that this is a revert of a previous commit, provides the reason (API should not expose :ref suffix as it's an implementation detail), describes the correct approach (MCO should handle it automatically), and includes relevant issue and PR references.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

machineconfiguration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml (1)

59-68: Add a direct negative test for :ref suffix rejection.

Given this PR’s main intent, consider adding a case like /var/lib/stargz-store:ref (or replacing one generic invalid-char case) so the contract is explicitly guarded by tests.

Proposed test addition

+    - name: Should fail if additionalLayerStores path ends with :ref suffix
+      initial: |
+        apiVersion: machineconfiguration.openshift.io/v1
+        kind: ContainerRuntimeConfig
+        spec:
+          containerRuntimeConfig:
+            additionalLayerStores:
+              - path: /var/lib/stargz-store:ref
+      expectedError: "path must be absolute and contain only alphanumeric characters, '/', '.', '_', and '-'"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@machineconfiguration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml`
around lines 59 - 68, Add a direct negative test that explicitly checks for
rejection of paths with a ":ref" suffix by updating the test case named "Should
fail if additionalLayerStores path contains invalid characters" in
AdditionalStorageConfig.yaml: either replace the existing invalid example
(/var/lib/stargz@store) or add a new case that uses a path like
/var/lib/stargz-store:ref and keep the same expectedError string ("path must be
absolute and contain only alphanumeric characters, '/', '.', '_', and '-'") so
the suite verifies the contract forbids colon/ref suffixes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@machineconfiguration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml`:
- Around line 59-68: Add a direct negative test that explicitly checks for
rejection of paths with a ":ref" suffix by updating the test case named "Should
fail if additionalLayerStores path contains invalid characters" in
AdditionalStorageConfig.yaml: either replace the existing invalid example
(/var/lib/stargz@store) or add a new case that uses a path like
/var/lib/stargz-store:ref and keep the same expectedError string ("path must be
absolute and contain only alphanumeric characters, '/', '.', '_', and '-'") so
the suite verifies the contract forbids colon/ref suffixes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 1d752ed0-6f50-4a18-b9f2-e232a3de1c2e

📥 Commits

Reviewing files that changed from the base of the PR and between 4f63a40 and ebfb8f0.

⛔ Files ignored due to path filters (5)

• machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs-CustomNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
• machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs-DevPreviewNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
• machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs-TechPreviewNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
• machineconfiguration/v1/zz_generated.featuregated-crd-manifests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• machineconfiguration/v1/zz_generated.swagger_doc_generated.go is excluded by !**/zz_generated*

📒 Files selected for processing (5)

• machineconfiguration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml
• machineconfiguration/v1/types.go
• payload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-CustomNoUpgrade.crd.yaml
• payload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-DevPreviewNoUpgrade.crd.yaml
• payload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-TechPreviewNoUpgrade.crd.yaml

[saschagrunert]

The revert is architecturally justified. The :ref suffix is an MCO implementation detail, not a user-facing concern.

[JoelSpeed]

Do we know why this wasn't picked up pre-merge? Is there a missing test?

/retest
/pipeline required

[openshift-merge-bot]

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-hypershift
/test e2e-aws-ovn-hypershift-conformance
/test e2e-aws-ovn-techpreview
/test e2e-aws-serial-1of2
/test e2e-aws-serial-2of2
/test e2e-aws-serial-techpreview-1of2
/test e2e-aws-serial-techpreview-2of2
/test e2e-azure
/test e2e-gcp
/test e2e-upgrade
/test e2e-upgrade-out-of-change
/test minor-e2e-upgrade-minor

[ngopalak-redhat]

/lgtm

[openshift-merge-bot]

Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ngopalak-redhat, saschagrunert
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci]

@BhargaviGudi: 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/e2e-upgrade-out-of-change	ebfb8f0	link	true	/test e2e-upgrade-out-of-change
ci/prow/e2e-azure	ebfb8f0	link	true	/test e2e-azure
ci/prow/e2e-upgrade	ebfb8f0	link	true	/test e2e-upgrade
ci/prow/e2e-aws-serial-techpreview-1of2	ebfb8f0	link	true	/test e2e-aws-serial-techpreview-1of2
ci/prow/e2e-aws-serial-techpreview-2of2	ebfb8f0	link	true	/test e2e-aws-serial-techpreview-2of2

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

[BhargaviGudi]

/verified by CI

[openshift-ci-robot]

@BhargaviGudi: This PR has been marked as verified by CI.

Details

In response to this:

/verified by CI

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 openshift-eng/jira-lifecycle-plugin repository.

[BhargaviGudi]

/retest-required