Repository navigation
fix(aws-batch): multi-node job nodes and Fargate platform checks - #1684
Satyam-Trivedi-ZS merged 2 commits into
Conversation
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
What I checked
- Both items of #1669 are covered. Per-node
nodeDetailsand ListJobs bymultiNodeJobIdare done. The Fargate platform rules the docs actually state are done, and the rules the docs don't pin down are written up innongoals/batch.md. - Layering is clean. Nodes are derived in the provider from the stored job, so there is no new state and nothing for persist to carry. The driver gains a plain
NodeDetailstype. The wire layer only maps it toJobDetail.nodeDetails{nodeIndex, isMainNode} andJobSummary.nodeProperties{isMainNode, numNodes, nodeIndex}, which match the API reference shapes.go generate ./...leaves no diff. - Parity against the SubmitJob, RegisterJobDefinition, ListJobs, JobDetail and multi-node user guide pages:
#Naddressing works, and the job's ID and ARN carry the node suffix.- Started, stopped and reason come from the job itself, as the guide says they come from the main node.
nodeOverridesis rejected on a Fargate queue ("use containerOverrides instead").- A multinode definition is rejected on a Fargate queue, and
nodePropertiestogether withFARGATEis rejected at register time. Both return ClientException 400.
- Gates on the PR head:
go build ./...,go vetandgo test -p 1on providers/aws/batch, server/aws/batch, services/batch and persist all pass.-racepasses on the two batch packages.- gofmt is clean,
go mod tidyandgo generateleave no diff, and CI is all green. - golangci-lint without
--new-from-revreports 10 issues, the same 10 as the stacked base 689b550 (dupl, goconst, one G115 in job_lifecycle.go), only shifted by line. All 10 come from #1648. This PR adds none.
- AWS CLI against
cloudemu serve:- describe-jobs on
$ID,#0,#2,#3: the parent hasnodeProperties.numNodes3 and nonodeDetails. Each node has the rightnodeDetails, image andexitCode0.#3is omitted. - list-jobs by
multiNodeJobId: the summaries are right, and--max-results 2paging returns tokenMQand then the last node. - The queue listing shows only the parent.
- The three Fargate rejections work.
- CancelJob on a node ID returns ClientException.
- A node ID on a non-MNP job returns nothing from both describe and list.
- With
--async-settle, the parent and its nodes move together from SUBMITTED to RUNNING with the samestartedAt. A non-zeromainNode(1) is reported correctly.
- describe-jobs on
- Terraform hashicorp/aws,
aws_batch_job_definitiontype multinode: apply, plan exit 0, an update to the node range command, plan exit 0, destroy. A multinode definition withplatform_capabilities = ["FARGATE"]fails apply with the new ClientException. - No AI attribution in the commits or the PR body.
High
None.
Medium
None.
Low
-
A node ID that isn't in canonical form still resolves, and the response uses a different jobId.
describe-jobs --jobs "$ID#01" "$ID#+1"returns the job"$ID#1"for both, becausestrconv.ParseIntaccepts a leading zero and a+sign (job_nodes.go:78). A client that matches results to request IDs won't find its entry. Real Batch treats these as unknown IDs. Accept only0or a digit string with no leading zero. The array-childsplitChildIDfrom #1648 has the same issue, so fix both together. -
A node of an array MNP job carries the parent's
arrayProperties. SubmitJob (#1648) acceptsarrayPropertieswith a multinode definition. When that happens,renderNodestarts fromrender(rec, -1), and eachJ#inode then reportsarrayProperties{size 2, statusSummary} next tonodeDetails. A node isn't an array parent, sorenderNodeshould clearArrayProperties(job_nodes.go:118, next toNodeProperties = nil). Separately, it's worth checking whether real Batch rejects arrays on multinode definitions. I couldn't find a statement either way in the reference. -
fargateQueuetreats a queue as Fargate if any of its compute environments is Fargate. It relies on the rule that a queue can't mix Fargate and EC2 (job_nodes.go:182), but the emulator doesn't enforce that rule:create-job-queuewithce-ec2andce-fgsucceeds. On such a queue, every MNP submission is now rejected. This gap is in CreateJobQueue and UpdateJobQueue, not this PR. A follow-up should reject the mixed order there, so the assumption behind this check holds. -
Two points from the #1648 review are still open. They aren't part of this PR's diff, but the new node rendering makes them visible:
containerOverrideson a multinode definition is still dropped silently. Submitting with--container-overrides command=xshowscontainer.command ["work"]on$ID#1(jobs.go:396,mergeContainerreturns unchanged when the definition has no containerProperties).- A node override aimed at part of a definition range is still rejected. Targeting
"2:"against a"1:"range returns "doesn't cover any node range" (overrides.go:552).
Both belong on #1648.
-
The SubmitJob reference doesn't document a check that the definition's
platformCapabilitiesmatches the queue (an EC2 definition on a Fargate queue, or the reverse), and the PR doesn't add one. That's reasonable given the evidence. It's worth a line innongoals/batch.mdso readers know it is unchecked and not just unmentioned.
| return "", 0, false | ||
| } | ||
|
|
||
| n, err := strconv.ParseInt(id[i+1:], 10, 32) |
There was a problem hiding this comment.
ParseInt accepts 01 and +1, so describe-jobs --jobs "$ID#01" returns a job whose jobId is $ID#1, which isn't the ID that was asked for. Real Batch treats a non-canonical node ID as unknown. Accept only 0 or digits with no leading zero, and apply the same check to splitChildID.
| suffix := nodeSeparator + strconv.Itoa(int(i)) | ||
| j.JobID += suffix | ||
| j.JobARN += suffix | ||
| j.NodeProperties = nil |
There was a problem hiding this comment.
SubmitJob accepts arrayProperties with a multinode definition. When it does, each J#i node inherits the parent's arrayProperties (size and statusSummary) from render(rec, -1). I'd clear j.ArrayProperties here as well, since a node isn't an array parent.
| // compute environments have FARGATE or FARGATE_SPOT compute resources. A queue | ||
| // can't mix Fargate and EC2 compute environments (Job queues on Fargate, Batch | ||
| // User Guide), so one Fargate compute environment decides. | ||
| func (m *Mock) fargateQueue(q *driver.JobQueue) bool { |
There was a problem hiding this comment.
This relies on a queue never mixing Fargate and EC2, but CreateJobQueue accepts order=1,ce-ec2 order=2,ce-fg today, and on that queue every MNP submission is rejected. That's a CreateJobQueue/UpdateJobQueue gap rather than one in this PR. A follow-up that rejects the mixed order would make this check hold.
DescribeJobs now resolves node job IDs ("<jobId>#<n>") and ListJobs with
multiNodeJobId lists the job's nodes instead of an empty list. Each node
carries nodeDetails (nodeIndex, isMainNode) and the container, ECS or EKS
properties of the node range that covers it; ListJobs summaries carry
nodeProperties (isMainNode, numNodes, nodeIndex).
Nodes are derived from the job record, the same way array children are,
so there is no new stored state. Every node follows the job's lifecycle
and, like array children, sends no state-change events of its own.
SubmitJob now returns a ClientException when nodeOverrides is sent to a job queue whose compute environments are FARGATE or FARGATE_SPOT, or when a multi-node job definition is submitted to one. The SubmitJob reference says nodeOverrides "isn't applicable to jobs that are running on Fargate resources; use containerOverrides instead", and the RegisterJobDefinition reference says multinode isn't supported for jobs run on Fargate. RegisterJobDefinition also rejects nodeProperties together with the FARGATE platform capability, per the same reference. The override tests that submit multi-node jobs now use an EC2 queue, so they keep testing the override rules rather than the platform rule.
fc2eaf1 to
fff15af
Compare
Builds on #1648 (merged); rebased onto
development, so the diff is now only this PR's changes.Summary
DescribeJobsresolves<jobId>#<n>, andListJobs --multi-node-job-idlists the nodes instead of returning an empty list.SubmitJobrejectsnodeOverrides, and multi-node job definitions, on a job queue backed by Fargate.RegisterJobDefinitionrejectsnodePropertiestogether with theFARGATEplatform capability.Part of #1669
Item status
nodeDetails, emptyListJobsbymultiNodeJobId)#Nnotation (starting with 0) ... describeaws_batch_job_id#1". JobDetailnodeDetails(nodeIndex,isMainNode). ListJobsmultiNodeJobId"lists all nodes that are associated with the specified job". JobSummarynodeProperties(isMainNode,numNodes,nodeIndex). Shapes checked against the botocore model.nodeOverrides: "This parameter isn't applicable to jobs that are running on Fargate resources; usecontainerOverridesinstead." RegisterJobDefinitiontype: "If the job is run on Fargate resources, thenmultinodeisn't supported";nodeProperties: "If the job runs on Fargate resources, then you must not specifynodeProperties". Job definitions on Fargate:type"Must be specified ascontainer". The only documented client error isClientException(HTTP 400).The other override/platform rules from the issue are left as they were, and
nongoals/batch.mdlists them:ecsPropertiesOverrideagainst EC2: SKIPPED, it isn't a real rule.ecsPropertiesruns on both EC2 and Fargate. The rule that does exist ("can only be specified for jobs that are run on Amazon ECS resources", and the reverse for EKS) needs EKS compute environments, and the emulator doesn't model those (eksConfigurationisn't stored). The existing check that the job definition hasecsProperties/eksPropertiesstays.UNMANAGEDcompute environments: UNVERIFIABLE. MNP compute environment considerations says they "aren't supported", but not whetherSubmitJobrejects the job or leaves it stuck.containerOverrides.instanceType/memory/vcpuson Fargate: SKIPPED. ContainerOverrides only says they "shouldn't be provided" or are "not supported", and doesn't say they're rejected.Changes
services/batch/driver: newNodeDetails(NodeIndex,IsMainNode,NumNodes) ondriver.Job. It is set only on a node.providers/aws/batch/job_nodes.go(new):job_lifecycle.go, so there's no new stored state and nothing new for persist or snapshots.nodeDetailsand thecontainer/ecsProperties/eksPropertiesof the node range that covers its index (nodeOverridesalready applied). It dropsnodeProperties.ListJobsbymultiNodeJobIdpages by node index with the same cursor token as array children. Every node shares the job's name, definition, creation time and status, so the first node decides status and filters, and the loop stays bounded by the page size.fargateQueue/validateJobPlatform/validateJobDefinitionPlatformimplement the Fargate rules. A queue counts as Fargate if a compute environment in its order hascomputeResources.typeFARGATEorFARGATE_SPOT. A queue can't mix Fargate and EC2 (Job queues on Fargate).providers/aws/batch/jobs.go:DescribeJobsfalls back to node IDs,ListJobsroutesmultiNodeJobIdto the node lister, andSubmitJobruns the platform check after resolving the queue and the definition.server/aws/batch/types_jobs.go: wire translation only.JobDetail.nodeDetailsandJobSummary.nodeProperties.docs/coverage/nongoals/batch.mdupdated andgo generate ./...output committed.Provider Coverage
Checklist
go test -p 2over every package, run in chunks of 30)golangci-lint run --new-from-rev=origin/fix/aws-batch-parity-1496onproviders/aws/batch,server/aws/batch,services/batch: 0 issues)cloudemu_test.go(not needed; provider and SDK wire tests cover it)go generate ./...) and committed the resultTest Plan
New tests. They fail on
689b5506and pass here; I checked by reverting the provider and wire changes:providers/aws/batch/job_nodes_test.go:TestMultiNodeJobDescribesEachNode: per-nodenodeDetails, range container, and the lifecycle on a FakeClock through RUNNING to SUCCEEDED withexitCode0.#3and#-1are omitted.TestMultiNodeNodesFollowCancelTestListJobsByMultiNodeJobID: order,numNodes, paging withmaxResults2, default RUNNING, filters, unknown and non-MNP IDs.TestMultiNodeNodesSurviveSnapshotTestSubmitJobRejectsMultiNodeOnFargate:FARGATE,FARGATE_SPOT, queue by ARN, and accepted on EC2.TestRegisterMultiNodeRejectsFargatePlatformTestMultiNodeNodesSendNoEventsis a guard, so it also passes on the base.server/aws/batch/jobs_sdk_test.go(aws-sdk-go-v2):TestSDKMultiNodeJobNodesTestSDKSubmitJobRejectsNodeOverridesOnFargate: asserts*types.ClientException.Commands:
go build ./...: ok.go test -race ./providers/aws/batch/... ./server/aws/batch/... ./providers/aws/ ./persist/... ./internal/settle/...: all ok.go test -p 2over all packages in chunks of 30: all ok.golangci-lint run --allow-parallel-runners --concurrency 2 --new-from-rev=origin/fix/aws-batch-parity-1496on the touched packages: 0 issues.Real-user check with the AWS CLI against
cloudemu serve --async-settle -providers aws -aws-port 4601. Every call used--endpoint-url http://127.0.0.1:4601.describe-jobs --jobs $ID $ID#0 $ID#1 $ID#2 $ID#3:nodeProperties.numNodes3 and nonodeDetails.#0has{"nodeIndex":0,"isMainNode":true}and imagelead.#1and#2haveisMainNode: false, imageworkerand command["work","--fast"].#3is omitted.#0and#1move together SUBMITTED, RUNNABLE, RUNNING (samestartedAt), then SUCCEEDED.list-jobs --multi-node-job-id $ID(default RUNNING, while running) returned both nodes withnodeProperties{isMainNode, numNodes: 2, nodeIndex}.--job-status SUCCEEDEDreturned#0,#1and#2, each withcontainer.exitCode0.--max-results 2 --no-paginatereturned#0and#1withnextTokenMQ. Passing that token returned#2with no token.submit-job --job-queue q-fg --job-definition mnp --node-overrides '{"numNodes":3}'returnedClientExceptionwith exit 255. Raw HTTP is 400 withX-Amzn-Errortype: ClientException.submit-job --job-queue q-fg --job-definition mnpreturnedClientException.register-job-definition --type multinode --platform-capabilities FARGATE ...returnedClientException.Known limits
RUNNING, and a child node can finish or fail on its own. The emulator runs no workload, so there's nothing that would make a node diverge.CancelJob/TerminateJobtake the job ID, not a node ID. A node ID returns the existing "does not exist"ClientException.SubmitJobwith an emulator-written message. The docs give the rule and theClientExceptiontype, but not the exact message.