Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config/v1/types_cluster_version: Add architecture field #1219

Closed
wants to merge 1 commit into from

Conversation

jottofar
Copy link
Contributor

@jottofar jottofar commented Jul 5, 2022

As described in enhancement, add architecture fields to ClusterVersionSpec and to ClusterVersionStatus. The former indicates the desired architecture and the later indicates the current architecture. A cluster's architecture can be either homogeneous or heterogeneous. A single valid architecture value, e.g. "amd64", indicates an homogeneous cluster and "multi" indicates a heterogeneous cluster.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 5, 2022

Hello @jottofar! 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.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label command.

Who should apply these qe/docs/px labels?

  • For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
  • For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
  • For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

@openshift-ci openshift-ci bot requested review from jwforres and mfojtik July 5, 2022 20:58
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 5, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jottofar
To complete the pull request process, please assign knobunc after the PR has been reviewed.
You can assign the PR to them by writing /assign @knobunc in a comment when ready.

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

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

@jottofar
Copy link
Contributor Author

jottofar commented Jul 6, 2022

/retitle WIP: config/v1/types_cluster_version: Add archiecture field

@openshift-ci openshift-ci bot changed the title config/v1/types_cluster_version: Add archiecture field WIP: config/v1/types_cluster_version: Add archiecture field Jul 6, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2022
@jottofar
Copy link
Contributor Author

jottofar commented Jul 6, 2022

/retitle config/v1/types_cluster_version: Add architecture field

@openshift-ci openshift-ci bot changed the title WIP: config/v1/types_cluster_version: Add archiecture field config/v1/types_cluster_version: Add architecture field Jul 6, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2022
@@ -95,6 +95,13 @@ type ClusterVersionStatus struct {
// +required
Desired Release `json:"desired"`

// architecture indicates whether the cluster is a homogeneous or
Copy link
Contributor

Choose a reason for hiding this comment

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

What transitions are allowed for this value?

Copy link
Member

Choose a reason for hiding this comment

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

all transitions are theoretically allowed. multi -> amd64 and amd64 -> multi should be fairly straightforward. To do amd64 -> s390x, you'd need to do some fairly exciting dances, likely passing through multi in between.

Copy link

Choose a reason for hiding this comment

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

are we saying that if there is a transition from amd64->s390x, it will involve a middle stage of first moving to multi and then to the other single arch?

i'm wondering if single -> multi should be the only allowed transition.

@jottofar
Copy link
Contributor Author

/retest

As described in [enhancement], add architecture fields
to ClusterVersionSpec and to ClusterVersionStatus. The
former indicates the desired architecture and the later
indicates the current architecture. A cluster's
architecture can be either homogeneous or heterogeneous.
A single valid architecture value, e.g. "amd64",
indicates an homogeneous cluster and "multi" indicates
a heterogeneous cluster.

[enhancement]: https://github.com/openshift/enhancements/blob/836516786ce47bf74a8a3dfe8a2fa7f1143e94c9/enhancements/multi-arch/heterogeneous-architecture-clusters.md
// value of the cluster architecture. Setting this value will trigger
// an upgrade if the value does not match the current cluster
// architecture.
// +optional
Copy link
Contributor

Choose a reason for hiding this comment

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

What does empty mean?

// architecture identifies the architecture of the cluster nodes. If
// the cluster architecture is heterogeneous, whereby the cluster
// has nodes running on different architectures, "multi" is used.
// +optional
Copy link
Contributor

Choose a reason for hiding this comment

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

explain whether empty is a temporary condition or a long term condition to help consumers know what to expect.

@deads2k
Copy link
Contributor

deads2k commented Jul 13, 2022

Minor comments, then, lgtm. bug me on slack when updated.

@jottofar
Copy link
Contributor Author

/hold
Still a lot decisions to be worked out. Being tracked by https://issues.redhat.com/browse/OTA-658 and https://issues.redhat.com/browse/OTA-659

@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 Jul 15, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2022
@openshift-merge-robot
Copy link
Contributor

@jottofar: PR needs rebase.

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/test-infra repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2022

@jottofar: 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/integration a1577d7 link true /test integration

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

@jottofar
Copy link
Contributor Author

/close

@openshift-ci openshift-ci bot closed this Sep 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2022

@jottofar: Closed this PR.

In response to this:

/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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants