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.

Sign up for GitHub

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

Jump to bottom

OSASINFRA-2571: Add quota validation for server group #9326

Merged
merged 1 commit into from
Jan 29, 2025
Merged

OSASINFRA-2571: Add quota validation for server group #9326

merged 1 commit into from
Jan 29, 2025

Conversation

dkokkino
Copy link
Contributor

No description provided.

@openshift-ci-robot
Copy link
Contributor

@dkokkino: No Jira issue with key OSAINFRA-2571 exists in the tracker at https://issues.redhat.com/.
Once a valid jira issue is referenced in the title of this pull request, request a refresh with /jira refresh.

In response to this:

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

openshift-ci bot commented Dec 18, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@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 Dec 18, 2024
@MaysaMacedo
Copy link
Contributor

MaysaMacedo commented Dec 18, 2024 •
edited by openshift-ci bot
Loading

/retitle OSASINFRA-2571: Add quota validation for server group

@openshift-ci openshift-ci bot changed the title OSAINFRA-2571: Add quota validation for server group OSASINFRA-2571: Add quota validation for server group Dec 18, 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 Dec 18, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 18, 2024 •
edited by openshift-ci bot
Loading

@dkokkino: This pull request references OSASINFRA-2571 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 task to target the "4.19.0" version, but no target version was set.

In response to this:

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.

@MaysaMacedo
Copy link
Contributor

MaysaMacedo commented Dec 30, 2024 •
edited
Loading

@dkokkino Great work!
This PR looks ready for review to me. Is there anything missing? If not, do you mind converting it from draft to ready for review so we can run CI and could you squash the commits into just one?

@dkokkino dkokkino marked this pull request as ready for review January 2, 2025 14:28
@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 Jan 2, 2025
@openshift-ci openshift-ci bot requested review from gryf and stephenfin January 2, 2025 14:29
@dkokkino dkokkino force-pushed the OSAINFRA-2571 branch 3 times, most recently from 0feda0f to 5bc0e7e Compare January 2, 2025 15:44
@dkokkino
Copy link
Contributor Author

dkokkino commented Jan 8, 2025

/test e2e-openstack-proxy

stephenfin
stephenfin reviewed Jan 8, 2025
View reviewed changes
Copy link
Contributor

@stephenfin stephenfin left a comment

Choose a reason for hiding this comment

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

A nit and a question inline. Looking good otherwise.

pkg/asset/quota/openstack/openstack.go
@@ -53,6 +53,13 @@ func Constraints(ci *validation.CloudInfo, controlPlanes []machineapi.Machine, c
if len(ci.ControlPlanePortSubnets) == 0 {
constraints = append(constraints, networkConstraint(1), routerConstraint(1), subnetConstraint(1))
}
// if the cluster does not have worker nodes then reduce the server group value from 2 to 1
numServerGroups := int64(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to wrap in int64 rather than just using "raw" values? I'm guessing this is consistency, but it'd be nice to know why we did it in the first place 😄

Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly, the number of workers is int64 in install-config. The rest of the numbers can be automatic ints I believe

Copy link
Member

Choose a reason for hiding this comment

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

I see that this is still int64. Was it not possible to use int?

pkg/asset/quota/openstack/openstack.go Outdated
Comment on lines 173 to 180

func serverGroupsConstraint(count int64) quota.Constraint {
return generateConstraint("ServerGroups", count)
}

func serverGroupMembersConstraint(count int64) quota.Constraint {
return generateConstraint("ServerGroupMembers", count)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we gather these with the other compute-related constraints? You could move these up to line 150.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay i have grouped the constraints.

@dkokkino
Add quota validation for server group
e7187d0 
- Added a quota constraint for server groups with a default of 2, reducing to 1 when no worker nodes are provisioned.
- Added a quota constraint for server group members, equal to the number of instances provisioned.
stephenfin
stephenfin reviewed Jan 9, 2025
View reviewed changes
Copy link
Contributor

@stephenfin stephenfin 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 Jan 9, 2025
@stephenfin
Copy link
Contributor

/retest

@dkokkino
Copy link
Contributor Author

/test e2e-openstack-ovn

@dkokkino
Copy link
Contributor Author

/test e2e-openstack-proxy
/test e2e-openstack-ovn

@stephenfin
Copy link
Contributor

/approve

Passing all jobs now.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 20, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stephenfin

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2025
@sadasu
Copy link
Contributor

sadasu commented Jan 28, 2025

/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Jan 28, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 29, 2025 •
edited
Loading

@dkokkino: 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 e7187d0 link false /test okd-scos-e2e-aws-ovn

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.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD fb88402 and 2 for PR HEAD e7187d0 in total

@openshift-merge-bot openshift-merge-bot bot merged commit 44b681d into openshift:main Jan 29, 2025
21 of 22 checks passed
@openshift-merge-bot openshift-merge-bot bot deleted the OSAINFRA-2571 branch January 29, 2025 07:58
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-altinfra
This PR has been included in build ose-installer-altinfra-container-v4.19.0-202501290837.p0.g44b681d.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-baremetal-installer
This PR has been included in build ose-baremetal-installer-container-v4.19.0-202501290837.p0.g44b681d.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-terraform-providers
This PR has been included in build ose-installer-terraform-providers-container-v4.19.0-202501290837.p0.g44b681d.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-artifacts
This PR has been included in build ose-installer-artifacts-container-v4.19.0-202501290837.p0.g44b681d.assembly.stream.el9.
All builds following this will include this PR.

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

@pierreprinetti pierreprinetti pierreprinetti left review comments

@stephenfin stephenfin stephenfin left review comments

@gryf gryf Awaiting requested review from gryf

Assignees

@stephenfin stephenfin

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dkokkino @openshift-ci-robot @MaysaMacedo @stephenfin @sadasu @openshift-bot @pierreprinetti