Update nf-azure: Add Azure Compute Gallery image and verification options#7338
Update nf-azure: Add Azure Compute Gallery image and verification options#7338LennyBEL wants to merge 3 commits into
Conversation
Add support for provisioning Azure Batch pool nodes from a custom VM
image in an Azure Compute Gallery via the new
'azure.batch.pools.<name>.virtualMachineImageId' option, and add the
'azure.batch.pools.<name>.verification' option to choose the image
verification type ('verified', 'unverified' or 'any').
Signed-off-by: Lenny Van de Winkel <vdwlenny@outlook.be>
✅ Deploy Preview for nextflow-docs canceled.
|
Signed-off-by: Lenny <vdwlenny@outlook.be>
adamrtalbot
left a comment
There was a problem hiding this comment.
This would also help when Azure deprecates a Batch image without a replacement image lined up. It would improve the supply chain security of our Azure users. So worth it overall.
|
|
||
| @ConfigOption | ||
| @Description(""" | ||
| The image verification type to match when resolving the VM image from the Batch supported-images list. Can be `verified`, `unverified`, or `any` (default: `verified`). Ignored when `virtualMachineImageId` is set. |
There was a problem hiding this comment.
I feel like verified should be included in unverified? It seems weird to reject verified images when you're allowing unverified images 🤔 this would remove the need for "any".
There was a problem hiding this comment.
In fact, unverified could be a boolean:
unverified = false // Only verified images
unverified = true // allow unverified images, here be monsters| **Image verification** | ||
|
|
||
| By default, Nextflow only selects images that Azure Batch has formally verified. Set `verification` to `unverified` to also allow images that Azure Batch lists but has not verified, or to `any` to accept both. This setting is ignored when `virtualMachineImageId` is set. | ||
|
|
||
| ```groovy | ||
| azure { | ||
| batch { | ||
| pools { | ||
| <pool-name> { | ||
| verification = 'unverified' // 'verified' (default), 'unverified' or 'any' | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
This might be bordering on too much information for this doc and should go to the advanced config options.
| @@ -175,6 +188,20 @@ | |||
| this.password = opts.password | |||
| this.virtualNetwork = opts.virtualNetwork | |||
| this.lowPriority = opts.lowPriority as boolean | |||
| this.virtualMachineImageId = opts.virtualMachineImageId ?: null | |||
| this.verification = parseVerification(opts.verification) | |||
| } | |||
There was a problem hiding this comment.
A bit of code tidying, let's put them with their logical groupings.
| } | |
| AzPoolOpts(Map opts) { | |
| this.runAs = opts.runAs ?: '' | |
| this.privileged = opts.privileged ?: false | |
| this.publisher = opts.publisher ?: DEFAULT_PUBLISHER | |
| this.offer = opts.offer ?: DEFAULT_OFFER | |
| this.virtualMachineImageId = opts.virtualMachineImageId ?: null | |
| this.verification = parseVerification(opts.verification) | |
| this.sku = opts.sku ?: DEFAULT_SKU | |
| this.vmType = opts.vmType ?: DEFAULT_VM_TYPE | |
| this.fileShareRootPath = opts.fileShareRootPath ?: buildFileShareRootPath() | |
| this.vmCount = opts.vmCount as Integer ?: 1 | |
| this.autoScale = opts.autoScale as boolean | |
| this.scaleFormula = opts.scaleFormula | |
| this.schedulePolicy = opts.schedulePolicy | |
| this.scaleInterval = opts.scaleInterval as Duration ?: DEFAULT_SCALE_INTERVAL | |
| this.maxVmCount = opts.maxVmCount as Integer ?: vmCount *3 | |
| this.startTask = new AzStartTaskOpts( opts.startTask ? opts.startTask as Map : Map.of() ) | |
| this.registry = opts.registry | |
| this.userName = opts.userName | |
| this.password = opts.password | |
| this.virtualNetwork = opts.virtualNetwork | |
| this.lowPriority = opts.lowPriority as boolean | |
| } |
| this.virtualMachineImageId = opts.virtualMachineImageId ?: null | ||
| this.verification = parseVerification(opts.verification) |
There was a problem hiding this comment.
Yeah, I think I like unverified = true/false more:
| this.virtualMachineImageId = opts.virtualMachineImageId ?: null | |
| this.verification = parseVerification(opts.verification) | |
| this.virtualMachineImageId = opts.virtualMachineImageId ?: null | |
| this.unverified = opts.verification as Boolean |
| this.privileged = opts.privileged ?: false | ||
| this.publisher = opts.publisher ?: DEFAULT_PUBLISHER | ||
| this.offer = opts.offer ?: DEFAULT_OFFER | ||
| this.sku = opts.sku ?: DEFAULT_SKU |
There was a problem hiding this comment.
There are enough fields here that it may require it's own config scope.
| if( it.osType != opts.osType ) | ||
| continue | ||
| if( it.verificationType != opts.verification ) | ||
| if( opts.verification != null && it.verificationType != opts.verification ) |
There was a problem hiding this comment.
If unverified was just a boolean, we might use falsey here to catch null and false together:
Or something, not sure, just spitballing 😆
| if( opts.verification != null && it.verificationType != opts.verification ) | |
| if( opts.unverified && it.verificationType != opts.verification ) |
There was a problem hiding this comment.
(Claude review here, but raised some good points)
Focused, sensible change — the verification == null = "any" handling in getImage, the gallery/marketplace branch in poolVmConfig, and the verified default all look correct. A few points below, most-important first.
Cache-key note (whole-PR): the pool funnel now hashes verification, which was a field but was previously not hashed. On upgrade every existing auto-pool id changes (hence the test hash change 42f3635f…→7483c5b1…), so existing pools won't be reused and will idle out. Worth calling out in the description — "Expected Impact: None" isn't quite accurate for users relying on pool reuse across the upgrade.
| } | ||
|
|
||
| protected ImageReference customImageReference(AzPoolOpts opts) { | ||
| if( !opts.sku ) |
There was a problem hiding this comment.
Medium — this guard is unreachable. In the AzPoolOpts constructor sku is opts.sku ?: DEFAULT_SKU ("batch.node.ubuntu 24.04"), so opts.sku is never null/empty and if( !opts.sku ) never fires.
The docs promise that for a gallery image "sku must be set to the Batch node agent SKU id that matches the image OS", but in practice a user who omits sku silently gets batch.node.ubuntu 24.04. If their custom image is a different OS the pool fails to provision at Azure with a much more confusing error than the intended one.
Either drop the dead guard, or detect "sku not explicitly provided" (check before the ?: DEFAULT_SKU fallback in the constructor) and enforce it for the gallery path.
| import com.azure.compute.batch.models.ContainerType | ||
| import com.azure.compute.batch.models.ElevationLevel | ||
| import com.azure.compute.batch.models.ImageReference | ||
| import com.azure.compute.batch.models.MetadataItem |
There was a problem hiding this comment.
Unused import — only BatchMetadataItem (below) is used; MetadataItem isn't referenced. Drop this line.
| hasher.putUnencodedChars(virtualNetwork ?: '') | ||
| hasher.putBoolean(lowPriority) | ||
| hasher.putUnencodedChars(virtualMachineImageId ?: '') | ||
| hasher.putUnencodedChars(verification?.toString() ?: 'any') |
There was a problem hiding this comment.
Minor: verification is hashed even when virtualMachineImageId is set, where it's documented/coded as ignored — two gallery configs differing only in a no-op verification get different pool ids. Cosmetic.
| if( str == 'unverified' ) return ImageVerificationType.UNVERIFIED | ||
| if( str == 'any' ) return null | ||
| throw new IllegalArgumentException("Invalid azure.batch.pools.<name>.verification value: '$value' - expected 'verified', 'unverified' or 'any'") | ||
| } |
There was a problem hiding this comment.
Optional: null here encodes a third state ("any") on an enum-typed field — a one-line comment would help future readers, since the type alone doesn't hint that null is meaningful.
Split from #7321
Overview
Azure Compute Gallery
Last year we had a major outage when an update to the Microsoft Marketplace Ubuntu 22.04 HPC image flipped its verification state to
unverified. Because Nextflow only targetsverifiedimages, new pools could no longer be created. Our pipeline tooling did not support Ubuntu 24.04 yet, so we had to scramble to restore service.In order to avoid such an event again, Microsoft suggested that we switch to Azure Compute Gallery images. Within Azure Compute Gallery, we have full control over the images. However, Nextflow did not support the usage of them yet.
A new option
virtualMachineImageIdhas been introduced to support usage of the Azure Compute Gallery.Image verification state
For non-production and testing purposes we occasionally need to use unverified Marketplace images, but Nextflow always forced
verified.This PR adds
azure.batch.pools.<name>.verificationwith valuesverified(default),unverified, orany- whereanyignores the verification state entirely. This setting is ignored whenvirtualMachineImageIdis set.Note: Windows support was left out compared to the original PR. OsType is no longer configurable and remains fixed on Linux.
Expected Impact
None, as the default behavior does not change at all.
We have been running these changes as a custom
nf-azurefork (based on the upstream plugin) in production for a while, so we are confident they behave as intended.Tests
Unit tests added/updated in
nf-azure:AzPoolOptsTestvirtualMachineImageId+sku).verification(verified/unverified/any, case-insensitive), with an error on invalid values.funnel) sensitivity: the pool hash changes when the image config changes.AzBatchServiceTestDocs updated:
docs/azure.mdxanddocs/reference/config.mdx.