feat: Placement Group Support #5307

lorrrrrrrenzo · 2023-12-12T11:27:54Z

Description
Allows for the configuration of a Placement Group when defining an AWSNodeTemplate resource. Based on existing work by @preflightsiren on PR #4553.

How was this change tested?

Does this change impact docs?

Yes, PR includes docs updates (in progress!)
Yes, issue opened: #
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

netlify · 2023-12-12T11:28:00Z

✅ Deploy Preview for karpenter-docs-prod canceled.

Name	Link
🔨 Latest commit	`bb8065f`
🔍 Latest deploy log	https://app.netlify.com/sites/karpenter-docs-prod/deploys/657843bd1f78d20008f6dd38

lorrrrrrrenzo · 2023-12-12T11:34:08Z

NB: I still need to import and update the documentation, ensure that the AWSNodeTemplate object is updated as intended (since this case is a bit different than than the OG PR), ensure all comments on the old PR addressed, and do some practical testing. CI is passing.

lorrrrrrrenzo · 2023-12-12T12:57:20Z

Regarding https://github.com/aws/karpenter-provider-aws/pull/4553/files#r1372675680

I agree that we shouldn't use multiple selectors for determining which Placement Group to use, and to go further I think we should allow users to define the spec of a Placement Group whose lifecycle would be tied to that of the NodePool, with Karpenter handling its creation, assignment, and cleanup. My sense is that it aligns with the Karpenter style to allow us to (in?)directly define resources that will solely be used to manage EC2 instances managed by Karpenter and with a particular NodePool.

The two configuration options could look like this:

# define the placementgroup inline
---
apiVersion: karpenter.k8s.aws/v1beta1
kind: EC2NodeClass
metadata:
  name: example-one
spec:
  # ...
  placementGroup:
    spec: 
      strategy: cluster # cluster | partition | spread
      spreadLevel: host # host | rack IF strategy = spread
      partitionCount: 1 # 1-7 | IF strategy = partition
---
# refer to a preexisting placement group
apiVersion: karpenter.k8s.aws/v1beta1
kind: EC2NodeClass
metadata:
  name: example-two
spec:
  # ...           
  placementGroup:
    selector: 
      matchLabels:
        karpenter.sh/placementgroup: "main"
        Name: "arbitrary-placementgroup-name"

A related comment: #4553 (comment)

lorrrrrrrenzo · 2023-12-12T13:11:14Z

pkg/controllers/nodeclass/controller.go

+		return err
+	}
+	if result != nil {
+		nodeClass.Status.PlacementGroups = append(nodeClass.Status.PlacementGroups, *result.GroupArn)


This comment is relevant here, but its instructions for proceeding are unclear

https://github.com/aws/karpenter-provider-aws/pull/4553/files#r1405582171

lorrrrrrrenzo · 2023-12-12T13:13:06Z

I believe I have linked all relevant comments from the previous PR. I will proceed with work on the CRDs and documentation once outstanding questions are addressed. Thanks!

jonathan-innis · 2023-12-12T16:24:44Z

pkg/apis/v1beta1/ec2nodeclass.go

+type PlacementGroupSelectorTerm struct {
+	// Name of the placement group to be selected
+	// +optional
+	Name string `json:"name,omitempty"`


I see that it's possible to also use tags here as well. I wonder if we also support tagging initially similar to the other selectors

One thing that's nice about tagging and naming is that it works consistently across accounts, so if you have similar set-ups for different accounts, everything just works out-of-the-box.

This is the general philosophy that Karpenter has taken towards selectors in the project, ensuring that we can move Karpenter from one account to another or from one region to another and everything "just works."

The trade-off of opening up this selector to tags is that now multiple placement groups can be returned, so how do you pick between them? There probably needs to be a consistent ordering and the selection between them shouldn't be random, particularly when it comes to the interaction of this feature with the drift feature.

jonathan-innis · 2023-12-12T16:30:12Z

pkg/apis/v1beta1/ec2nodeclass_status.go

@@ -63,6 +63,9 @@ type EC2NodeClassStatus struct {
 	// cluster under the AMI selectors.
 	// +optional
 	AMIs []AMI `json:"amis,omitempty"`
+	// PlacementGroups contains the ec2 placement group arns
+	// +optional
+	PlacementGroups []string `json:"placementGroups,omitempty"`


Should we resolve the strategy into the status block here? If the placement group type is partition, should we consider putting the number of partitions that we are using into the status as well?

jonathan-innis · 2023-12-12T16:38:21Z

pkg/apis/v1beta1/ec2nodeclass.go

@@ -167,6 +170,14 @@ type AMISelectorTerm struct {
 	Owner string `json:"owner,omitempty"`
 }

+// PlacementGroupSelectorTerm defines the selection logic for ec2 placement groups
+// that are used to launch nodes. If multiple fields are used for selection, the requirements are ANDed
+type PlacementGroupSelectorTerm struct {


There's a question here of how we should pass requirements down when using something like cluster placement groups OR when using something like spread with rack.

When using the spread with rack, the EC2 documentation mentions that you can't launch more than 7 instances in a single AZ (https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/placement-groups.html#:~:text=A%20rack%20spread%20placement%20group%20supports%20a%20maximum%20of%20seven%20running%20instances%20per%20Availability%20Zone), which means that we are restricted with our node limits when using this type of placement group. One option here is that we could constrain our requirements when using this type of placement group so that when we run out of instances to launch in a single AZ, we just get rid of that AZ from our requirements and start launching in other AZs. Another option here is that we allow to select on multiple placement groups and if we run out of space with one of the spread placement groups we just move to the other one.

This handle differently, though, when we are dealing with cluster or partition placement groups where there is no limit on the number of instances that can be launched and it makes less sense to allow for multiple placement groups to be selected on.

Based on all of these considerations, I think we should think about writing up a small design that considers the use-cases around placement groups, how they meld with requirements and what the API surface should look like.

coveralls · 2023-12-13T07:53:57Z

Pull Request Test Coverage Report for Build 7180793423

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

63 of 85 (74.12%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.1%) to 82.368%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/operator/operator.go	0	3	0.0%
pkg/controllers/nodeclass/controller.go	10	14	71.43%
pkg/fake/ec2api.go	6	10	60.0%
pkg/providers/amifamily/resolver.go	10	14	71.43%
pkg/providers/placementgroup/placementgroup.go	33	40	82.5%

Files with Coverage Reduction	New Missed Lines	%
pkg/operator/operator.go	1	9.47%

Totals
Change from base Build 7178284604:	-0.1%
Covered Lines:	4975
Relevant Lines:	6040

💛 - Coveralls

github-actions · 2023-12-27T12:04:14Z

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

github-actions · 2024-03-17T12:04:52Z

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

lorrrrrrrenzo · 2024-07-17T06:56:58Z

~~I would like this PR re-opened if possible~~

Copy/reconcile commits from PR by @preflightsiren

bb8065f

lorrrrrrrenzo requested a review from a team as a code owner December 12, 2023 11:27

lorrrrrrrenzo requested a review from ellistarn December 12, 2023 11:27

lorrrrrrrenzo commented Dec 12, 2023

View reviewed changes

jonathan-innis reviewed Dec 12, 2023

View reviewed changes

lorrrrrrrenzo mentioned this pull request Dec 25, 2023

docs: Placement Group Design Document #5389

Closed

3 tasks

github-actions bot added the lifecycle/stale label Dec 27, 2023

jonathan-innis added needs-design Design required and removed lifecycle/stale labels Jan 2, 2024

jonathan-innis added lifecycle/stale and removed needs-design Design required labels Mar 2, 2024

jonathan-innis self-assigned this Mar 2, 2024

github-actions bot removed the lifecycle/stale label Mar 3, 2024

github-actions bot added the lifecycle/stale label Mar 17, 2024

github-actions bot added the lifecycle/closed label Apr 1, 2024

github-actions bot closed this Apr 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Placement Group Support #5307

feat: Placement Group Support #5307

lorrrrrrrenzo commented Dec 12, 2023 •

edited

Loading

netlify bot commented Dec 12, 2023 •

edited

Loading

lorrrrrrrenzo commented Dec 12, 2023

lorrrrrrrenzo commented Dec 12, 2023 •

edited

Loading

lorrrrrrrenzo Dec 12, 2023

lorrrrrrrenzo commented Dec 12, 2023

jonathan-innis Dec 12, 2023

jonathan-innis Dec 12, 2023

jonathan-innis Dec 12, 2023

jonathan-innis Dec 12, 2023

coveralls commented Dec 13, 2023

github-actions bot commented Dec 27, 2023

github-actions bot commented Mar 17, 2024

lorrrrrrrenzo commented Jul 17, 2024 •

edited

Loading

feat: Placement Group Support #5307

feat: Placement Group Support #5307

Conversation

lorrrrrrrenzo commented Dec 12, 2023 • edited Loading

netlify bot commented Dec 12, 2023 • edited Loading

✅ Deploy Preview for karpenter-docs-prod canceled.

lorrrrrrrenzo commented Dec 12, 2023

lorrrrrrrenzo commented Dec 12, 2023 • edited Loading

lorrrrrrrenzo Dec 12, 2023

Choose a reason for hiding this comment

lorrrrrrrenzo commented Dec 12, 2023

jonathan-innis Dec 12, 2023

Choose a reason for hiding this comment

jonathan-innis Dec 12, 2023

Choose a reason for hiding this comment

jonathan-innis Dec 12, 2023

Choose a reason for hiding this comment

jonathan-innis Dec 12, 2023

Choose a reason for hiding this comment

coveralls commented Dec 13, 2023

Pull Request Test Coverage Report for Build 7180793423

Warning: This coverage report may be inaccurate.

💛 - Coveralls

github-actions bot commented Dec 27, 2023

github-actions bot commented Mar 17, 2024

lorrrrrrrenzo commented Jul 17, 2024 • edited Loading

lorrrrrrrenzo commented Dec 12, 2023 •

edited

Loading

netlify bot commented Dec 12, 2023 •

edited

Loading

lorrrrrrrenzo commented Dec 12, 2023 •

edited

Loading

lorrrrrrrenzo commented Jul 17, 2024 •

edited

Loading