Activate Accelerated Networking per default #3449
Conversation
ghost
assigned 
|
@lachie83 @jackfrancis @CecileRobertMichon would love to have a first review on this to see if I am on the right track. I tested using VMSS and VMAS and it works with simple and multiple agentpool profile. Thanks ! |
julienstroheker
changed the title
|
cc @sozercan can you please review |
pkg/api/types.go
Outdated
| } | ||
| return true | ||
| } | ||
| return *a.AcceleratedNetworkingEnabled |
There was a problem hiding this comment.
If the user manually sets a.AcceleratedNetworkingEnabled to true, I think we should also check if it is supported with the VM SKU, either by checking here and overriding to false if it's not, or by adding validation for the apimodel (https://github.com/Azure/acs-engine/blob/master/pkg/api/vlabs/validate.go) to make sure that if a.AcceleratedNetworkingEnabled is true, then the VM SKU supports accelerated networking and returning an error if it doesn't. I personally think the latter is better UX so we don't silently override.
pkg/api/types.go
Outdated
| @@ -451,6 +451,45 @@ type AgentPoolProfile struct { | |||
| ImageRef *ImageReference `json:"imageReference,omitempty"` | |||
| } | |||
|
|
|||
| // IsEnabledAcc returns if the AcceleratedNetworkingEnabled is explicitly enabled, or the user-provided default if non explicitly enabled | |||
| func (a *AgentPoolProfile) IsEnabledAcc() bool { | |||
There was a problem hiding this comment.
Not sure if we need an extra field/function IsEnabledAcc() : instead, we could set a default for AcceleratedNetworkingEnabled in https://github.com/Azure/acs-engine/blob/master/pkg/acsengine/defaults.go#L791 (using the same logic as below)
| @@ -32,7 +32,11 @@ | |||
| "location": "[variables('location')]", | |||
| "name": "[concat(variables('{{.Name}}VMNamePrefix'), 'nic-', copyIndex(variables('{{.Name}}Offset')))]", | |||
| "properties": { | |||
| "enableAcceleratedNetworking" : "{{.AcceleratedNetworkingEnabled}}", | |||
There was a problem hiding this comment.
I think this code could stay unchanged, see my comment below
| @@ -53,7 +53,11 @@ | |||
| "name": "[variables('{{.Name}}VMNamePrefix')]", | |||
| "properties": { | |||
| "primary": true, | |||
| "enableAcceleratedNetworking" : "{{.AcceleratedNetworkingEnabled}}", | |||
| {{if IsEnabledAcc .}} | |||
pkg/acsengine/template_generator.go
Outdated
| @@ -645,6 +645,12 @@ func (t *TemplateGenerator) getTemplateFuncMap(cs *api.ContainerService) templat | |||
| } | |||
| return false | |||
| }, | |||
| "IsEnabledAcc": func(profile *api.AgentPoolProfile) bool { | |||
There was a problem hiding this comment.
this would also be removed
Codecov Report
@@ Coverage Diff @@
## master #3449 +/- ##
==========================================
- Coverage 55.94% 55.94% -0.01%
==========================================
Files 105 105
Lines 15888 15917 +29
==========================================
+ Hits 8889 8905 +16
- Misses 6253 6262 +9
- Partials 746 750 +4 |
pkg/api/vlabs/validate.go
Outdated
| @@ -347,6 +347,11 @@ func (a *Properties) validateAgentPoolProfiles() error { | |||
| if e := validatePoolOSType(agentPoolProfile.OSType); e != nil { | |||
| return e | |||
| } | |||
| if agentPoolProfile.AcceleratedNetworkingEnabled != nil { | |||
There was a problem hiding this comment.
Here you could use helpers.IsTrueBoolPointer(agentPoolProfile.AcceleratedNetworkingEnabled) instead of checking if it's nil and check helpers.AcceleratedNetworkingSupported(VMSize)` in validatePoolAcceleratedNetworking, no need to check if it is true or if AcceleratedNetworkingSupported == AcceleratedNetworking.
julienstroheker
changed the title
| @@ -164,3 +164,58 @@ EPDesL0rH+3s1CKpgkhYdbJ675GFoGoq+X21QaqsdvoXmmuJF9qq9Tq+JaWloUNq | |||
| t.Fatalf("Public Key did not match expected format/value") | |||
| } | |||
| } | |||
|
|
|||
| func TestAcceleratedNetworkingSupported(t *testing.T) { | |||
julienstroheker
force-pushed
the
feat/acc-net-default
branch
from
afb8b0e to
3a13d1b
Compare
What this PR does / why we need it:
Activate the Accelerated Networking feature per default is the SKU is supported
Which issue this PR fixes *
fixes #3236
Special notes for your reviewer: