Skip to content

Conversation

@erpeters157
Copy link
Contributor

@erpeters157 erpeters157 commented Sep 29, 2025

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 29, 2025
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Sep 29, 2025

🤖 Tue Oct 21 19:28:22 - Prow CI generated the docs preview:
https://99811--ocpdocs-pr.netlify.app
Complete list of updated preview URLs: artifacts/updated_preview_urls.txt

@erpeters157 erpeters157 changed the title Initial draft of updates for heterogenous cluster updates Updates for golden image heterogenous cluster support Sep 29, 2025
@avivtur
Copy link

avivtur commented Sep 30, 2025

LGTM as for the parts referring to the web console

@nunnatsa
Copy link
Contributor

Thanks @erpeters157 !

Several things I fill missing:

  1. The fact that the feature is in tech-preview, and is not fully supported
  2. some background: e.g. what is the problem we're trying to solve here and why is it needed; what are the risk of not solving the issue; what are multi-arch manifest images (e.g. add a link to some cncf doc) and so on.
    you can use the background part of these docs for reference:
    1. VEP
    2. Upstream user guide
  3. Add an alternative to enabling the feature; see the runbook of the HCOMultiArchGoldenImagesDisabled alert

Copy link

@Acedus Acedus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the lower-level resource that is a DataVolume, which is essentially used to power this entire feature behind the scenes, is pretty much usable out-of-the-box. With that said, aside from upstream documentation on the matter[1], there's no mention of it in the official OpenShift docs and I believe that for certain advanced users, this information may be valuable, should we include it?

[1] https://github.com/kubevirt/containerized-data-importer/blob/main/doc/image-from-registry.md#import-registry-image-by-platform-specification

@nunnatsa
Copy link
Contributor

nunnatsa commented Oct 5, 2025

Hi @erpeters157 . Thanks for the fixes.

I still think we must have some background of why this feature is needed, and what is the risk of not using it in heterogeneous cluster. I think it's a meaningful risk and the user should know about it. This is why we added a special alert for this case.

@erpeters157
Copy link
Contributor Author

@nunnatsa Thanks for your feedback. It's definitely still a work in progress based on everyone's feedback but I appreciate your input.

@erpeters157
Copy link
Contributor Author

@Acedus I'm still new to CNV but I'll ask the established writers if there is a reason why this isn't mentioned in the downstream documentation. I'm going to focus on addressing what we need to document for heterogenous cluster support and if I have time, and it should be added, I will work on that. If I can't get to it, may be we could open a Jira to make it an async update.

@erpeters157
Copy link
Contributor Author

@Acedus I spoke to my team and none of them know why the DataVolume resource does not appear in the downstream documentation. Since we are close to GA, I will create a documentation Epic in Jira to follow up on this post-GA. I will make you the reporter and assign it to myself. Thank you very much for bringing this issue to our attention.

@erpeters157
Copy link
Contributor Author

@nunnatsa Please see my latest updates with more of the information you requested. Please let me know if this is good or you'd would like additions. Thank you for your support and feedback.

@duyanyan
Copy link

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2025
Copy link
Contributor

@danielclowers danielclowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few things needing addressed and a couple of suggestions

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2025
@erpeters157
Copy link
Contributor Author

@nunnatsa Please see my latest updates. I rearranged the text order in the section introduction and called out the bit about the potential problems for not using it in an IMPORTANT note. Does this work better for you?

Copy link
Contributor

@nunnatsa nunnatsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2025
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2025
@nunnatsa
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2025
@ousleyp ousleyp added this to the Planned for 4.20 GA milestone Oct 20, 2025
Copy link
Member

@ousleyp ousleyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there might be more things, but these are the things that jumped out at me. overall it looks great though!

@bergerhoffer
Copy link
Contributor

The branch/enterprise-4.21 label has been added to this PR.

This is because your PR targets the main branch and is labeled for enterprise-4.20. And any PR going into main must also target the latest version branch (enterprise-4.21).

If the update in your PR does NOT apply to version 4.21 onward, please re-target this PR to go directly into the appropriate version branch or branches (enterprise-4.x) instead of main.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2025
@openshift-ci
Copy link

openshift-ci bot commented Oct 20, 2025

New changes are detected. LGTM label has been removed.

@sjhala-ccs sjhala-ccs added the CNV Label for all CNV PRs label Oct 21, 2025
Copy link
Contributor

@sjhala-ccs sjhala-ccs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erpeters157 A few more suggestions before this is ready for merging. Thanks!

@openshift-ci
Copy link

openshift-ci bot commented Oct 21, 2025

@erpeters157: all tests passed!

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

Copy link
Contributor

@sjhala-ccs sjhala-ccs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@sjhala-ccs sjhala-ccs merged commit 6ecb9a7 into openshift:main Oct 21, 2025
2 checks passed
@sjhala-ccs
Copy link
Contributor

/cherrypick enterprise-4.20

@sjhala-ccs
Copy link
Contributor

/cherrypick enterprise-4.21

@openshift-cherrypick-robot

@sjhala-ccs: new pull request created: #100856

In response to this:

/cherrypick enterprise-4.20

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.

@openshift-cherrypick-robot

@sjhala-ccs: new pull request created: #100857

In response to this:

/cherrypick enterprise-4.21

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.20 branch/enterprise-4.21 CNV Label for all CNV PRs size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.