-
Notifications
You must be signed in to change notification settings - Fork 791
Update nf-azure: Add Azure Batch node communication mode option #7337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -577,6 +577,10 @@ Enable the `startTask` to run with elevated access (default`false`). | |||||||
|
|
||||||||
| The `startTask` that is executed as the node joins the Azure Batch node pool. | ||||||||
|
|
||||||||
| ##### `azure.batch.pools.<name>.targetCommunicationMode` | ||||||||
|
|
||||||||
| The node communication mode used by the pool. Can be either `classic` or `simplified`. When not set, the Azure Batch service selects the mode. In `simplified` mode the Batch service initiates communication with the compute nodes, reducing the outbound network access required by the nodes. | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
|
|
||||||||
| ##### `azure.batch.pools.<name>.virtualNetwork` | ||||||||
|
|
||||||||
| <AddedInVersion version="23.04" /> | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| package nextflow.cloud.azure.config | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import com.azure.compute.batch.models.BatchNodeCommunicationMode | ||||||||||||||||||||||||||||||||||
| import com.azure.compute.batch.models.ImageVerificationType | ||||||||||||||||||||||||||||||||||
| import com.azure.compute.batch.models.OSType | ||||||||||||||||||||||||||||||||||
| import com.google.common.hash.Hasher | ||||||||||||||||||||||||||||||||||
|
|
@@ -144,6 +145,12 @@ class AzPoolOpts implements CacheFunnel, ConfigScope { | |||||||||||||||||||||||||||||||||
| """) | ||||||||||||||||||||||||||||||||||
| final String vmType | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @ConfigOption | ||||||||||||||||||||||||||||||||||
| @Description(""" | ||||||||||||||||||||||||||||||||||
| The node communication mode used by the pool. Can be either `classic` or `simplified`. When not set, the Azure Batch service selects the mode. In `simplified` mode the Batch service initiates communication with the compute nodes, reducing the outbound network access required by the nodes. | ||||||||||||||||||||||||||||||||||
| """) | ||||||||||||||||||||||||||||||||||
| final BatchNodeCommunicationMode targetCommunicationMode | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| OSType osType = DEFAULT_OS_TYPE | ||||||||||||||||||||||||||||||||||
| ImageVerificationType verification = ImageVerificationType.VERIFIED | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -175,6 +182,18 @@ class AzPoolOpts implements CacheFunnel, ConfigScope { | |||||||||||||||||||||||||||||||||
| this.password = opts.password | ||||||||||||||||||||||||||||||||||
| this.virtualNetwork = opts.virtualNetwork | ||||||||||||||||||||||||||||||||||
| this.lowPriority = opts.lowPriority as boolean | ||||||||||||||||||||||||||||||||||
| this.targetCommunicationMode = parseCommunicationMode(opts.targetCommunicationMode) | ||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Nextflow team have the final say on style, but it may be more disciplined to only use parseCommunicationMode when targetCommunicationMode is populated:
Suggested change
Style choice, it's been a while since I've contributed so I want one of the core developers to look at it! |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| protected static BatchNodeCommunicationMode parseCommunicationMode(value) { | ||||||||||||||||||||||||||||||||||
| if( value == null ) | ||||||||||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||||||||||
| if( value instanceof BatchNodeCommunicationMode ) | ||||||||||||||||||||||||||||||||||
| return value | ||||||||||||||||||||||||||||||||||
| final str = value.toString().toLowerCase() | ||||||||||||||||||||||||||||||||||
| if( str == 'simplified' ) return BatchNodeCommunicationMode.SIMPLIFIED | ||||||||||||||||||||||||||||||||||
| if( str == 'classic' ) return BatchNodeCommunicationMode.CLASSIC | ||||||||||||||||||||||||||||||||||
| throw new IllegalArgumentException("Invalid azure.batch.pools.<name>.targetCommunicationMode value: '$value' - expected 'simplified' or 'classic'") | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+188
to
+196
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could include an explicit "default" here: I've also removed the using BatchNodeCommunication mode because I think it makes the method confusing.
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm about 99% certain that that's the case. However, I can't find any clear documentation on that... However, given that this option was never explicitly set up until now, we can safely conclude that leaving it empty means it takes the 'Default' option. |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||
|
|
@@ -195,6 +214,7 @@ class AzPoolOpts implements CacheFunnel, ConfigScope { | |||||||||||||||||||||||||||||||||
| hasher.putUnencodedChars(schedulePolicy ?: '') | ||||||||||||||||||||||||||||||||||
| hasher.putUnencodedChars(virtualNetwork ?: '') | ||||||||||||||||||||||||||||||||||
| hasher.putBoolean(lowPriority) | ||||||||||||||||||||||||||||||||||
| hasher.putUnencodedChars(targetCommunicationMode?.toString() ?: '') | ||||||||||||||||||||||||||||||||||
| hasher.putUnencodedChars(startTask.script ?: '') | ||||||||||||||||||||||||||||||||||
| hasher.putBoolean(startTask.privileged) | ||||||||||||||||||||||||||||||||||
| return hasher | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
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.