[Bug Report]: Microsoft.Cache/redis Premium deploys all replicas in Zone 1

[DanielLarsenNZ]

Describe the bug

I think this is an oversight:

zones: skuName == 'Premium' ? pickZones('Microsoft.Cache', 'redis', location, 1) : null

https://github.com/Azure/ResourceModules/blob/main/modules/Microsoft.Cache/redis/deploy.bicep#L206

When the target region has AZ's enabled, and Premium cache is selected, all replicas are deployed to Zone 1. A user should be able to choose which zones to use when opting for zone redundancy. Or they should be able to opt out of ZR and let the service decide where the replicas are deployed (I suspect replicas are deployed in an availability set when zones is not set).

Line 206 could be changed to:

zones: skuName == 'Premium' ? pickZones('Microsoft.Cache', 'redis', location, 3) : null

However, this will error if there are less than 3 zones in the Region (unlikely, but feasible). Also, some users may want all of their replicas in a particular zone or zones. I suggest that zones is converted to a param, i.e.

param zones array = []

// ...

zones: zones

When zones is set, ZR is enabled and the specified zones are used. When zones is not set (defaults to []) ZR is not enabled.

To reproduce

This bicep:

module redisCacheModule 'modules/Microsoft.Cache/redis/deploy.bicep' = {
  name: redis
  params: {
    name: redis
    location: 'westus2'
    skuName: 'Premium'
    capacity: 1
    replicasPerMaster: 2
    replicasPerPrimary: 2
    tags: tags
  }
}

Results in this deployment:

"instances": [
            {
                "sslPort": 15000,
                "zone": "1",
                "shardId": 0,
                "isMaster": false
            },
            {
                "sslPort": 15001,
                "zone": "1",
                "shardId": 0,
                "isMaster": true
            },
            {
                "sslPort": 15002,
                "zone": "1",
                "shardId": 0,
                "isMaster": false
            }
        ]

"zones": [
        "1"
    ]

Note that all replicas are deployed to zone 1.

Code snippet

No response

Relevant log output

No response

[DanielLarsenNZ]

@itpropro, @MrMCake, if you agree with my suggestion to add a zones param, I am happy to create a PR.

[itpropro]

Hey @DanielLarsenNZ, we should aim to provide good defaults to the user, which is why I would keep the automatic picking with the addition of using 3 instead of 1 for the number of zones (pickZones should handle, if there are only two zones) and optionally provide a zones array.
Could look something like this:

@description('Optional. A list of availability zones denoting where the resource needs to come from.')
param zones array = []
...
var availabilityZones = skuName == 'Premium' ? zones ? zones : pickZones('Microsoft.Cache', 'redis', location, 3) : null
...
zones: availabilityZones

[DanielLarsenNZ]

Hey @itpropro, yes I considered that too, but what if user does not want a zonal deployment? I checked with PG; if zones is null or empty array then service will choose where replicas are deployed. How about a new param zoneRedundant bool similar to Microsoft.Web/serverfarms? Something like:

@description('Optional. When true, replicas will be provisioned in availability zones specified in zones param. When false, service will choose where replicas are deployed.')
param zoneRedundant bool = false
@description('Optional. If zoneRedundant param is true, replicas will be provisioned in the availability zones specified here.')
param zones array = []
...
var availabilityZones = skuName == 'Premium' ? zoneRedundant ? zones ? zones : pickZones('Microsoft.Cache', 'redis', location, 3) : []
...
zones: availabilityZones

[itpropro]

That looks fine for me @DanielLarsenNZ, the only thing I would change is to set zones deployment to true as default to provide production ready configurations if nothing is provided. We should find a way to standardize this as a building block like with locks and custom roles. The only thing we would have to adjust per service is if zones are required when using a specific SKUs or if it is still optional with all SKUs.

[AlexanderSehr]

The implementation

@description('Optional. When true, replicas will be provisioned in availability zones specified in zones param. When false, service will choose where replicas are deployed.')
param zoneRedundant bool = true

@description('Optional. If zoneRedundant param is true, replicas will be provisioned in the availability zones specified here.')
param zones array = []

var availabilityZones = skuName == 'Premium' ? zoneRedundant ? zones ? zones : pickZones('Microsoft.Cache', 'redis', location, 3) : []

(..)
properties: {
  zones: availabilityZones
  (..)
}

seems to be a good compromise. Should be implemented once #1819 is merged to avoid conflicts. Thanks you very much for the suggestions.

[ChrisSidebotham]

@AlexanderSehr - Do we have this staged ready to PR?