New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ support accelerated networking #645
✨ support accelerated networking #645
Conversation
Hi @mboersma. 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/test-infra repository. |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple SDK related nits.
@mboersma for the verify failure: since you added a field to the azuremachine spec, you have to generate the CRDs yaml files and include them in the commit |
Actually the generated changes to I'm not able to reproduce or fix the current CI failure. When I run |
@mboersma try deleting |
@devigned I tried that, but no diff. 😕 |
28918f6
to
c6ddf8c
Compare
I have had a similar problem before and what I did was removed the |
c6ddf8c
to
7ea52b6
Compare
Thanks @nader-ziada. I did something similar and it pointed to a missing comment in generated code that has been added. I think we are back on track now. |
/test pull-cluster-api-provider-azure-test |
@mboersma is this ready for another review or should I wait? I think you mentioned having to add UTs for VMSS earlier in office hours |
@CecileRobertMichon there aren't any cases in vmss_test.go that test accelerated networking toggled on and off, specifically. Otherwise I think it's ready, but let me know if you see other test gaps or issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm pending vmss test cases
e6a88ff
to
98fa443
Compare
9a569d9
to
b4e6772
Compare
b4e6772
to
ffad0ad
Compare
@CecileRobertMichon I added a vmss_test.go spec for accelerated networking on and off, then squashed and rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
Thanks @mboersma !
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon, mboersma 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 |
What this PR does / why we need it:
Creates an
acceleratedNetworking
boolean inAzureMachineTemplate
and inAzureMachinePool
that enables Azure accelerated networking on the machine's network interface or on the VMSS, respectively.If
acceleratedNetworking
is omitted from the provisioning template, it is enabled or disabled after an API call is made to determine if the requested SKU supports it.Fixes #307
Special notes for your reviewer:
This could use more specific unit test coverage. I'm working on that, so maybe consider this a WIP.
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: