Skip to content

[WIP][OCPCLOUD-1381] Implement AWS Placement Group support and creation #16

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
Closed
JoelSpeed wants to merge 5 commits into from
Closed

[WIP][OCPCLOUD-1381] Implement AWS Placement Group support and creation #16

JoelSpeed wants to merge 5 commits into from

Conversation

@JoelSpeed
Copy link
Contributor

@JoelSpeed JoelSpeed commented Jan 10, 2022

This PR implements the changes required to allow users to add Machines to placement groups.

Additionally, should the placement group not exist, this PR allows the provider to create the placement group based on some pre-existing configuration within the placement struct.

This is currently blocked on openshift/api#1091, hence being marked WIP

@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 Jan 10, 2022
@openshift-ci openshift-ci bot requested review from elmiko and lobziik January 10, 2022 14:31
elmiko
elmiko reviewed Jan 10, 2022
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

this generally makes sense to me, i just have one minor question

pkg/actuators/machine/instances.go Outdated
func isAWS4xxError(err error) bool {
if _, ok := err.(awserr.Error); ok {
if reqErr, ok := err.(awserr.RequestFailure); ok {
if strings.HasPrefix(strconv.Itoa(reqErr.StatusCode()), "4") {
Copy link
Contributor

@elmiko elmiko Jan 10, 2022

Choose a reason for hiding this comment

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

do we need to convert the int to ascii here, couldn't we just check StatusCode() >= 400 && StatusCode() < 500?

Copy link
Contributor Author

@JoelSpeed JoelSpeed Jan 10, 2022

Choose a reason for hiding this comment

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

This was just copied from https://github.com/openshift/machine-api-provider-aws/pull/16/files#diff-c5079a7c5d2916c3661d0c44d1267050b3496c40b4530363d3825d66af581b6fR376, but you're right, that would make more sense, I'll update it

Copy link
Contributor

@elmiko elmiko Jan 10, 2022

Choose a reason for hiding this comment

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

no worries, i just thought converting to ascii seemed like a needless use of cycles

elmiko
elmiko reviewed Jan 10, 2022
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

thanks for the update, i'm good with this.
/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2022
@JoelSpeed JoelSpeed force-pushed the aws-placement-groups branch from 5b8d232 to 3fe0576 Compare January 10, 2022 16:57
@JoelSpeed JoelSpeed mentioned this pull request Jan 10, 2022
Merged
alexander-demicev
alexander-demicev approved these changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexander-demichev, elmiko

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 [alexander-demichev,elmiko]

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

@JoelSpeed JoelSpeed mentioned this pull request Jan 31, 2022
Closed
@JoelSpeed JoelSpeed force-pushed the aws-placement-groups branch from 540256d to f01a4d1 Compare February 4, 2022 11:37
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2022
@JoelSpeed JoelSpeed force-pushed the aws-placement-groups branch from f01a4d1 to fb83850 Compare February 7, 2022 10:10
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 7, 2022

@JoelSpeed: The following tests 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/e2e-aws-operator fb83850 link true /test e2e-aws-operator
ci/prow/e2e-aws-upgrade fb83850 link true /test e2e-aws-upgrade

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 8, 2022

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 8, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 8, 2022

@JoelSpeed: PR needs rebase.

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

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2022
@JoelSpeed
Copy link
Contributor Author

JoelSpeed commented May 9, 2022

This was superseded by other work

@JoelSpeed JoelSpeed closed this May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lobziik lobziik Awaiting requested review from lobziik

2 more reviewers

@elmiko elmiko elmiko left review comments

@alexander-demicev alexander-demicev alexander-demicev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JoelSpeed @openshift-bot @elmiko @alexander-demicev