Add enhancement for AWS Placement Groups in Machine API

[JoelSpeed]

This PR adds a new enhancement describing how we will integrate support for AWS Placement Groups into the Machine API.

Notably, as we have a specific customer requirement, we are not planning to introduce this into the installer initially as this is not required by the customer. This has been left as a future implementation and is noted within the enhancement.

[sdodson]

/uncc @imcleod @kbsingh
/cc @sdodson @deads2k @danwinship

[jeana-redhat]

Noted a few things while reading - pretty sure my questions don't need to be addressed until closer to actually creating product docs for this.

[jeana-redhat]

Suggested change

	workloads are unlikely to suffer from hardware failures simultanesouly.
	workloads are unlikely to suffer from hardware failures simultaneously.

[jeana-redhat]

HPC

I don't see this term in existing product docs. Google tells me it stands for "high-performance computing", I'm assuming that is the meaning here?

[JoelSpeed]

Yeah that's my bad, i should have spelled that out on first usage, will update. You're correct

[jeana-redhat]

All good. Mostly I am wondering if we refer to this in prod docs with some other term, since it seems like the idea of HPC ought to have come up. But that's for me to dig in on :)

[elmiko]

this generally reads well to me @JoelSpeed. i added a comment about the CAO that we discussed during our team standup. my only other comment is that i think it would be nice to call more attention to the idea that this feature is highly sensitive to user configuration, but i'm not sure where that might fit, maybe drawbacks?

[danwinship]

(It's not unique to AWS; Azure and GCP have similar features.)

[JoelSpeed]

I was trying to highlight that I don't think there's another way to place groups of instances together in AWS, so there is no alternative, I'll try to reword this

[danwinship]

maybe "AWS recommends launching ..." rather than "it is recommended to launch ..." to make it clear that we aren't recommending that users do something that we don't actually let them do...

[danwinship]

maybe clarify somehow that this isn't about "OpenShift Dedicated"? (likewise below)

[JoelSpeed]

Will prefix with AWS for now, perhaps linking to the AWS docs might help

[danwinship]

Suggested change

	- Do we need to propogate user defined tags from the Infrastructure resource onto the placement groups we create?
	- Do we need to propagate user defined tags from the Infrastructure resource onto the placement groups we create?

[danwinship]

It seems like maybe if the placement group already exists, that it should ensure that it has the correct type, and error out if not?

Otherwise someone might accidentally use the same GroupName for two different MachineSets with different placement needs.

(If MachineSets and placement groups were 1-to-1 then the placement group name could just be based off the MachineSet name...)

[JoelSpeed]

It seems like maybe if the placement group already exists, that it should ensure that it has the correct type, and error out if not?

We could implement that, though at the moment, the group type is optional, so would it make sense to ignore the check if the placement group type is empty?

Otherwise someone might accidentally use the same GroupName for two different MachineSets with different placement needs.

It is valid to use the same placement group across multiple machinesets, though yes, I can see the intention here, I think if the user has set an intended type, then checking against this should be safe. The get out jail card is to either fix the type or remove the type field

(If MachineSets and placement groups were 1-to-1 then the placement group name could just be based off the MachineSet name...)

This isn't necessarily true as there are valid use cases for using the same placement group across multiple machinesets, have mentioned this elsewhere

[sdodson]

Very well put together enhancement, just a few questions and comments.

[deads2k]

if I render manifests and then modify them, will I be able to set a PlacementGroup?

[JoelSpeed]

Yes for worker nodes, this won't work for control plane machines though as we don't use MAPI to create them. Will clarify this statement to show this is workers only

[deads2k]

If you claim this, I'd like like to see exactly how this will happen. If you choose to permanently exclude it, you don't have to design it.

[JoelSpeed]

Heh, so the future automation I'm referring to is what I'm designing in #1008, now that the PR is up, I could link to this doc I guess but I think given how early that other enhancement is in review, I might wait a bit

[deads2k]

is this a current customer requirement or just explaining how else this could be used? I'm ok with the latter, but I'd like to understand

[JoelSpeed]

This one is not a current customer requirement as far as I understand. The customer ask we have that's driving this is for using Cluster placement groups, which is the second user story.

I want to implement this feature fully so we've explored all of the use cases and added users stories for each

[deads2k]

this is part of AWSMachineProviderConfig. How many of these are present in our standard installation?

[JoelSpeed]

This is part of AWSMachineProviderConfig, it is a singleton within the provider config.

On AWS, we create 3 MachineSets and 3 control plane Machines, each of those will contain an AWSMachineProviderConfig, and the MachineSets each create 1 Machine by default.

If a user wants to add placement groups during install, they probably want different placement groups per MachineSet, so I'd expect them to make up to 3 edits during the manifest phase

[staebler]

The installer will create 1 MachineSet for each availability zone. By default, the installer will include all availability zones in a region. So, taking us-east-1 as an example, not 3 edits but rather 6 edits.

[deads2k]

This description really makes this sound like it should be part of discriminated union.

[JoelSpeed]

So I've added the type as an explicit option so we know what the customer wants us to do, as there (currently) is no configuration apart from for partition types, I didn't think we would need to add another struct level to make it a proper discriminated union, but I guess there's no harm in it

To clarify you're suggestion is:

type Placement struct {
  Region
  AvailabilityZone
  Tenancy
  GroupType AWSPlacementGroupType
  GroupName string
  Partition *AWSPartitionPlacement
}

type AWSPartitionPlacement struct {
  Count int32
  Number int32
}

[sdodson]

I'm wondering if @deads2k was suggesting that the entire placement group policy should be a discriminating union?

[Miciah]

That example makes sense to me, but I would suggest breaking it out into a separate type, which could be inlined:

type Placement struct {
  // ...
  GroupUnion  `json:",inline"`
}

// +union
type GroupUnion struct {
  // +unionDiscriminator
  // +kubebuilder:validation:Required
  // +required
  GroupType AWSPlacementGroupType

  // +optional
  Cluster *AWSClusterPlacement

  // +optional
  Partition *AWSPartitionPlacement

  // +optional
  Spread *AWSSpreadPlacement  
}

type AWSClusterPlacement struct {}

type AWSPartitionPlacement struct {
  Count int32
  Number int32
}

type AWSSpreadPlacement struct {}

[JoelSpeed]

I can do this yep, though, I'm not hugely familiar with the union types, does this mean users have to set one of the structs to be non-empty? Or can they just set the type for Cluster or Spread and omit the struct? Also, do we even need to add those structs to the API if they are empty/not required right now?

[Miciah]

The user can specify the discriminator, or specify one of the structs, or both. I think you can omit the empty structs and corresponding fields if you prefer.

[deads2k]

can you link to the current docs for me?

[JoelSpeed]

Do you mean product docs? https://docs.openshift.com/container-platform/4.9/machine_management/creating_machinesets/creating-machineset-aws.html

[deads2k]

The existing fields in Placement are

1. Region string json:"region,omitempty"
2. AvailabilityZone string json:"availabilityZone,omitempty"
3. Tenancy InstanceTenancy json:"tenancy,omitempty"

Can you describe how these interact with cluster, partition, and spread options? Are they expected to empty when setting a groupName? Does Spread make sense if there's only one AZ?

[JoelSpeed]

Region and AvailabilityZone remain the same, there's no effect apart from you may want to create multiple machinesets across a single AZ to make use of a partition placement group, though we would normally just spread machines across AZs

Tenancy is affected because you can't use dedicated hosts with certain placement group types, this is called out in the sections where I talk about each type of placement group

Does Spread make sense if there's only one AZ?

Yes! Placement groups are a way of defining how your EC2 instances are laid out within a AZ, as opposed to across AZs. For spread in particular, each AZ in AWS is divided into 7 sections, each section having independent power and network for HA. When you create a spread placement group, it will place at most 1 instance in each of the 7 sections. If you use the same PG across 2 AZs, you then in theory have 14 sections, 7 in each AZ.

[deads2k]

can I use it for muitiple availability zones normally?

[JoelSpeed]

There's nothing to stop you using it across multiple availability zones, but a MachineSet is tied to a single AZ, so each MachineSet gains a limit of seven instances when it has a spread placement group (unless they create more than one machineset in the same zone, in which case, fewer than 7 I would expect)

[deads2k]

is "spread" equivalent to our current behavior? I honestly don't know.

[JoelSpeed]

No. The current behaviour is that an ec2 instance we create can go anywhere within the AZ, just gonna paste a response from another comment as I think it explains the difference

For spread in particular, each AZ in AWS is divided into 7 sections, each section having independent power and network for HA. When you create a spread placement group, it will place at most 1 instance in each of the 7 sections. If you use the same PG across 2 AZs, you then in theory have 14 sections, 7 in each AZ.

In our current behaviour, we could quite easily create 5 instances and have them all end up in the same section of the datacenter, meaning a power failure in that section would wipe out all instances

I've discussed this with @sdodson and we agreed that there isn't actually a sane default. If the value is omitted and we need to create a placement group, that should be considered an error, I've updated the description here.

[deads2k]

cam you provide an example of using this later in the enhancement? i think it's too big for godoc here, but I don't quite see it.

[JoelSpeed]

Yep, have added a yaml example to the Partition Placement Groups heading

[deads2k]

when someone tries to scale beyond this, how will the failure be presented to the user?

[JoelSpeed]

An error will be presented to them on the Machine, which will have moved into the Failed phase, this should be sufficient to prompt them that they can't add a new instance to the placement group

[deads2k]

why should we delete a placement group that we did not create?

[JoelSpeed]

We shouldn't, we will only delete placement groups that look like they were created by MAPI (tagged appropriately) and also only in the case when we can recreate the PG if required later (ie we know what groupType it is)

[Miciah]

Does this mean we will still leak a placement group in the following scenario?

1. User specifies a placement group on a Machine or MachineSet.
2. OpenShift creates the placement group.
3. User creates an instance outside of OpenShift and assigns it to the placement group.
4. User deletes the Machine or MachineSet from step 1.
5. OpenShift observes that the placement group is still in use and does not delete it.
6. User deletes the instance from step 3.

Maybe that's fine since the user is doing funky things in this example. Anyway, I guess the installer's cluster deprovisioning logic would still clean up any unused placement group with the cluster tag.

[JoelSpeed]

Maybe that's fine since the user is doing funky things in this example. Anyway, I guess the installer's cluster deprovisioning logic would still clean up any unused placement group with the cluster tag.

You're right, we would "leak" it in this case, but, this isn't particularly bad as placement groups are a no-cost feature of AWS, the only problem would be if the customer leaked, by doing this process, a lot of placement groups and then reached the 500 group limit

The installer deprovision should catch it though as I've already implemented that and tested it before I'd implemented the removal logic in the POC

[deads2k]

"minimise" how very british. :) I don't think we ever made a doc declaring american spelling as standard, but now I secretly suspect those people who add spell checkers of trying to backdoor it!

Entirely tongue in cheek, the spelling is fine :)

[JoelSpeed]

I don't think we ever made a doc declaring american spelling as standard, but now I secretly suspect those people who add spell checkers of trying to backdoor it!

If I spell it with a z (pronounced "zed") then I get red squiggly line under it ;)

I have wondered about whether we had an official spelling recommendation, I know product docs have to be american english but this isn't customer facing (or is it?)

Until I'm forced to write and speak in american english, I'll stick to british and keep pronouncing quay in the british way as well ;)

[Miciah]

how very british

The Britishism that trips me up the most is the parsimonious spelling of "fulfil".

I have wondered about whether we had an official spelling recommendation, I know product docs have to be american english but this isn't customer facing (or is it?)

Upstream has a similar guideline:

The English-language documentation uses U.S. English spelling and grammar.

https://kubernetes.io/docs/contribute/style/style-guide/

I don't know whether that applies to enhancements though, and anyway, we're not upstream.

Until I'm forced to write and speak in american english, I'll stick to british and keep pronouncing quay in the british way as well ;)

For what it's worth, I think you have parts of the US on your side with that one (I think the British pronunciation is common in the northeastern states).

[deads2k]

if we require everything to be set ahead of time, we could add this protection?

[JoelSpeed]

We don't have required fields within the provider specs, especially with optional features such as this so I'm not sure if we could reasonably enforce things to be set ahead of time. I can imagine customers frequently tweak their machineset definitions, delete all the current machines and let MAPI create them new ones based on the new configuration

[deads2k]

We create this resource via the API, right? Why can't we enforce it during creation?

[JoelSpeed]

We could if we had webhooks, but we don't today and I'd rather not expand the scope of this enhancement to introduce them, it doesn't seem justified for a single validation like this.

The MachineAutoscaler is a separate CR to the MachineSet, so to check this, we need to have something lookup the MachineSet based on the object reference, then check the spec of that and report back whether it's using a spread group or not. While that's entirely possible, there's nothing to stop the end user changing the definition of their machineset during the lifecycle of the cluster, they could create the MachineAutoscaler, then configure the placement groups later

[sdodson]

Aside from the new comments it looks fine to me, api review not withstanding.

[sdodson]

I'm wondering if @deads2k was suggesting that the entire placement group policy should be a discriminating union?

[JoelSpeed]

Aside from the new comments it looks fine to me, api review not withstanding.

I've updated based on the last couple of comments and the union suggestion

[deads2k]

If I have three machinesets and each contains an AWSMachineProviderConfig and I specify a GroupName of "BestGroupEver" in each machineset, what happens?

[JoelSpeed]

That's allowed and is a valid use case. If you read through the partition placement groups section, it mentions having a partition placement group with multiple partitions and having each partition represented by the group number.

There isn't a negative effect to having multiple MachineSets with the same group name, if it already exists, it will just be used as is

[deads2k]

There isn't a negative effect to having multiple MachineSets with the same group name, if it already exists, it will just be used as is

Given three different definitions of the GroupName, which of those definition is accurate? Based on what I'm seeing here, I gather that it is racy.

Using this as both the definition PlacementGroup and the reference to which PlacementGroup to use makes the usage of this API inconsistent depending on read order. The same resources played against two different clusters can have two different results. this is a problem with the API.

As an alternative, why not have one resource that describes the PlacementGroup and simply reference those PlacementGroups from here.

[JoelSpeed]

Given three different definitions of the GroupName, which of those definition is accurate? Based on what I'm seeing here, I gather that it is racy.

I think you're right this would cause a problem for users as one of the MachineSets would come up successfully and the others, as we have it implemented now, would fail with errors

As an alternative, why not have one resource that describes the PlacementGroup and simply reference those PlacementGroups from here.

This is something we could do, though it would be a lot more effort to get done (will need to review team planning/product requirements) and does add complexity for the users. A concern that has come up before is proliferation of the number of CRDs within OpenShift, does this tiny feature warrant adding a new CRD? Is that acceptable?

I'll have a few conversations today and gather feedback, fwiw, to avoid a lot of the complexity here about preventing misconfiguration, having a new CRD is probably the best thing to do. I'm wondering if we should investigate the idea of the AWSCluster resource from Cluster API which gathers a lot of information like this, if that's a singleton within the cluster and has a list of placement group configs, it could manage those and the other "cluster level" infrastructure

[deads2k]

What if i specify the same group three times as in https://github.com/openshift/enhancements/pull/995/files#r798856729 and each one specifies a different GroupType?

[JoelSpeed]

We are planning (I will double check if this made it into the enhancement or just comments) to check the group type against the group we find on AWS. If the user specifies a cluster PG and we find a spread PG, we will return an error and tell them they need to fix the Machine configuration

[deads2k]

What happens if different machinesets specify this number differently for the same GroupName?

[Miciah]

From the above statement, "This value is only observed when creating a placement group", I would infer that the parameters applied would be those specified in the first reconciled machineset that is configured to use the group.

The more I read and come to understand how placement groups work, the more it seems to me that placement groups really should be configured as separate config objects from machines/machinesets. Defining placement groups within machinesets makes the semantics extremely confusing. As it is, both the basic usage and behaviors of placement groups as well as edge cases (such as the ones David brings up) are difficult to understand.

[JoelSpeed]

The more I read and come to understand how placement groups work, the more it seems to me that placement groups really should be configured as separate config objects from machines/machinesets. Defining placement groups within machinesets makes the semantics extremely confusing. As it is, both the basic usage and behaviors of placement groups as well as edge cases (such as the ones David brings up) are difficult to understand.

I could envisage a separate CRD for this that then gets created, thought that's a lot of work and means more controllers and more complication from a user perspective, so I think there's a balance to make here.

The upstream community in Azure already have similar concepts being created by the Machine controller so we have followed that pattern for now.

What happens if different machinesets specify this number differently for the same GroupName?

We can validate that this configuration is matching the existing placement group and error out if it isn't, this would enforce the count and type to be consistent across multiple MachineSets

[Miciah]

It isn't clear to me from this godoc what "Partition" actually does. From my reading of the implementation details, if I understand them correctly, "Partition" defines two very different behaviors depending on whether number is specified or not:

• When number is specified, "Partition" behaves like "Cluster" except it places the instances within the numberth partition.
• When number is not specified, "Partition" behaves like "Spread" except it restricts instances to racks belonging to partitions of the placement group.

Is that correct?

[JoelSpeed]

Sort of

When number is specified, "Partition" behaves like "Cluster" except it places the instances within the numberth partition.

The additional behaviour here is that if you had two cluster groups, they may overlap and instances in one group may be on the same physical hardware as the other. With partitions, each partition is distinct, so you are guaranteed that an instance in partition 1 is not on the same hardware as partition 2

When number is not specified, "Partition" behaves like "Spread" except it restricts instances to racks belonging to partitions of the placement group.

This is right, the only difference is that in spread you can only have 1 instance per hardware failure domain (the 7 partitions in each AZ), where in a partition it spreads them equally across however many partitions you choose

[Miciah]

This sentence is missing either a comma or a conjunct. I would guess the former, but I want to make sure there isn't some missing information here.

Suggested change

	If the placement group does not exist, it will be created based upon the group type and, if applicable partition count provided.
	If the placement group does not exist, it will be created based upon the group type and, if applicable, partition count provided.

Suggested change

	If the placement group does not exist, it will be created based upon the group type and, if applicable partition count provided.
	If the placement group does not exist, it will be created based upon the group type and [missing words here], if applicable partition count provided.

[JoelSpeed]

First suggestion, missing comma is what I had intended

[Miciah]

Suggested change

	// paramenter.
	// parameter.

[Miciah]

"Machine or MachineSet", right? Or do I have a fundamental misunderstanding of this API?

Suggested change

	// This parameter is only used when a Machine is being created and the named
	// This parameter is only used when a Machine or MachineSet is being created and the named

[JoelSpeed]

MachineSet is to a Machine as a ReplicaSet is to a Pod, so we don't actually create the PG until the first Machine in a MachineSet is created

[Miciah]

Note that the godoc on the AWSPlacementGroupType type definitions does not get included in the generated Swagger, so any documentation from the const definitions that you want the user to see in the generated Swagger should be repeated here for the field definition. That said, I don't know whether users really look to Swagger for API documentation, so maybe this isn't important. (Where do users turn for MAPI documentation? Do most users just read the Go definitions?)

[JoelSpeed]

We don't currently have the Machine ProviderSpecs documented very well, typically we have guides that instruct users to turn on a particular feature. I imagine there will be a new page or section for placement groups under which we will talk about the new fields and the way that the structure should be created within the MachineSet.

Our docs writer would normally grab this information from the enhancement IIUC (cc @jeana-redhat)

[Miciah]

I'm confused because you wrote the following above:

Should the user not define the type, we will return an error and place the Machine into a Failed state.

So if groupName is specified and groupType is omitted, then the Machine API definitely didn't create the placement group, right?

[JoelSpeed]

So if groupName is specified and groupType is omitted, then the Machine API definitely didn't create the placement group, right?

I believe so yes. Though, having now talked about making sure that the placement group we are using is the correct kind for the customer, I think we want to require name and type and then perform validation that the group is of the type they are expecting. I think I missed this while updating from previous feedback

[Miciah]

Above, you wrote the following:

• A partition placement group with AWS Dedicated Instances can have a maximum of two partitions.

I don't know how to reconcile the "general limitation" on line 336 with the cited limitation for "Partition". Is there a distinction between "AWS Dedicated Hosts" and "AWS Dedicated Instances"? Is the user able launch a AWS Dedicated Host and then put it in a placement group after it is launched?

[JoelSpeed]

This was copied from the AWS docs, which having checked, of course contradict themselves. I think the general limitations is "You should assume this", and then looking at partition in particular, "you can now use this but only for this type of placement group" (Dedicated hosts/instances are synonyms)

[damdo]

To thicken the plot, it looks like there are two different pages for Amazon EC2 Dedicated Hosts and Amazon EC2 Dedicated Instances
. Quite confusing unfortunately, are we really sure they are synonyms? The instanceTenancy type also has a "dedicated" and an "host" value here, could they highlight this difference already?

[JoelSpeed]

Reading the two pages, they appear to me to describe the same thing

[Miciah]

how very british

The Britishism that trips me up the most is the parsimonious spelling of "fulfil".

I have wondered about whether we had an official spelling recommendation, I know product docs have to be american english but this isn't customer facing (or is it?)

Upstream has a similar guideline:

The English-language documentation uses U.S. English spelling and grammar.

https://kubernetes.io/docs/contribute/style/style-guide/

I don't know whether that applies to enhancements though, and anyway, we're not upstream.

Until I'm forced to write and speak in american english, I'll stick to british and keep pronouncing quay in the british way as well ;)

For what it's worth, I think you have parts of the US on your side with that one (I think the British pronunciation is common in the northeastern states).

[Miciah]

It may not be supported, but is it possible for users to configure this on the control-plane Machines, or will we block this somehow?

[JoelSpeed]

Not supported for the moment, this is blocked purely on the fact that we aren't going to implement this in the installer. I've added some notes about control plane machines to the future implementation and drawbacks sections

[Miciah]

Does this mean it is not possible to change the placement group of an instance after it has been launched?

[JoelSpeed]

You can, but not via Machine API. To change the PG, you have to shut down the instance on AWS and then move it, and then restart it. AWS document that this can't be done but I've been informed by AWS engineers that it can be done

[JoelSpeed]

@Miciah @deads2k Thanks for the reviews, I've updated the enhancement to clarify some of the points you made and responded in the comments for each

The important change is we will now require group name and group type and validate these so that if users do share the config across multiple groups, they definitely get the correct type of group. Will add the group count for partition into that validation as well

[staebler]

We should call out in this comment that only Placement Groups created by the cluster will be deleted.

[staebler]

If the user adds an instance to a placement group created by the cluster, then the destroyer will never complete as it will continually fail to delete the placement group. That does not sound like a terrible problem to me: But I did want to call it out. It is similar issue to not being able to delete a VPC to which the user has added an instance.

[JoelSpeed]

I've added a note under this to call that out, I guess the only thing we can do is try to make sure a useful error is returned in this case?

[staebler]

ec2:DeletePlacementGroup, too, right?

If so, then this permission also needs to be added to the permissions checked by the installer since it will be needed by the destroyer.

[JoelSpeed]

Good catch, I hadn't done any testing of the deletion (or implemented it) when this was written so it slipped through. It is needed though so will add a note.

Do you happen to have a link to roughly where the list of permissions the installer checks is sourced from? Can raise a PR

[staebler]

https://github.com/openshift/installer/blob/e9bd965ebc73bf2fd8c2b052ecf3918437727c0a/pkg/asset/installconfig/aws/permissions.go#L175-L192

[JoelSpeed]

PR openshift/installer#5655

[staebler]

I would suspect so.

[JoelSpeed]

I've already created a PR for this so this isn't actually an open question anymore, will remove

[damdo]

Suggested change

	new object reference within the `Placement` section of the AWS Machine Provider Config which will allow users
	object reference within the `Placement` section of the AWS Machine Provider Config which will allow users

[damdo]

Suggested change

	// provided by attached IAM role where the actuator is running.
	// +optional
	CredentialsSecret *corev1.LocalObjectReference `json:"credentialsSecret,omitempty"`
	// provided by attached IAM role where the actuator is running.
	// +optional
	CredentialsSecret *corev1.LocalObjectReference `json:"credentialsSecret,omitempty"`

[damdo]

This is the Tenancy field under Placement right?
https://github.com/openshift/api/blob/b632c5fc10c0ea86cb18577db9388109a43d465f/machine/v1beta1/types_awsprovider.go#L189
It would be nice if we could make it more obvious it is that one. Maybe we could specifying its full path?

[damdo]

My understanding (but correct me if I'm wrong here) after reading the AWS docs is that the partition number is a concept only for Partition placement groups.

With that in mind, what happens if the user specifies a PartitionNumber but then references an AWSPlacementGroup of an AWSPlacementGroupType (which doesn't have the concept of partition number)? What is the behaviour we intend to pursue? We could maybe add a note on that in the spec comment above.

[JoelSpeed]

We should return an error in that case to the user, they probably didn't mean to set the partition number in that case I would guess

[damdo]

👍

[damdo]

So my understanding is that once a partition placement group is created (and it's initial count specified), then machines can be created within the various logical partitions (correct me if I'm wrong here).

A couple of follow up questions to this:

• the count of the desired logical partitions must be specified at creation time, instead where these partitions are within a region is not pin-pointed by the creation step. How does this work? Is a partition automatically assigned to an AZ depending on where the first instance that we put into the partition lives? Or does a single logical partition allow, for example, 2 instances that live on two different AZs to be in the same logical partition? And if the latter is true what how does this count towards the partitions per Availability Zone cap?
• do you envision us allowing different MachineSets biding to the same logical partition number? If yes, what if the two MachineSets instances are on different AZs? (this then ties back to the previous question).

[JoelSpeed]

So you can have 7 partitions maximum, but that is 7 partitions per availability zone in actuality.

So there is a partition numbered 1 in AZ A and in AZ B, and you can put the machines in to partition 1 on each of these, but because they are separate AZs, they are separate failure domains already. It's best to just think of placement groups as a way to organise machines within a single AZ

Machines in AZ A P1 and P2 are protected from hardware failures by being in different redundancy zones within the AZ, AZ A P1 and AZ B P1 are protected because they are in separate AZs (data centers)

[damdo]

Ok I see. Makes sense, thank you.

[damdo]

By instance here you mean the "physical" instance of the AWSPlacementGroup, not an AWS EC2 instance within the group, correct?

[JoelSpeed]

I mean the placement group on AWS, will clarify

[damdo]

To thicken the plot, it looks like there are two different pages for Amazon EC2 Dedicated Hosts and Amazon EC2 Dedicated Instances
. Quite confusing unfortunately, are we really sure they are synonyms? The instanceTenancy type also has a "dedicated" and an "host" value here, could they highlight this difference already?

[damdo]

Thanks for the answers and for including the suggestions @JoelSpeed
/lgtm

[Fedosin]

/lgtm

[JoelSpeed]

@sdodson @deads2k I believe everything here has been resolved, I've updated the API to match what was approved in the API repo, if you are happy, I'll squash the commits and we can merge this and get on with the implementation

[JoelSpeed]

I've squashed the commits here so that we can get this merged

[sdodson]

/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Fedosin, sdodson

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 [sdodson]

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

[openshift-ci]

@JoelSpeed: all tests passed!

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.