CFE-1063: Add PlacementGroupPartition in AWSMachineProviderConfig #1897
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @chiragkyal! Some important instructions when contributing to openshift/api: |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
size/S
|
/test all |
chiragkyal
changed the title
chiragkyal
force-pushed
the
partition-number
branch
from
07e6810 to
26c6f3c
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
chiragkyal
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references CFE-1063 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 story to target the "4.17.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. |
|
/assign @JoelSpeed |
|
@chiragkyal: This pull request references CFE-1063 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 story to target the "4.17.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. |
chiragkyal
force-pushed
the
partition-number
branch
from
26c6f3c to
1eae6f1
Compare
machine/v1beta1/types_awsprovider.go
Outdated
| // PlacementGroupPartition is the partition number within the placement group in which to launch the instance. | ||
| // This value is only valid if the placement group, referred in `PlacementGroupName`, was created with | ||
| // strategy set to partition. |
There was a problem hiding this comment.
Does this field exist in the upstream CAPA?
Godoc comments should start with json tag name not Go field name, so placementGroupPartition would be better.
Godoc should also explain the validations in place, so in this case, explain in the text that the value should be an integer value between 1 and 7.
There was a problem hiding this comment.
Yes, we have added the field in the upstream CAPA as well. Please see: kubernetes-sigs/cluster-api-provider-aws#4883
There was a problem hiding this comment.
Godoc comments should start with json tag name not Go field name, so
placementGroupPartitionwould be better.
Okay. However, I observed that the existing Godoc comments for fields like PlacementGroupName, MetadataServiceOptions, etc., are also using the Go field name. The new field should be consistent with them as well.
There was a problem hiding this comment.
Prefer not to repeat mistakes of the past, that just adds to tech debt
There was a problem hiding this comment.
Okay, thanks. Updated.
machine/v1beta1/types_awsprovider.go
Outdated
| // +kubebuilder:validation:Minimum:=1 | ||
| // +kubebuilder:validation:Maximum:=7 | ||
| // +optional | ||
| PlacementGroupPartition int64 `json:"placementGroupPartition,omitempty"` |
There was a problem hiding this comment.
Why int64? Change to int32, we only need to support small numbers here
There was a problem hiding this comment.
The aws-sdk uses pointer to int64 to specify the partition number.
https://pkg.go.dev/github.com/aws/aws-sdk-go@v1.50.0/service/ec2#Placement.PartitionNumber
xref:
PartitionNumber *int64 `locationName:"partitionNumber" type:"integer"`In order to match with it and CAPA PR, I chose int64 here as well.
There was a problem hiding this comment.
In general we prefer to build APIs that make sense for us, rather than being restricted by cloud provider APIs. But given the context of this and the fact that we are going to convert to CAPI in the near future, there's value in this being the same.
Do you think there's any likelihood of being able to change the upstream to int32? Since that would make more sense for the Kube API
There was a problem hiding this comment.
Updating it to int32 would be better, making more sense to our API. We can anyway convert it to int64 in the provider code to match with aws-sdk.
For upstream, I'll try to open a PR to fix it.
machine/v1beta1/types_awsprovider.go
Outdated
| // +kubebuilder:validation:Minimum:=1 | ||
| // +kubebuilder:validation:Maximum:=7 | ||
| // +optional | ||
| PlacementGroupPartition int64 `json:"placementGroupPartition,omitempty"` |
There was a problem hiding this comment.
Do you think we should add some CEL Validation?
!(has(self.placementGroupPartition) && !(has(self.placementGroupName) && size(self.placementGroupName)>0))message="placementGroupPartition is invalid, because placementGroupName is not set"
There was a problem hiding this comment.
You can't use CEL in this API because it is a raw extension, but, for upstream you could try to get that through yes
Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
partition-number
branch
from
86fba4f to
14a7205
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chiragkyal, 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@chiragkyal: 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-sigs/prow repository. I understand the commands that are listed here. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-cluster-config-api-container-v4.17.0-202406131541.p0.g76a71da.assembly.stream.el9 for distgit ose-cluster-config-api. |
| // +kubebuilder:validation:Minimum:=1 | ||
| // +kubebuilder:validation:Maximum:=7 |
There was a problem hiding this comment.
@JoelSpeed I was doing some dev testing, and it turns out that these kubebuilder validations are not working as expected while creating a MachineSet object. Do we need them to be added inside the webhook?
There was a problem hiding this comment.
Yes, kubebuilder validations won't ever work on this particular API, please add them to the webhook validations
There was a problem hiding this comment.
Gotcha! Then these kubebuilder validations can be removed from here I think. Created #1942 to drop them
There was a problem hiding this comment.
openshift/machine-api-operator#1265 to add them into webhook validations
Add a new field
PlacementGroupPartitioninAWSMachineProviderConfigto allow users specify the Partition Number of placement group for AWSMachineRelated to https://issues.redhat.com/browse/CFE-1063