Skip to content

CORS-4280: AWS: Add throughput validation for gp3 volumes#1430

Merged
openshift-merge-bot[bot] merged 4 commits intoopenshift:mainfrom
jhixson74:main_aws_gp3_throughput
Nov 25, 2025
Merged

CORS-4280: AWS: Add throughput validation for gp3 volumes#1430
openshift-merge-bot[bot] merged 4 commits intoopenshift:mainfrom
jhixson74:main_aws_gp3_throughput

Conversation

@jhixson74
Copy link
Copy Markdown
Member

Throughput can only be configured on gp3 volumes and must be between 125 and 2000 MiB's

This pull request depends on openshift/api#2480

@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 7, 2025
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Nov 7, 2025

@jhixson74: This pull request references CORS-4280 which is a valid jira issue.

Details

In response to this:

Throughput can only be configured on gp3 volumes and must be between 125 and 2000 MiB's

This pull request depends on openshift/api#2480

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.

@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 13, 2025

/assign @JoelSpeed @damdo

Copy link
Copy Markdown
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

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

/approve

/hold

For other reviews and in case you want to address my personal preference

Comment thread pkg/webhooks/machine_webhook.go
@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 Nov 13, 2025
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 13, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damdo

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2025
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 13, 2025

/test unit

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2025
@jhixson74 jhixson74 force-pushed the main_aws_gp3_throughput branch from bc526a8 to 710235e Compare November 14, 2025 04:26
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2025
@jhixson74 jhixson74 requested a review from damdo November 15, 2025 00:48
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 15, 2025

@jhixson74 Needs rebasing

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2025
@jhixson74 jhixson74 force-pushed the main_aws_gp3_throughput branch from 710235e to 38f58bd Compare November 17, 2025 17:14
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2025
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 17, 2025

@jhixson74 Needs fixing, compilation is broken after rebase

@jhixson74
Copy link
Copy Markdown
Member Author

@jhixson74 Needs fixing, compilation is broken after rebase

I would expect compilation to not work since the new API field is required. Am I missing something else?

@jhixson74
Copy link
Copy Markdown
Member Author

@jhixson74 Needs fixing, compilation is broken after rebase

I would expect compilation to not work since the new API field is required. Am I missing something else?

Nevermind. I found it . Fixes coming soon. Thanks for the heads up.

@jhixson74 jhixson74 force-pushed the main_aws_gp3_throughput branch from 38f58bd to cc98b67 Compare November 17, 2025 18:23
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 17, 2025

@jhixson74 looks like something might still be off

@jhixson74
Copy link
Copy Markdown
Member Author

@jhixson74 looks like something might still be off

I am not seeing anything other than the expected ThroughputMib error. I tested this locally with a patched openshift/api and build and tests were fine. What is off?

@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 18, 2025

# github.com/openshift/machine-api-operator/pkg/webhooks pkg/webhooks/machine_webhook.go:799:49: ebs.ThroughputMib undefined (type *"github.com/openshift/api/machine/v1beta1".EBSBlockDeviceSpec has no field or method ThroughputMib) pkg/webhooks/machine_webhook.go:804:27: ebs.ThroughputMib undefined (type *"github.com/openshift/api/machine/v1beta1".EBSBlockDeviceSpec has no field or method ThroughputMib)

according to the CI job

@jhixson74 jhixson74 force-pushed the main_aws_gp3_throughput branch 2 times, most recently from a9f7897 to d0598c3 Compare November 21, 2025 16:11
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 21, 2025

/test golint

@jhixson74 jhixson74 force-pushed the main_aws_gp3_throughput branch from d0598c3 to bb4779e Compare November 21, 2025 16:46
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 21, 2025

/test unit

@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 21, 2025

/retest

1 similar comment
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 22, 2025

/retest

Copy link
Copy Markdown
Member

@damdo damdo 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 22, 2025
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 22, 2025

/unhold

@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 Nov 22, 2025
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 22, 2025

/retest

1 similar comment
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 23, 2025

/retest

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 23, 2025

@jhixson74: 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 710235e 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.

@tthvo
Copy link
Copy Markdown
Member

tthvo commented Nov 24, 2025

/retest

@liweinan
Copy link
Copy Markdown

Verified with the build: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/release-openshift-origin-installer-launch-aws-modern/1993316592309506048

Used the test script from: openshift/release#71426

cd /Users/weli/works/oc-swarm/release && mkdir -p /Users/weli/works/oc-swarm/openshift-versions/work/cluster-profile && cp /Users/weli/.aws/credentials /Users/weli/works/oc-swarm/openshift-versions/work/cluster-profile/.awscred && cp /Users/weli/works/oc-swarm/openshift-versions/work/install-config.yaml.bkup /Users/weli/works/oc-swarm/openshift-versions/work/install-config.yaml && cp /Users/weli/works/oc-swarm/openshift-versions/work/auth/kubeconfig /Users/weli/works/oc-swarm/openshift-versions/work/kubeconfig && export CLUSTER_PROFILE_DIR=/Users/weli/works/oc-swarm/openshift-versions/work/cluster-profile && export SHARED_DIR=/Users/weli/works/oc-swarm/openshift-versions/work && ci-operator/step-registry/cucushift/installer/check/aws/rootvolume/cucushift-installer-check-aws-rootvolume-commands.sh |& tee /Users/weli/works/oc-swarm/openshift-versions/work/gp3-throughput-check.log
Cluster ID: weli-test3-d7j8h
Region: us-east-1
Expected throughput: 500 MiB/s
Expected worker rootVolume size: 130 GiB
Expected control-plane rootVolume size: 130 GiB
Checking worker nodes
PASS: ip-10-0-114-239.ec2.internal volume vol-05752be10666a276c type=gp3 size=130GiB iops=3000 throughput=500MiB/s
PASS: ip-10-0-134-253.ec2.internal volume vol-0bee845cdf74607ac type=gp3 size=130GiB iops=3000 throughput=500MiB/s
PASS: ip-10-0-74-88.ec2.internal volume vol-089c5c17df90e0414 type=gp3 size=130GiB iops=3000 throughput=500MiB/s
Checking control plane nodes
PASS: ip-10-0-107-238.ec2.internal volume vol-0e9c5ae15dd92af10 type=gp3 size=130GiB iops=3000 throughput=500MiB/s
PASS: ip-10-0-40-2.ec2.internal volume vol-0aa07aeff7282dba5 type=gp3 size=130GiB iops=3000 throughput=500MiB/s
PASS: ip-10-0-90-23.ec2.internal volume vol-003cebc10ca7bf3a0 type=gp3 size=130GiB iops=3000 throughput=500MiB/s
==========================================
Test Summary
All root volume throughput checks passed.

@liweinan
Copy link
Copy Markdown

/verified

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@liweinan: The /verified command must be used with one of the following actions: by, later, remove, or bypass. See https://docs.ci.openshift.org/docs/architecture/jira/#premerge-verification for more information.

Details

In response to this:

/verified

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.

@liweinan
Copy link
Copy Markdown

/verified by liweinan

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Nov 25, 2025
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@liweinan: This PR has been marked as verified by liweinan.

Details

In response to this:

/verified by liweinan

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-merge-bot openshift-merge-bot Bot merged commit 798f605 into openshift:main Nov 25, 2025
16 checks passed
jianlinliu added a commit to jianlinliu/machine-api-operator that referenced this pull request Nov 26, 2025
…_throughput"

This reverts commit 798f605, reversing
changes made to 1d78f2e.
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 26, 2025

/test ?

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. 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. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants