Skip to content

Conversation

@2uasimojo
Copy link
Member

@2uasimojo 2uasimojo commented Feb 9, 2024

Add a new field, ClusterDeployment.Status.ClusterVersionStatus.

At the behest of an (undocumented, unsupported) annotation, hive.openshift.io/sync-clusterversion-status: "true" (note: string "true" since it's an annotation) our clusterversion controller (the same one that updates the hive.openshift.io/version* labels) will populate it with a wholesale copy of the spoke's ClusterVersion.Status field.

NOTE: This requires that our vendor of openshift/api be kept up to date, as e.g. new enum values for capabilities will break us.

HIVE-2366

@openshift-ci openshift-ci bot requested review from abutcher and dlom February 9, 2024 21:38
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2024
@2uasimojo
Copy link
Member Author

/hold

  • This currently represents a hybrid approach, where we've added an actual API field (ClusterDeployment.Status.ClusterVersionStatus) for the content, but are using an annotation (hive.openshift.io/sync-clusterversion-status) to activate it. Design review is needed to decide whether we want to do that, or both in the API, or both in annotations.
  • The current approach also types our new field by importing the entire ClusterVersionStatus sub-object schema. This is in contrast to hypershift's approach, where they are hand-picking certain fields to copy in, and "converting" some of them to locally-defined types. A third alternative is to blat the content down as a JSON blob (in a RawExtension if we stick with the API approach; or as an ugly escaped string if in an annotation, as these things seem to be done "normally").
  • As currently written this is busted for newer OCP versions of the spoke because breaking changes were made to ClusterVersion.Status in openshift/api somewhere between what we currently vendor (20230720094506-afcbe27aec7c) and 4.15.0-rc.2 (I haven't narrowed it down further than that yet). Upgrading o/api pulls k8s deps bumps to 0.29.1, which for us also drags in a new revendor of openshift/installer. In other words, it stacks this behind Bump to k8s 0.29.1 #2193 which itself has a chain of prereqs. That's assuming we continue with the current design of importing the entire ClusterVersionStatus schema. Any of the other approaches -- annotation, RawExtension, or piecemeal copyin -- would be able to ignore discrepancies like this.
  • We'll want to add some UT for this, wherever it lands.

/cc @vkareh @cblecker @jewzaam @LalatenduMohanty @gvanderpotte @wanghaoran1988 @wking
cf. https://docs.google.com/document/d/1hgMiDYN9W60BEIzYCSiu09uV4CrD_cCCZ8As2m7Br1s/edit

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2024
@codecov
Copy link

codecov bot commented Feb 9, 2024

Codecov Report

❌ Patch coverage is 80.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.69%. Comparing base (76a65b6) to head (ffd8a17).
⚠️ Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
...roller/clusterversion/clusterversion_controller.go 80.00% 3 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2206      +/-   ##
==========================================
+ Coverage   50.31%   51.69%   +1.38%     
==========================================
  Files         288      288              
  Lines       34087    36358    +2271     
==========================================
+ Hits        17151    18797    +1646     
- Misses      15594    16169     +575     
- Partials     1342     1392      +50     
Files with missing lines Coverage Δ
pkg/constants/constants.go 100.00% <ø> (ø)
...shift/hive/apis/hive/v1/clusterdeployment_types.go 0.00% <ø> (ø)
...roller/clusterversion/clusterversion_controller.go 56.16% <80.00%> (+10.70%) ⬆️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2024
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 14, 2024
@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 13, 2024
@openshift-bot
Copy link

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci openshift-ci bot closed this Jul 14, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2024

@openshift-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.

@2uasimojo
Copy link
Member Author

/reopen
/remove-lifecycle rotten

@openshift-ci openshift-ci bot reopened this Aug 26, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 26, 2025

@2uasimojo: Reopened this PR.

Details

In response to this:

/reopen
/remove-lifecycle rotten

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-ci openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 26, 2025
@2uasimojo 2uasimojo force-pushed the HIVE-2366/copy-clusterversion branch from 46ebaf7 to 5af6c5f Compare August 26, 2025 19:59
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2025
@2uasimojo 2uasimojo force-pushed the HIVE-2366/copy-clusterversion branch 2 times, most recently from 0c829ca to fc15313 Compare August 29, 2025 18:19
@2uasimojo 2uasimojo force-pushed the HIVE-2366/copy-clusterversion branch from fc15313 to a8c3d1c Compare August 29, 2025 18:21
@2uasimojo
Copy link
Member Author

/assign @jstuever

/hold cancel

/cc @wking @lucasponce

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2025
@openshift-ci openshift-ci bot requested a review from lucasponce August 29, 2025 18:23
Add a new field, ClusterDeployment.Status.ClusterVersionStatus.

At the behest of an (undocumented, unsupported) annotation,
`hive.openshift.io/sync-clusterversion-status: "true"` (note: string
`"true"` since it's an annotation) our clusterversion controller (the
same one that updates the `hive.openshift.io/version*` labels) will
populate it with a wholesale copy of the spoke's ClusterVersion.Status
field.

NOTE: This requires that our vendor of openshift/api be kept up to date,
as e.g. new enum values for `capabilities` will break us.

HIVE-2366
@2uasimojo 2uasimojo force-pushed the HIVE-2366/copy-clusterversion branch from a8c3d1c to ffd8a17 Compare August 29, 2025 21:52
@2uasimojo
Copy link
Member Author

/hold for more reviews

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2025
Copy link
Member

@wking wking 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 Aug 29, 2025
@2uasimojo
Copy link
Member Author

/retest hive-mce-29-on-pull-request

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 29, 2025

@2uasimojo: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test coverage
/test e2e
/test e2e-azure
/test e2e-gcp
/test e2e-openstack
/test e2e-pool
/test e2e-vsphere
/test images
/test periodic-images
/test security
/test unit
/test verify

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-hive-master-coverage
pull-ci-openshift-hive-master-e2e
pull-ci-openshift-hive-master-e2e-pool
pull-ci-openshift-hive-master-images
pull-ci-openshift-hive-master-periodic-images
pull-ci-openshift-hive-master-security
pull-ci-openshift-hive-master-unit
pull-ci-openshift-hive-master-verify
Details

In response to this:

/retest hive-mce-29-on-pull-request

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.

// `clusterversion version` object. This is not officially supported, and is only populated
// on request.
// +optional
ClusterVersionStatus *configv1.ClusterVersionStatus `json:"clusterVersionStatus,omitempty"`

Choose a reason for hiding this comment

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

In case it's relevant to compare with Hypershift API

https://github.com/openshift/hypershift/blob/main/api/hypershift/v1beta1/hostedcluster_types.go#L1787

I'm not familiar with the motivation to create a struct rather than fetching the whole object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, we definitely considered doing it that way. Ultimately we landed on this approach mainly because it makes it so much easier to carry changes forward: we just have to revendor o/api. That's especially easy for hive as we don't operate under the same long release cadence as OCP itself. The only serious concern was CR space, but we convinced ourselves with live data that we're nowhere near the danger zone in that regard.

@lucasponce
Copy link

/lgtm

Minor comment about the decision that took Hypershift to copy the struct instead to reuse the type from openshift api.

But PR looks good, thanks Eric.

@jstuever
Copy link
Contributor

jstuever commented Sep 4, 2025

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, jstuever, lucasponce, wking

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@2uasimojo
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 5, 2025

@2uasimojo: all tests passed!

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.

@openshift-merge-bot openshift-merge-bot bot merged commit e10fdcc into openshift:master Sep 5, 2025
20 checks passed
@2uasimojo 2uasimojo deleted the HIVE-2366/copy-clusterversion branch September 5, 2025 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants