✨ Implement support for EFA interface type #5275
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @RadekManak! |
|
Hi @RadekManak. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -153,6 +163,11 @@ type AWSMachineSpec struct { | |||
| // +kubebuilder:validation:MaxItems=2 | |||
| NetworkInterfaces []string `json:"networkInterfaces,omitempty"` | |||
|
|
|||
| // NetworkInterfaceType is the interface type of the primary network Interface. | |||
There was a problem hiding this comment.
Maybe mention here that the default is interface and that "" and interface are equivalent.
There was a problem hiding this comment.
Maybe we can have explicit default here? E.g. // +kubebuilder:default=interface
There was a problem hiding this comment.
Is it possible in the future that we might want to change the default? For example, what if Amazon deprecates the current interface type and makes EFA the choice, we would want to move folks across to EFA. If we add an explicit default here, then they would have to manually update their configs to move across should things ever change
There was a problem hiding this comment.
Added note that when field is not set, aws defaults the value.
|
Overall looks good, just one small comment. /assign @damdo |
| // NetworkInterfaceType is the type of network interface. | ||
| type NetworkInterfaceType string | ||
|
|
||
| const ( |
There was a problem hiding this comment.
Should we follow Kube conventions and use PascalCase rather than these lowercase version, so Interface and EFA
There was a problem hiding this comment.
I used the aws API values. The API is case insensitive so I think it is safe to use PascalCase.
There was a problem hiding this comment.
In the future, we may want to add other interface types.
Is the correct approach to use PascalCase and convert to the API options?
If for e.g. we want to add EFA only interface that is "efa-only" do we use "EFAOnly" and have dict to convert them? Or is parity with API preferred here?
api/v1beta2/awsmachine_types.go
Outdated
| // NetworkInterfaceType is the interface type of the primary network Interface. | ||
| // +kubebuilder:validation:Enum=interface;efa | ||
| // +optional | ||
| NetworkInterfaceType *NetworkInterfaceType `json:"networkInterfaceType,omitempty"` |
There was a problem hiding this comment.
Does this need to be a pointer? There's no difference between "" and a nil value right?
There was a problem hiding this comment.
I used the pointer to avoid "" value being passed through to the AWS API.
Changed to non-pointer and updated the code to not set empty string on the AWS API call.
There was a problem hiding this comment.
I might be missing something, but isn't "" passed to the AWS API due to Go's defaulting to zero values?
There was a problem hiding this comment.
The AWS API call still uses *string. I added a condition to keep the value nil when "" value is on the spec. https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/5275/files#diff-4d2ea189a70ea097ffeeb50f78cc676a224386d4fe17b1bec9c83a4731d9c379R586-R589
RadekManak
force-pushed
the
efa-support
branch
4 times, most recently
from
f8138bc to
51631e7
Compare
RadekManak
force-pushed
the
efa-support
branch
from
51631e7 to
b0922f7
Compare
|
/test pull-cluster-api-provider-aws-test |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds a new enum field to AWSMachineSpec called NetworkInterfaceType that configures the type of the first interface (also called primary interface) on the created instance.
The two values are:
NetworkInterfaceTypeENI: The network interface will be ENI. Currently, AWS defaults to this type when not specified.
NetworkInterfaceTypeEFAWithENAInterface: This option sets up both ENA (for IP networking) and EFA interface for HPC connections.
Fixes #3035
Special notes for your reviewer:
Checklist:
Release note: