Skip to content

MULTIARCH-4559: pkg: Propagate Release.Architecture#1110

Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom
wking:status-architecture
Nov 20, 2024
Merged

MULTIARCH-4559: pkg: Propagate Release.Architecture#1110
openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom
wking:status-architecture

Conversation

@wking
Copy link
Copy Markdown
Member

@wking wking commented Nov 19, 2024

Pass the new property around in all clusters, but only propagate it out to status in the in-cluster ClusterVersion when the relevant feature gate is enabled. Also includes a vendor bump to pull in openshift/api#2024.

Pulling in [1].  Generated with:

  $ go get github.com/openshift/api@master
  $ go mod tidy
  $ go mod vendor
  $ git add -A go.* vendor

using:

  $ go version
  go version go1.23.1 linux/amd64

[1]: openshift/api#2024
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 19, 2024
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Nov 19, 2024

@wking: This pull request references MULTIARCH-4559 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

Details

In response to this:

Pass the new property around in all clusters, but only propagate it out to status in the in-cluster ClusterVersion when the relevant feature gate is enabled. Also includes a vendor bump to pull in openshift/api#2024.

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.

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2024
@wking wking force-pushed the status-architecture branch 5 times, most recently from 4c41ae5 to d2ed35b Compare November 20, 2024 18:39
Pass the new property around in all clusters, but only propagate it
out to 'status' in the in-cluster ClusterVersion when the relevant
feature gate is enabled.

I'm dropping the "report an error condition when architecture isn't
set" test-case, because now that a non-Multi Operator.release is
always defaulted to runtime.GOARCH in syncAvailableUpdates, we no
longer have a way to trigger architecture-less Cincinnati requests.

Setting 'Architecture: runtime.GOARCH' in
TestOperator_availableUpdatesSync/if_last_successful_check_time_was_too_recent,_do_nothing
avoids triggering a new request because the CVO thinks the
architecture has changed.  Without that pivot:

  $ go test -run TestOperator_availableUpdatesSync/if_last_successful_check_time_was_too_recent,_do_nothing ./pkg/cvo/
  I1120 11:23:53.053380   24806 cvo.go:731] Started syncing available updates "test/default"
  I1120 11:23:53.057477   24806 availableupdates.go:79] Retrieving available updates again, because the architecture has changed from "" to "amd64"
  I1120 11:23:53.068889   24806 availableupdates.go:398] Update service http://127.0.0.1:46459 could not return available updates: ResponseFailed: unexpected HTTP status: 500 Internal Server Error
  I1120 11:23:53.070095   24806 cvo.go:733] Finished syncing available updates "test/default" (16.782722ms)
  --- FAIL: TestOperator_availableUpdatesSync (0.04s)
      --- FAIL: TestOperator_availableUpdatesSync/if_last_successful_check_time_was_too_recent,_do_nothing (0.04s)
          cvo_test.go:2784: unexpected:   (*cvo.availableUpdates)(
              -   nil,
              +   &{
              +           UpdateService: "http://localhost:8080/graph",
              +           Channel:       "fast",
              +           Architecture:  "amd64",
              +           Condition: v1.ClusterOperatorStatusCondition{
  ...
@wking wking force-pushed the status-architecture branch from d2ed35b to 28b3b40 Compare November 20, 2024 20:05
@wking
Copy link
Copy Markdown
Member Author

wking commented Nov 20, 2024

/test any-multi-arch?

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 20, 2024

@wking: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test e2e-agnostic-operator
  • /test e2e-agnostic-operator-techpreview
  • /test e2e-agnostic-ovn
  • /test e2e-agnostic-ovn-upgrade-into-change
  • /test e2e-agnostic-ovn-upgrade-into-change-techpreview
  • /test e2e-agnostic-ovn-upgrade-out-of-change
  • /test e2e-agnostic-ovn-upgrade-out-of-change-techpreview
  • /test e2e-aws-ovn-techpreview
  • /test e2e-hypershift
  • /test e2e-hypershift-conformance
  • /test gofmt
  • /test images
  • /test lint
  • /test unit

The following commands are available to trigger optional jobs:

  • /test okd-scos-e2e-aws-ovn
  • /test okd-scos-images

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

  • pull-ci-openshift-cluster-version-operator-master-e2e-agnostic-operator
  • pull-ci-openshift-cluster-version-operator-master-e2e-agnostic-ovn
  • pull-ci-openshift-cluster-version-operator-master-e2e-agnostic-ovn-upgrade-into-change
  • pull-ci-openshift-cluster-version-operator-master-e2e-agnostic-ovn-upgrade-out-of-change
  • pull-ci-openshift-cluster-version-operator-master-e2e-aws-ovn-techpreview
  • pull-ci-openshift-cluster-version-operator-master-e2e-hypershift
  • pull-ci-openshift-cluster-version-operator-master-e2e-hypershift-conformance
  • pull-ci-openshift-cluster-version-operator-master-gofmt
  • pull-ci-openshift-cluster-version-operator-master-images
  • pull-ci-openshift-cluster-version-operator-master-lint
  • pull-ci-openshift-cluster-version-operator-master-okd-scos-e2e-aws-ovn
  • pull-ci-openshift-cluster-version-operator-master-unit
Details

In response to this:

/test any-multi-arch?

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.

Copy link
Copy Markdown
Contributor

@PratikMahajan PratikMahajan 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 Nov 20, 2024
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: PratikMahajan, 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:
  • OWNERS [PratikMahajan,wking]

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

@wking
Copy link
Copy Markdown
Member Author

wking commented Nov 20, 2024

e2e-aws-ovn-techpreview is using a single-arch release image (the only kind that Prow can currently build for CI), and there's no architecture set (as expected) in ClusterVersion status:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1110/pull-ci-openshift-cluster-version-operator-master-e2e-aws-ovn-techpreview/1859327528825524224/artifacts/e2e-aws-ovn-techpreview/gather-extra/artifacts/clusterversion.json | jq '.items[].status' | grep architecture
      "message": "Payload loaded version=\"4.18.0-0.ci.test-2024-11-20-201451-ci-op-fl24z3yn-latest\" image=\"registry.build05.ci.openshift.org/ci-op-fl24z3yn/release@sha256:84d1c2e402c84c60cab015bd617a567ad92add1d2dc6f43192324ca9b489cb9e\" architecture=\"amd64\"",

The CRD does have the expected property available:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1110/pull-ci-openshift-cluster-version-operator-master-e2e-aws-ovn-techpreview/1859327528825524224/artifacts/e2e-aws-ovn-techpreview/gather-must-gather/artifacts/must-gather.tar | tar -xOz registry-build05-ci-openshift-org-ci-op-fl24z3yn-stable-sha256-bc50366bd28cb4b3fa6b306a1d8e06544107c9df2a9e2dc5f1356d9585c99057/cluster-scoped-resources/apiextensions.k8s.io/customresourcedefinitions/clusterversions.config.openshift.io.yaml | yaml2json | jq '.spec.versions[].schema.openAPIV3Schema.properties.status' | grep architecture
          "architecture": {
            "description": "architecture is an optional field that indicates the\nvalue of the cluster architecture. In this context cluster\narchitecture means either a single architecture or a multi\narchitecture.\nValid values are 'Multi' and empty.",
              "architecture": {
                "description": "architecture is an optional field that indicates the\nvalue of the cluster architecture. In this context cluster\narchitecture means either a single architecture or a multi\narchitecture.\nValid values are 'Multi' and empty.",
        "architecture": {
          "description": "architecture is an optional field that indicates the\nvalue of the cluster architecture. In this context cluster\narchitecture means either a single architecture or a multi\narchitecture.\nValid values are 'Multi' and empty.",

And checking e2e-agnostic-ovn, we don't see the new property (because it's only tech-preview, and not yet part of the default feature set):

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1110/pull-ci-openshift-cluster-version-operator-master-e2e-agnostic-ovn/1859327528666140672/artifacts/e2e-agnostic-ovn/gather-must-gather/artifacts/must-gather.tar | tar -xOz registry-build09-ci-openshift-org-ci-op-fl24z3yn-stable-sha256-bc50366bd28cb4b3fa6b306a1d8e06544107c9df2a9e2dc5f1356d9585c99057/cluster-scoped-resources/apiextensions.k8s.io/customresourcedefinitions/clusterversions.config.openshift.io.yaml | yaml2json | jq '.spec.versions[].schema.openAPIV3Schema.properties.status' | grep -c architecture
0

And nothing in CVO logs that looks like "I tried to set architecture, but the Kube API server warned me that the property did not exist":

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1110/pull-ci-openshift-cluster-version-operator-master-e2e-agnostic-ovn/1859327528666140672/artifacts/e2e-agnostic-ovn/gather-extra/artifacts/pods/openshift-cluster-version_cluster-version-operator-7cf7684948-bnf8b_cluster-version-operator.log | grep -i architecture | grep -v 'Desired version from operator\|merge into existing history\|Synchronizing status\|Previous sync status'
I1120 21:20:46.700850       1 payload.go:402] Architecture from release-metadata (4.18.0-0.ci.test-2024-11-20-201646-ci-op-fl24z3yn-latest) retrieved from runtime: "amd64"
I1120 21:20:47.961366       1 start.go:590] CVO features for version 4.18.0-0.ci.test-2024-11-20-201646-ci-op-fl24z3yn-latest enabled at startup: {desiredVersion:4.18.0-0.ci.test-2024-11-20-201646-ci-op-fl24z3yn-latest unknownVersion:false reconciliationIssuesCondition:false statusReleaseArchitecture:false}
I1120 21:20:47.961433       1 featurechangestopper.go:123] Starting stop-on-features-change controller with startingRequiredFeatureSet="" startingCvoGates={desiredVersion:4.18.0-0.ci.test-2024-11-20-201646-ci-op-fl24z3yn-latest unknownVersion:false reconciliationIssuesCondition:false statusReleaseArchitecture:false}
I1120 21:20:47.961926       1 payload.go:402] Architecture from release-metadata (4.18.0-0.ci.test-2024-11-20-201646-ci-op-fl24z3yn-latest) retrieved from runtime: "amd64"
I1120 21:20:49.167932       1 sync_worker.go:431] Payload loaded from registry.build09.ci.openshift.org/ci-op-fl24z3yn/release@sha256:64813a6a0a859ad349d2dec6e3094e6526884eb9b7cdae72e97a248df53ca2c9 with hash Bqv_uxQZnnE=, architecture amd64
I1120 21:20:49.167980       1 event.go:377] Event(v1.ObjectReference{Kind:"ClusterVersion", Namespace:"openshift-cluster-version", Name:"version", UID:"", APIVersion:"config.openshift.io/v1", ResourceVersion:"", FieldPath:""}): type: 'Normal' reason: 'PayloadLoaded' Payload loaded version="4.18.0-0.ci.test-2024-11-20-201646-ci-op-fl24z3yn-latest" image="registry.build09.ci.openshift.org/ci-op-fl24z3yn/release@sha256:64813a6a0a859ad349d2dec6e3094e6526884eb9b7cdae72e97a248df53ca2c9" architecture="amd64"

So looks good to me, and I'm labeling this up so it can merge, and we can do post-merge testing in multi-arch nightlies, and we hopefully make 4.18's branch forking off :)

/label no-qe
/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci Bot added no-qe Allows PRs to merge without qe-approved label acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. labels Nov 20, 2024
@wking
Copy link
Copy Markdown
Member Author

wking commented Nov 20, 2024

I dunno what the API LBs follow /readyz of kube-apiserver and stop sending requests before server shutdowns for external clients failure is saying, but I can't see how it would be related to this pull.

/override ci/prow/e2e-hypershift-conformance

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 20, 2024

@wking: Overrode contexts on behalf of wking: ci/prow/e2e-hypershift-conformance

Details

In response to this:

I dunno what the API LBs follow /readyz of kube-apiserver and stop sending requests before server shutdowns for external clients failure is saying, but I can't see how it would be related to this pull.

/override ci/prow/e2e-hypershift-conformance

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
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 20, 2024

@wking: The following test 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/okd-scos-e2e-aws-ovn 28b3b40 link false /test okd-scos-e2e-aws-ovn

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 75706cd into openshift:master Nov 20, 2024
@wking wking deleted the status-architecture branch November 20, 2024 23:01
@openshift-bot
Copy link
Copy Markdown
Contributor

[ART PR BUILD NOTIFIER]

Distgit: cluster-version-operator
This PR has been included in build cluster-version-operator-container-v4.18.0-202411210007.p0.g75706cd.assembly.stream.el9.
All builds following this will include this PR.

case features.FeatureGateUpgradeStatus:
enabledGates.reconciliationIssuesCondition = true
case features.FeatureGateImageStreamImportMode:
enabledGates.statusReleaseArchitecture = false
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤦

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

#1111 up to fix

wking added a commit to wking/cluster-version-operator that referenced this pull request Nov 21, 2024
Fixing my sloppy copy/paste typo from 28b3b40 (pkg: Propagate
Release.Architecture, 2024-11-18, openshift#1110).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. no-qe Allows PRs to merge without qe-approved label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants