roachprod: add availability zone and network disk type support to azure #55775
Conversation
|
This change is |
| storageAccType = compute.StorageAccountTypesUltraSSDLRS | ||
| default: | ||
| err = errors.Newf("unsuported network disk type: %s", p.opts.networkDiskType) | ||
| return |
There was a problem hiding this comment.
Should we keep default == premium-disk? Like it was before when not using SSD?
| ) (network.SecurityGroup, error) { | ||
| p.mu.Lock() | ||
| group, ok := p.mu.securityGroups[name] | ||
| p.mu.Unlock() |
There was a problem hiding this comment.
Does this function get call from multiple threads? Looks like it is...
But then we release the lock, and proceed to create the group.. So, wouldn't we wind up creating multipole groups?
Should this be a defer p.mu.Unlock()?
| p.mu.Lock() | ||
| group, ok := p.mu.resourceGroups[groupName] | ||
| group, ok := p.mu.resourceGroups[name] |
There was a problem hiding this comment.
same question re concurrence.
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @miretskiy)
pkg/cmd/roachprod/vm/azure/azure.go, line 597 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
Should we keep default == premium-disk? Like it was before when not using SSD?
That is still the default behaviour when the user says --local-ssd = false (see the changes in flags.go). This is triggered if the user provides a value for --azure-network-disk-type that isn't premium-disk or ultra-disk -- in which case it makes sense to return an error to the user instead of swallowing the erroneous value and creating a premium disk underneath the hood.
pkg/cmd/roachprod/vm/azure/azure.go, line 679 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
Does this function get call from multiple threads? Looks like it is...
But then we release the lock, and proceed to create the group.. So, wouldn't we wind up creating multipole groups?
Should this be adefer p.mu.Unlock()?
I'm honestly not sure about the flow of the code here and where/how this function can be called from multiple goroutines tbh. I just followed the pattern of the existing code in getResourceGroup, which didn't wrap the API calls to the client inside the mutex. Do you want me to switch it?
User facing change: - New flag `--azure-availability-zone` lets the user supply which zone a VM should be provisioned in. - New flag `--azure-network-disk-type` lets the user choose which type of network disk to use (ultra-disk, premium-ssd) Previously, we didn't allow users to decide which availability zone a VM was provisioned in. To achieve this, this patch adds support for Network Security Groups. This was required as we had to update the IP SKU from Basic (which is now deprecated) to Standard. By default, this SKU blocks all inbound/outbound communication, which needed to be overriden by creating network security rules that allow HTTPS/SSH/HTTP connections. Network Security Groups are created under the resource groups that manage VNets for a particular location the first time a VNet is created. Release note: None
arulajmani
force-pushed
the
add-azure-zones
branch
from
d56eea0
to
e73b57e
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachprod/vm/azure/azure.go, line 597 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
That is still the default behaviour when the user says --local-ssd = false (see the changes in
flags.go). This is triggered if the user provides a value for--azure-network-disk-typethat isn't premium-disk or ultra-disk -- in which case it makes sense to return an error to the user instead of swallowing the erroneous value and creating a premium disk underneath the hood.
ack.
pkg/cmd/roachprod/vm/azure/azure.go, line 679 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I'm honestly not sure about the flow of the code here and where/how this function can be called from multiple goroutines tbh. I just followed the pattern of the existing code in
getResourceGroup, which didn't wrap the API calls to the client inside the mutex. Do you want me to switch it?
sounds good
|
TFTR! bors r=miretskiy |
|
Build succeeded: |
User facing change:
--azure-availability-zonelets the user supply which zonea VM should be provisioned in.
--azure-network-disk-typelets the user choose which typeof network disk to use (ultra-disk, premium-ssd)
Previously, we didn't allow users to decide which availability zone a
VM was provisioned in. To achieve this, this patch adds support for
Network Security Groups. This was required as we had to update the
IP SKU from Basic (which is now deprecated) to Standard. By default,
this SKU blocks all inbound/outbound communication, which needed to be
overriden by creating network security rules that allow HTTPS/SSH/HTTP
connections. Network Security Groups are created under the resource
groups that manage VNets for a particular location the first time a
VNet is created.
Release note: None