[OCPCLOUD-1380] Add Placement Group options to AWS machine provider config #1091

JoelSpeed · 2022-01-07T16:46:29Z

This PR implements the API changes described in openshift/enhancements#995.

This adds options to allow the use of the placement group feature within AWS. Notably users will be able to specify a placement group name to use and, if the placement group doesn't exist, will be able to have it created based on the placement group type, and, if applicable, partition count.

I have included constants for the three different types of placement group type as we will use these for validation of the field within the controller.

Note this enhancement is targeted for 4.11, we are hoping to merge this shortly after master opens for development.

alexander-demicev

/lgtm

JoelSpeed · 2022-02-11T10:04:43Z

/test verify

damdo

Two very small nits, otherwise LGTM

damdo · 2022-02-11T10:27:05Z

machine/v1beta1/types_awsplacementgroup.go

+// +kubebuilder:printcolumn:name="Type",type="string",JSONPath=".spec.groupType",description="Placement Group Type"
+// +kubebuilder:printcolumn:name="Management",type="string",JSONPath=".spec.managementState",description="Management State"
+// +kubebuilder:printcolumn:name="Replicas",type="integer",JSONPath=".status.replicas",description="EC2 Replicas within the Placement Group"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="  AWSPlacementGroup age"


There are a couple extra spaces here at the beginning of the description. Is there a specific reason (like some inlining) or is it a typo?

damdo · 2022-02-11T10:43:40Z

machine/v1beta1/types_awsplacementgroup.go

+	// Note the partition count of a placement group cannot be changed after creation.
+	// If unset, AWS will provide a default partition count.
+	// This default is currently 2.
+	// Note: When using more than 2 partitions, placement.tenancy "dedicated" on Machines


small nit: Noticed placement.tenancy is lowercase here but uppercase within the AWSPlacementGroupUnion type. It'd be good to have them similar.

perhaps we could make this a little clearer too, eg

Suggested change

// Note: When using more than 2 partitions, placement.tenancy "dedicated" on Machines

// Note: When using more than 2 partitions, the "dedicated" tenancy option on Machines

JoelSpeed

Thanks for the review @damdo, have resolved both typos

damdo

Great!
/lgtm

elmiko

generally lgtm, i have one question and a suggestion

machine/v1beta1/0000_10_awsplacementgroup.crd.yaml

elmiko · 2022-02-11T18:34:01Z

machine/v1beta1/types_awsplacementgroup.go

+	// Note the partition count of a placement group cannot be changed after creation.
+	// If unset, AWS will provide a default partition count.
+	// This default is currently 2.
+	// Note: When using more than 2 partitions, placement.tenancy "dedicated" on Machines


perhaps we could make this a little clearer too, eg

Suggested change

// Note: When using more than 2 partitions, placement.tenancy "dedicated" on Machines

// Note: When using more than 2 partitions, the "dedicated" tenancy option on Machines

deads2k · 2022-02-14T21:09:08Z

machine/v1beta1/types_awsplacementgroup.go

@@ -0,0 +1,164 @@
+package v1beta1


Since we support upgrades on this and we know we'll never want to deal with migration of customer manifests, I think this is really v1.

Good catch, I had actually said in the enhancement we would add this as V1 🤦

deads2k · 2022-02-14T21:10:13Z

machine/v1beta1/types_awsplacementgroup.go

+	// to be managed by this CRD or whether it is user managed.
+	// A managed placement group may be moved to unmanaged, however an unmanaged
+	// group may not be moved back to managed.
+	// +kubebuilder:default:=Managed


Will we actually manage any of these and actively reconcile them? If so, this fine. If not, and you only want to support Unmanaged, I'm willing to allow Unmanaged only.

We are hoping to deliver a dev preview to a customer next week in which we will not manage the placement groups, however, before 4.11 ships we intend to implement the management. The customer wants us to have a declarative way to create/delete hundreds of placement groups and MachineSets so this will definitely be required

deads2k · 2022-02-14T21:10:23Z

machine/v1beta1/types_awsplacementgroup.go

+	// A managed placement group may be moved to unmanaged, however an unmanaged
+	// group may not be moved back to managed.
+	// +kubebuilder:default:=Managed
+	// +optional


If I make this required, then the defaulting doesn't work, do you think it's worth making customers explicitly specify that this is Managed vs defaulting it for them?

deads2k · 2022-02-14T21:10:52Z

machine/v1beta1/types_awsplacementgroup.go

+	ManagementState ManagementState `json:"managementState"`
+
+	// CredentialsSecret is a reference to the secret with AWS credentials. Otherwise, defaults to permissions
+	// provided by attached IAM role where the actuator is running.


on openshift, where is the actuator and how can I tell its permissions?

This is the equivalent of the Machine IAM role, this is an AWSism primarily, I will update the comment to reflect that this will be the control plane Machine IAM role

alexander-demicev

/lgtm

elmiko

/lgtm

deads2k · 2022-02-16T15:19:40Z

machine/v1/types_awsplacementgroup.go

+	// group may not be moved back to managed.
+	// +kubebuilder:default:=Managed
+	// +optional
+	ManagementState ManagementState `json:"managementState"`


per slack, this and awsplacementgroupunion for a discriminated union distinct from credentialsSecret

Agreed, that makes sense, this is now pushed

deads2k · 2022-02-16T18:21:49Z

machine/v1/types_awsplacementgroup.go

+
+type AWSPlacementGroupSpec struct {
+	// AWSPlacementGroupManagementSpec defines the configuration for a managed or unmanaged placement group.
+	AWSPlacementGroupManagementSpec `json:",inline"`


you cannot have this inline because the credentialsSecret isn't controlled by the the value of the managementstate.

I thought this was ok, there's an example doing the same (TopLevelUnion) in the KEP, do you know what this would break/why this wouldn't work? Is it a UX thing rather than a technical issue?

deads2k · 2022-02-16T18:22:02Z

machine/v1/types_awsplacementgroup.go

+	AWSPlacementGroupManagementSpec `json:",inline"`
+
+	// CredentialsSecret is a reference to the secret with AWS credentials. The secret must reside in the same namespace
+	// as the AWSPlacementGroup resourec. Otherwise, the controller will leverage the EC2 instance assigned IAM Role,


typo: resource

deads2k · 2022-02-16T18:23:16Z

machine/v1/types_awsplacementgroup.go

+	// group may not be moved back to managed.
+	// +kubebuilder:default:=Managed
+	// +unionDiscriminator
+	// +optional


A user is already going to have to set the grouptype. I would make this required and not defaulted.

deads2k · 2022-02-16T18:23:45Z

machine/v1/types_awsplacementgroup.go

+	GroupType AWSPlacementGroupType `json:"groupType,omitempty"`
+
+	// +optional
+	Partition *AWSPartitionPlacement `json:"partition,omitempty"`


missing godoc.

deads2k · 2022-02-16T18:25:55Z

the remaining comments are minor, but required before merge.

JoelSpeed · 2022-02-21T09:05:59Z

@deads2k This has now been updated, if you are happy with the changes, I'll squash and we can get this merged

deads2k · 2022-02-21T20:55:44Z

/lgtm

JoelSpeed · 2022-02-22T10:06:32Z

Squashed the commits before merge, no other changes since last review

This project is not part of the no-ff process, manually adding the appropriate labels

/label docs-approved
/label px-approved
/label qe-approved

openshift-ci · 2022-02-22T10:19:07Z

@JoelSpeed: all tests passed!

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.

damdo

/lgtm

openshift-ci · 2022-02-22T10:19:49Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [deads2k]

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

openshift-ci bot requested review from alexander-demicev and mandre January 7, 2022 16:47

JoelSpeed force-pushed the mapi-aws-placement-groups branch 2 times, most recently from d1bbaa7 to 383fb55 Compare January 10, 2022 10:39

JoelSpeed mentioned this pull request Jan 10, 2022

[WIP][OCPCLOUD-1381] Implement AWS Placement Group support and creation openshift/machine-api-provider-aws#16

Closed

alexander-demicev approved these changes Jan 11, 2022

View reviewed changes

openshift-ci bot assigned alexander-demicev Jan 11, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2022

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2022

JoelSpeed force-pushed the mapi-aws-placement-groups branch 5 times, most recently from 641c952 to e04688d Compare February 11, 2022 09:37

JoelSpeed force-pushed the mapi-aws-placement-groups branch from e04688d to e407135 Compare February 11, 2022 10:10

JoelSpeed mentioned this pull request Feb 11, 2022

Update OpenShift API to include placement groups openshift/machine-api-operator#992

Merged

damdo reviewed Feb 11, 2022

View reviewed changes

JoelSpeed force-pushed the mapi-aws-placement-groups branch from e407135 to 875e760 Compare February 11, 2022 12:16

JoelSpeed commented Feb 11, 2022

View reviewed changes

damdo reviewed Feb 11, 2022

View reviewed changes

openshift-ci bot assigned damdo Feb 11, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2022

JoelSpeed force-pushed the mapi-aws-placement-groups branch from 875e760 to 827650e Compare February 11, 2022 13:50

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2022

elmiko reviewed Feb 11, 2022

View reviewed changes

deads2k reviewed Feb 14, 2022

View reviewed changes

alexander-demicev approved these changes Feb 16, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2022

elmiko reviewed Feb 16, 2022

View reviewed changes

openshift-ci bot assigned elmiko Feb 16, 2022

deads2k reviewed Feb 16, 2022

View reviewed changes

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2022

deads2k reviewed Feb 16, 2022

View reviewed changes

openshift-ci bot assigned deads2k Feb 21, 2022

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 21, 2022

Add Placement Group options to AWS machine provider config

dfab187

JoelSpeed force-pushed the mapi-aws-placement-groups branch from fdd63b3 to dfab187 Compare February 22, 2022 10:05

openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR and removed lgtm Indicates that a PR is ready to be merged. labels Feb 22, 2022

damdo reviewed Feb 22, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2022

openshift-merge-robot merged commit 354aa98 into openshift:master Feb 22, 2022

JoelSpeed deleted the mapi-aws-placement-groups branch February 22, 2022 10:23

JoelSpeed mentioned this pull request Mar 3, 2022

Bug 2060697: Correct json tag on AWS Placement PartitionNumber #1131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OCPCLOUD-1380] Add Placement Group options to AWS machine provider config #1091

[OCPCLOUD-1380] Add Placement Group options to AWS machine provider config #1091

JoelSpeed commented Jan 7, 2022

alexander-demicev left a comment

JoelSpeed commented Feb 11, 2022

damdo left a comment

damdo Feb 11, 2022

damdo Feb 11, 2022

elmiko Feb 11, 2022

JoelSpeed left a comment

damdo left a comment

elmiko left a comment

elmiko Feb 11, 2022

deads2k Feb 14, 2022

JoelSpeed Feb 15, 2022

deads2k Feb 14, 2022

JoelSpeed Feb 15, 2022

deads2k Feb 14, 2022

JoelSpeed Feb 15, 2022

deads2k Feb 14, 2022

JoelSpeed Feb 15, 2022

alexander-demicev left a comment

elmiko left a comment

deads2k Feb 16, 2022

JoelSpeed Feb 16, 2022

deads2k Feb 16, 2022

JoelSpeed Feb 17, 2022

deads2k Feb 16, 2022

deads2k Feb 16, 2022

deads2k Feb 16, 2022

deads2k commented Feb 16, 2022

JoelSpeed commented Feb 21, 2022

deads2k commented Feb 21, 2022

JoelSpeed commented Feb 22, 2022 •

edited

openshift-ci bot commented Feb 22, 2022

damdo left a comment

openshift-ci bot commented Feb 22, 2022

	// Note: When using more than 2 partitions, placement.tenancy "dedicated" on Machines
	// Note: When using more than 2 partitions, the "dedicated" tenancy option on Machines

[OCPCLOUD-1380] Add Placement Group options to AWS machine provider config #1091

[OCPCLOUD-1380] Add Placement Group options to AWS machine provider config #1091

Conversation

JoelSpeed commented Jan 7, 2022

alexander-demicev left a comment

Choose a reason for hiding this comment

JoelSpeed commented Feb 11, 2022

damdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

damdo left a comment

Choose a reason for hiding this comment

elmiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-demicev left a comment

Choose a reason for hiding this comment

elmiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Feb 16, 2022

JoelSpeed commented Feb 21, 2022

deads2k commented Feb 21, 2022

JoelSpeed commented Feb 22, 2022 • edited

openshift-ci bot commented Feb 22, 2022

damdo left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 22, 2022

JoelSpeed commented Feb 22, 2022 •

edited