Add Azure Managed Lustre Filesystem Import Job Create#319
Add Azure Managed Lustre Filesystem Import Job Create#319
Conversation
|
Some questions:
|
|
Failed live tests are related to the test resources bicep file not creating HSM enabled AMLFS quite yet. This PR for AMLFS create should solve this issue. |
docs/azmcp-commands.md
Outdated
| azmcp azuremanagedlustre filesystem import-job create --subscription <subscription> \ | ||
| --resource-group <resource-group> \ | ||
| --file-system <filesystem-name> \ | ||
| --import-prefixes <prefix1> <prefix2> ... <prefixN> \ |
There was a problem hiding this comment.
| --import-prefixes <prefix1> <prefix2> ... <prefixN> \ | |
| [--import-prefixes <prefix1> <prefix2> ... <prefixN>] \ |
Given that this isn't required in the tool call.
| using Azure.Mcp.Tools.AzureManagedLustre.Services; | ||
| using Microsoft.Extensions.Logging; | ||
|
|
||
| namespace Azure.Mcp.Tools.AzureManagedLustre.Commands.FileSystem; |
There was a problem hiding this comment.
Will there be many job-based tools? Might be worth putting this into a sub-namespace of FileSystem given create is too generic of a Name.
|
|
||
| public override string Title => CommandTitle; | ||
|
|
||
| public override ToolMetadata Metadata => new() { Destructive = false, ReadOnly = false }; |
There was a problem hiding this comment.
Wouldn't this be considered a Destructive tool given OverwriteAlways can mutate existing resources
There was a problem hiding this comment.
Haven't updated yet. Currently figuring out if we want to remove the Overwrite options to prevent destruction.
| options.ConflictResolutionMode ?? "OverwriteAlways", | ||
| options.MaximumErrors ?? -1, |
There was a problem hiding this comment.
Based on BindOptions are the null-coalescing cases even possible?
| var options = BindOptions(parseResult); | ||
| try | ||
| { | ||
| if (!Validate(parseResult.CommandResult, context.Response).IsValid) | ||
| { | ||
| return context.Response; | ||
| } |
There was a problem hiding this comment.
There's an upcoming broad update where Validate should be called before BindOptions, mind updating that here.
There was a problem hiding this comment.
Also make sure you rebase with main
|
|
||
| # Returns the required number of IP addresses for a specific Azure Managed Lustre SKU and filesystem size | ||
| azmcp azuremanagedlustre filesystem required-subnet-size --subscription <subscription> \ | ||
| --sku <azure-managed-lustre-sku> \ | ||
| --size <filesystem-size-in-tib> | ||
|
|
||
| # Create an Azure Managed Lustre filesystem import job (preview / placeholder) | ||
| azmcp azuremanagedlustre filesystem import-job create --subscription <subscription> \ |
There was a problem hiding this comment.
don't include azure in the name
There was a problem hiding this comment.
This is a planned refactor which will be addressed after the current PRed actions are in. Here is the open conversion issue: #345 .
Is this a sufficient solution for now?
There was a problem hiding this comment.
Please take that as a PR this week, thanks.
docs/azmcp-commands.md
Outdated
| [--conflict-resolution-mode <conflict-mode>] \ | ||
| [--maximum-errors <maximum-errors>] \ | ||
| [--admin-status <admin-status>] \ | ||
| [--job-name <job-name>] |
There was a problem hiding this comment.
only include -name in option name if required to diff from another name
| { | ||
| var azureManagedLustre = new CommandGroup(Name, | ||
| "Azure Managed Lustre operations - Commands for listing and inspecting Azure Managed Lustre file systems (AMLFS) used for high-performance computing workloads."); | ||
| "Azure Managed Lustre operations - Commands for non-destructive interaction with Azure Managed Lustre file systems (AMLFS) used for high-performance computing workloads."); |
There was a problem hiding this comment.
Simplify this, too many repeated words
|
|
||
| fileSystem.AddCommand("list", new FileSystemListCommand(loggerFactory.CreateLogger<FileSystemListCommand>())); | ||
| fileSystem.AddCommand("required-subnet-size", new FileSystemSubnetSizeCommand(loggerFactory.CreateLogger<FileSystemSubnetSizeCommand>())); | ||
| var importJob = new CommandGroup("import-job", "AMLFS import job operations - Create manual import jobs to hydrate the file system namespace."); |
There was a problem hiding this comment.
What is AMLFS IDK if copilot will know that acronym.
There was a problem hiding this comment.
AMLFS is Azure Managed Lustre File System. I can expand to words in the next push.
| Assert.Equal("/", result.ImportJob.ImportPrefixes![0]); | ||
| } | ||
|
|
||
| // TODO: may need a delay between tests to prevent conflicts with successful jobs |
| IList<string>? importPrefixes = null, | ||
| string conflictResolutionMode = "OverwriteAlways", | ||
| int? maximumErrors = 0, | ||
| string? adminStatus = "Active", // TODO: discuss the necessity of this |
There was a problem hiding this comment.
@wolfgang-desalvador thoughts on the necessity of this param? It must always be "Active" when creating an import job. Only "Cancel" when canceling the job.
There was a problem hiding this comment.
Create and update will be separate commands so remove adminStatus can be hardcoded as "Active".
|
|
||
| # Returns the required number of IP addresses for a specific Azure Managed Lustre SKU and filesystem size | ||
| azmcp azuremanagedlustre filesystem required-subnet-size --subscription <subscription> \ | ||
| --sku <azure-managed-lustre-sku> \ | ||
| --size <filesystem-size-in-tib> | ||
|
|
||
| # Create an Azure Managed Lustre filesystem import job (preview / placeholder) | ||
| azmcp azuremanagedlustre filesystem import-job create --subscription <subscription> \ |
There was a problem hiding this comment.
This is a planned refactor which will be addressed after the current PRed actions are in. Here is the open conversion issue: #345 .
Is this a sufficient solution for now?
|
|
||
| fileSystem.AddCommand("list", new FileSystemListCommand(loggerFactory.CreateLogger<FileSystemListCommand>())); | ||
| fileSystem.AddCommand("required-subnet-size", new FileSystemSubnetSizeCommand(loggerFactory.CreateLogger<FileSystemSubnetSizeCommand>())); | ||
| var importJob = new CommandGroup("import-job", "AMLFS import job operations - Create manual import jobs to hydrate the file system namespace."); |
There was a problem hiding this comment.
AMLFS is Azure Managed Lustre File System. I can expand to words in the next push.
|
|
||
| public override string Title => CommandTitle; | ||
|
|
||
| public override ToolMetadata Metadata => new() { Destructive = false, ReadOnly = false }; |
| var options = BindOptions(parseResult); | ||
| try | ||
| { | ||
| if (!Validate(parseResult.CommandResult, context.Response).IsValid) | ||
| { | ||
| return context.Response; | ||
| } |
There was a problem hiding this comment.
Also make sure you rebase with main
|
|
||
| public static readonly Option<string> FileSystemOption = new( | ||
| $"--{fileSystem}", | ||
| "The name of the Azure Managed Lustre file system (AMLFS)." |
There was a problem hiding this comment.
All of these need to be updated to Required and explicit Description property set
| string? tenant = null, | ||
| RetryPolicyOptions? retryPolicy = null) | ||
| { | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(subscription); |
There was a problem hiding this comment.
Use ValidateRequiredParameters method
| var rg = await _resourceGroupService.GetResourceGroupResource(subscription, resourceGroup, tenant, retryPolicy) | ||
| ?? throw new Exception($"Resource group '{resourceGroup}' not found"); | ||
|
|
||
| var fs = await rg.GetAmlFileSystemAsync(fileSystemName); |
There was a problem hiding this comment.
IF this takes a handle, then make sure it is disposed
There was a problem hiding this comment.
The return object does not seem to be disposable.
67d1a8f to
a04e969
Compare
|
|
||
| public override string Title => CommandTitle; | ||
|
|
||
| public override ToolMetadata Metadata => new() { Destructive = false, ReadOnly = false }; |
There was a problem hiding this comment.
Please provide values for all hints.
| { | ||
| private const string CommandTitle = "Create AMLFS Import Job"; | ||
|
|
||
| private readonly Option<string> _fileSystemOption = AzureManagedLustreOptionDefinitions.FileSystemOption; |
There was a problem hiding this comment.
Please rebase and use new Option handling pattern. new-command.md can help
6fad6fb to
3d4d527
Compare
3d4d527 to
2539341
Compare
6e38b58 to
fa6f157
Compare
fa6f157 to
ced327f
Compare
|
Make sure to update |
What does this PR do?
Adds
azmcp azuremanagedlustre filesystem import-job createand required testing and docs. [#318]GitHub issue number?
Issue #318
Pre-merge Checklist
CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)README.mddocumentation/docs/azmcp-commands.md/docs/e2eTestPrompts.mdToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptscrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline