-
Notifications
You must be signed in to change notification settings - Fork 519
feat: configurable disk caching #2863
feat: configurable disk caching #2863
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2863 +/- ##
==========================================
+ Coverage 71.13% 71.22% +0.09%
==========================================
Files 147 147
Lines 25670 25738 +68
==========================================
+ Hits 18261 18333 +72
+ Misses 6272 6268 -4
Partials 1137 1137
Continue to review full report at Codecov.
|
Looks like that PR model spec would work for us, Thanks! |
@jackfrancis This should only add changes to the ARM correct? |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
39e2820
to
ba6756a
Compare
c70e730
to
9d49220
Compare
@@ -121,7 +123,8 @@ | |||
"vmSize": "Standard_D2s_v3", | |||
"osType": "Windows", | |||
"availabilityProfile": "VirtualMachineScaleSets", | |||
"vnetSubnetId": "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME" | |||
"vnetSubnetId": "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME", | |||
"osDiskCachingType": "ReadOnly" |
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.
@marosset FYI, adding this new configuration to a Windows pool in our E2E cluster configuration to confirm that this is not a Linux/Windows thing.
Btw, should we add a data disk to this pool to (1) test that functionality generally, and to also validate the data disk caching type override?
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.
Sure.
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
{ | ||
name: "Master Profile with invalid OSDiskCachingType", | ||
masterProfile: MasterProfile{ | ||
DNSPrefix: "whizbang", |
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.
I thought "qux" followed "baz." 😜
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.
There is no spoon.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, 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 |
Reason for Change:
This PR introduces configurable disk caching:
For OS disk configuration on master VMs:
For OS disk configuration on agent pool VMs:
For data disk configuration on agent pool VMs:
An example api model w/ this new configuration:
Issue Fixed:
Fixes #2249
Requirements:
Notes: