Repository navigation
feat(aws-sagemaker): close real-cloud parity gaps in #1450 - #1548
Conversation
…to feat/sagemaker-parity
…alidation, security, tests)
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Thanks for this. It's a big piece of work and most of it holds up well. I checked out 2ad8082, built it, and ran it against the AWS CLI and the real Terraform AWS provider (6.66.0), with and without --async-settle and across a --persist restart. All 20 items from #1450 are addressed, and the Terraform lifecycle for model, endpoint config, endpoint and notebook instance is clean. I'm requesting changes for one regression in the error mapping and a few Medium gaps listed below.
Issue coverage (#1450)
| Item | Status | Evidence |
|---|---|---|
| UpdateNotebookInstance missing | Done | Works through the CLI. Terraform instance_type update goes stop, update, start, then plan is 0. Updating while InService gets the real Status (InService) not in ([Stopped, Failed]) message |
| DescribeEndpoint/ListEndpoints ProductionVariants shape | Done | Current/Desired weight and count, DeployedImages and VariantStatus are returned. PendingDeploymentSummary appears only while Updating |
| Model Monitor family | Done | Job definitions and schedules created. ListMonitoringAlerts can only be populated through a Go-only seam, see Medium 6 |
| TrialComponent family | Done | create/describe via CLI |
| ML Lineage | Done | CreateArtifact, AddAssociation, QueryLineage |
| Projects CRUD | Done | CreateInProgress, then CreateCompleted under async settle |
| DescribeTrainingJob dropped fields | Done | SecondaryStatusTransitions have distinct stage times, and the request is echoed |
| List filters / sort / pagination | Partial | Paging and filters work, but the default SortOrder is wrong for 4 ops, and LastModifiedTime filters silently return nothing on 2 ops. See Medium 2 and 3 |
| Create/UpdateEndpoint go straight to InService | Done | Under --async-settle, Creating, then Updating, then InService can all be observed. Terraform waiters finish in 1-3s |
| VariantName dim, 4XX/5XX, OverheadLatency, job utilization | Done (with Low gaps) | AWS/SageMaker has Invocations, InvocationsPerInstance, ModelLatency (Microseconds) and OverheadLatency, keyed by EndpointName+VariantName. See Low 1 and 2 |
| EventBridge state-change events | Partial | Events reach SQS through a rule on aws.sagemaker. Under --async-settle only CREATING/UPDATING/DELETING are sent and IN_SERVICE never is (documented). See Low 3 |
| Workteam/Workforce | Done | workteam/private-crowd/<name> ARN |
| Studio custom images | Done | image-version/<name>/1 |
| Search / GetSearchSuggestions | Done | Search returns training jobs. GetSearchSuggestions with hint Train returns no suggestions (Low) |
| BatchDescribeModelPackage, Get/PutModelPackageGroupPolicy | Done | Present and routed. CreateModelPackage still rejects a group ARN, see Medium 5 |
| DescribeCluster InstanceGroups counts | Done | ThreadsPerCore, counts and LifeCycleConfig are serialized |
| InvokeEndpoint echoes Accept as Content-Type | Done | Accept: */* with text/csv returns text/csv |
| Jobs complete synchronously | Done | InProgress/Starting, then later stages under async settle |
| Fabricated JobsCreated / ResourcesCreated metrics | Done | Removed |
None of these exist on the merge base: UpdateNotebookInstance, CurrentInstanceCount, the new family ops, SecondaryStatusTransitions, OverheadLatency, Invocation4XXErrors and the State Change detail types all return zero hits there. The new tests don't compile on the base because they call new driver methods.
Findings
High
1. Duplicate creates of jobs, feature groups, Studio children and clusters now return ValidationException instead of ResourceInUse. (server/aws/sagemaker/errors.go:36, pinned by server/aws/sagemaker/dispatch_test.go:75)
On the base, AlreadyExists mapped to ResourceInUse. This PR maps every AlreadyExists to ValidationException. That's right for CreateModel, CreateEndpointConfig and CreateEndpoint ("Cannot create already existing ..."). For these ops, AWS returns ResourceInUse and the SDK models it:
- CreateTrainingJob, CreateProcessingJob, CreateTransformJob, CreateHyperParameterTuningJob, CreateAutoMLJobV2, CreateLabelingJob, CreateCompilationJob
- CreateFeatureGroup
- CreateUserProfile, CreateSpace, CreateApp
- CreateCluster
The provider still returns plain AlreadyExists for all of them (jobs.go, feature_store.go, studio.go, cluster.go).
Repro: aws sagemaker create-training-job --training-job-name dupjob ... twice. The second call returns ValidationException: training job "dupjob" already exists.
Impact: idempotent job-submission code that does errors.As(err, &types.ResourceInUse{}) (or catches ResourceInUse in boto3) stops matching and treats a retry as a hard failure. That code worked before this PR.
Fix: return driver.InUse(...) from those creates, as you already do for Image, AppImageConfig, monitoring and Workteam. Keep the generic AlreadyExists to ValidationException mapping only for ops whose SDK error list has no ResourceInUse. Then fix the test row.
Medium
1. A deleted endpoint comes back InService after a --persist restart when --async-settle is on. (providers/aws/sagemaker/endpoint.go:549)
deleteEndpoint only records a Deleting transition in epTrans. The endpoint is purged lazily on the next read. Snapshot dumps the store as-is, and epTrans isn't persisted.
Repro:
- Start serve with
--async-settle --persist. - Run
create-endpoint pe, wait for InService, thendelete-endpoint pe. - Stop the server and start it again.
describe-endpoint pereturnsInServiceandlist-endpointsshows it.
Until that next read, arnExists (tags.go:203, m.endpoints.Has) also still accepts AddTags on the deleted endpoint.
Fix: drop endpoints with a Deleting transition when snapshotting (or purge any elapsed Deleting endpoint first), and route the tag liveness check through liveEndpoint.
2. Default SortOrder is ascending where AWS defaults to Descending.
AWS documents SortOrder default Descending for ListEndpoints, ListModels, ListEndpointConfigs and ListTransformJobs (ListEndpoints, ListModels, ListEndpointConfigs, ListTransformJobs). None of their specs sets defaultOrder:
providers/aws/sagemaker/endpoint.go:313inference.go:61inference.go:135jobs.go:379
Repro: create ord-a, ord-b, ord-c, then run list-endpoint-configs. It returns them oldest first.
Impact: with the default page size of 10, list-endpoints --max-results 1 and any "latest first" code get the oldest item. ListTrainingJobs and ListProcessingJobs default to Ascending correctly.
3. LastModifiedTime filters silently return nothing, and a valid SortBy is rejected, on two list ops.
ListNotebookInstanceLifecycleConfigs(notebook.go:389) andListCodeRepositories(notebook.go:467) have nomodifiedaccessor.- So
--sort-by LastModifiedTimereturnsValidationException ... not supported by this operation. - And
--last-modified-time-after 2000-01-01returns an empty list, becauseinRangereturns false when the accessor is nil (list.go:277). - Both ops document
LastModifiedTimeas a SortBy value and as a filter (ListNotebookInstanceLifecycleConfigs, ListCodeRepositories). - ListCodeRepositories and ListNotebookInstances (
notebook.go:135) also defaultSortBytoNamein AWS, but default to CreationTime here.
An unsupported filter field should never quietly empty the result. Add the modified accessors and the Name defaults.
4. InvokeEndpointAsync ignores the caller's InferenceId. (server/aws/sagemaker/runtime.go:108-112, :122, providers/aws/sagemaker/runtime.go:158)
The handler never reads X-Amzn-SageMaker-Inference-Id, so --inference-id myid-1 comes back as "InferenceId": "0000008e". AWS only generates one "if none is specified" (InvokeEndpointAsync). Clients that correlate async results by their own id lose the link. The handler also reads the content type from Content-Type rather than X-Amzn-SageMaker-Content-Type.
5. CreateModelPackage rejects a group ARN. (providers/aws/sagemaker/registry.go:126)
ModelPackageGroupName is "The name or Amazon Resource Name (ARN) of the model package group" (CreateModelPackage). Describe/List/Delete and the policy ops normalise through groupNameOf, but create checks the raw value. Passing arn:aws:sagemaker:us-east-1:000000000000:model-package-group/grp1 returns model package group "arn:..." not found.
Fix: cfg.GroupName = m.groupNameOf(cfg.GroupName) on entry.
6. Production code exposes test-only seams that serve can't reach.
These have no non-test caller:
SetInvocationFailure(providers/aws/sagemaker/runtime.go:165)PutMonitoringAlert(monitoring.go:656)SetMonitoringExecutionStatus(monitoring.go:684)
PutMonitoringAlert is the only writer of monAlerts, so through serve or Docker ListMonitoringAlerts is always empty. invFailures is also never cleared when an endpoint is deleted, so a failure seeded on one endpoint applies to a re-created endpoint with the same name.
Either expose these through the seed/admin surface, so they're real features for every way of running cloudemu, or move them into an export_test.go.
7. The InitialVariantWeight default lives only in the wire layer. (server/aws/sagemaker/inference.go:290)
weightOrDefault turns an omitted weight into 1 in the server only. driver.ProductionVariant.InitialVariantWeight is a plain float64, so a Go library user who omits it stores 0. DescribeEndpoint then shows CurrentWeight: 0, and variant picking falls back, so the library and serve disagree.
Make the field *float64 (or otherwise carry "unset") and apply the default of 1.0 in the provider.
Low
-
InvocationsPerInstance is always 1 (
providers/aws/sagemaker/metrics.go:69). AWS sends 1/numberOfInstances per request (monitoring-cloudwatch), so a variant with 2 instances reports double. -
Other metric gaps.
- Invocation4XXErrors/5XXErrors are only emitted on failure. AWS sends 0 otherwise, so the Average (error rate) is always 1 here.
- A bad TargetVariant emits a 4XX datapoint under a
VariantNamethat doesn't exist (nope). - Pipeline metrics use unit
Noneinstead ofCount.
-
No terminal event under
--async-settle(providers/aws/sagemaker/events.go:42). A rule onEndpointStatus: IN_SERVICE(or a training job'sCompleted) never fires under async settle, which is the main reason people wire these events. It's documented, but the observe path that flips the status could also emit the transition once. -
Monitoring executions freeze after 1000 fires (
monitoring.go:528).fireskeeps the oldest 1000, so an hourly schedule stops producing new executions after about 41 days, or after a large FakeClock advance. Keep the newest N instead. -
UpdateEndpointWeightsAndCapacities rejects DesiredInstanceCount=0 (
endpoint.go:501). The type allows a minimum of 0 (DesiredWeightAndCapacity). -
The reflected-content guard is a short denylist (
server/aws/sagemaker/runtime.go:100).application/*+xmlandtext/xslstill render as active content. Neuter any+xmlsuffix, or addContent-Security-Policy: sandboxto invocation responses. -
Message wording differs from AWS.
- AWS quotes the ARN for not-found:
Could not find endpoint "arn:aws:sagemaker:...:endpoint/nope". - Duplicate model and endpoint creates say
Cannot create already existing model "arn:...". - DescribeNotebookInstance on a missing name returns the bare message
RecordNotFound.
Terraform's substring checks still pass.
- AWS quotes the ARN for not-found:
-
ARNs keep the caller's casing. AWS lowercases the name segment, e.g. the EventBridge doc example
"EndpointName": "MyEndpoint"with.../endpoint/myendpoint. This predates the PR, but every new family copies it. Lineage artifact ARNs use a 17-character id where AWS uses 32 hex characters. -
Small fidelity gaps.
DeployedImages.ResolvedImageshould be digest-pinned (image@sha256:...), not a copy of the tag.- Training-job events send
"0"for unset start and end times instead of omitting them. - Notebook instances skip
Pendingeven under--async-settle. DeleteWorkteamon a missing team returns{"Success": false}. Please confirm AWS doesn't raise ValidationException here.
-
Concurrency nits (no data race, but logical races).
DeleteDomainchecks for children and then deletes without a lock, while the child creates check the domain unlocked, which can leave orphans.- Event payloads use the
.Tagscopy taken at create, so they're stale after AddTags/DeleteTags.
-
Structure. The endpoint driver API is still in
services/sagemaker/driver/inference.go, while provider and server now haveendpoint.go. All new families go into the genericservices/sagemaker/wrappers_rest.gorather than per-feature files. -
Docs and limits.
docs/services.mdsays "Real invocation, job-utilization and pipeline metrics", while the non-goals say the values are nominal. Drop "Real".- The 1 MiB control-plane body cap rejects a maximum-size
PipelineDefinitiononce it is JSON-escaped.
-
Coverage.
server/aws/sagemakeris at 73.2% andproviders/aws/sagemakerat 87.8%, both under the 90% target.services/sagemakeris at 98.5%.
AWS docs divergences (summary)
- Default SortOrder Descending: ListEndpoints, ListModels, ListEndpointConfigs, ListTransformJobs
- LastModifiedTime sort and filters, Name default: ListNotebookInstanceLifecycleConfigs, ListCodeRepositories, ListNotebookInstances
- Group name or ARN: CreateModelPackage
- Caller InferenceId: InvokeEndpointAsync
- Minimum DesiredInstanceCount 0: DesiredWeightAndCapacity
- InvocationsPerInstance, 4XX/5XX zero datapoints, pipeline metric units: monitoring-cloudwatch
- Model, Image, Image Version, Endpoint Deployment and Pipeline Step events not emitted: EventBridge events. Partly listed as non-goals.
- DeleteModelPackageGroup: AWS documents a
ConflictException(DeleteModelPackageGroup). Please double-check that a group which still holds versions is cascade-deleted by AWS rather than refused. The emulator cascades.
Terraform and CLI results
Terraform (hashicorp/aws 6.66.0, default sagemaker endpoint override) with aws_sagemaker_model, aws_sagemaker_endpoint_configuration, aws_sagemaker_endpoint and aws_sagemaker_notebook_instance:
- apply: created in 0s (endpoint 3s under
--async-settle), no waiter hang. plan -detailed-exitcode: 0.- Update notebook
instance_type, tags on every resource, and the endpoint config weight. This replaces the config and sends UpdateEndpoint. Applied cleanly in both modes. - plan: 0.
- After a
--persistrestart, plan is still 0. - destroy: clean.
- Describe on all four afterwards returns the not-found ValidationException.
AWS CLI:
- Not-found and duplicate cases for endpoint, config, model, notebook and training job.
- Notebook update and delete while InService (correctly refused).
- Update while Updating (refused).
- InvokeEndpoint with Content-Type, Accept and TargetVariant, plus missing endpoint (400
ValidationError "Endpoint nope of account ... not found."). - InvokeEndpointAsync.
- CloudWatch list-metrics and get-metric-statistics.
- EventBridge rule to SQS.
- New families: image, image version, trial component, artifact, project, workforce, workteam, search, data-quality job definition.
What I verified
- Clean build of 2ad8082 with
go build ./.... go veton the touched packages.gofmtclean on the changed files.go mod tidyleaves no diff.go generate ./...leaves no diff.- golangci-lint reports 0 new issues against the merge base on the touched packages.
go test -racepasses onproviders/aws/sagemaker,server/aws/sagemaker,services/sagemaker,features/chaos,services/resourcediscoveryandpersist, plus the SageMaker, wiring, authz and golden tests inproviders/aws,server/awsand the root package.- A concurrent probe ran 8 goroutines × 200 ops of create, describe, update, delete and list endpoint, notebook stop/update/start, invoke, and clock advance under
--async-settle, three times with-race. No races. - The branch merges cleanly into the current development branch.
- CI is all green.
Verdict
Request changes. The ResourceInUse regression (High) and Medium 1 to 7 should be fixed before merge. The Low items can go in a follow-up.
| wire.WriteJSONError(w, http.StatusBadRequest, "ResourceNotFound", cerrors.Message(err)) | ||
| case cerrors.IsAlreadyExists(err): | ||
| wire.WriteJSONError(w, http.StatusBadRequest, "ResourceInUse", err.Error()) | ||
| wire.WriteJSONError(w, http.StatusBadRequest, driver.ExValidation, cerrors.Message(err)) |
There was a problem hiding this comment.
This maps every AlreadyExists to ValidationException. That's right for CreateModel/CreateEndpointConfig/CreateEndpoint, but AWS (and the SDK error lists) return ResourceInUse for duplicate CreateTrainingJob/ProcessingJob/TransformJob/HyperParameterTuningJob/AutoMLJobV2/LabelingJob/CompilationJob, CreateFeatureGroup, CreateUserProfile/Space/App and CreateCluster. The base mapped these to ResourceInUse, so this is a regression: create-training-job twice now returns ValidationException: training job "dupjob" already exists, and errors.As(err, &types.ResourceInUse{}) stops matching. Could those creates return driver.InUse(...) (as Image/Workteam already do), with this generic mapping kept only for ops whose SDK error list has no ResourceInUse?
There was a problem hiding this comment.
Confirmed against the SDK error lists: CreateTrainingJob/ProcessingJob/TransformJob/HyperParameterTuningJob/AutoMLJobV2/LabelingJob/CompilationJob, CreateFeatureGroup, CreateUserProfile/Space/App and CreateCluster all model ResourceInUse, and the base mapped every AlreadyExists to it. Those twelve creates now return driver.InUse(...); the generic AlreadyExists -> ValidationException mapping stays for the operations whose list has no ResourceInUse (CreateModel, CreateEndpointConfig, CreateEndpoint, CreateNotebookInstance and so on). Pinned per operation in the provider test, and an SDK round trip checks that a duplicate CreateTrainingJob decodes as *types.ResourceInUse. Fixed in 64c3a74.
| typ string | ||
| }{ | ||
| {"not found", cerrors.New(cerrors.NotFound, "x"), 400, "ResourceNotFound"}, | ||
| {"already exists is validation", cerrors.New(cerrors.AlreadyExists, "x"), 400, "ValidationException"}, |
There was a problem hiding this comment.
This row pins the generic AlreadyExists -> ValidationException rule for every op, which is wrong for jobs, feature groups, Studio children and clusters (they model ResourceInUse). Could you replace it with per-op cases once the creates return the right typed error?
There was a problem hiding this comment.
The row no longer claims to cover every op: it is renamed to the plain already-exists case (an operation with no ResourceInUse), and the per-op cases live in TestDuplicateCreates_ExceptionPerOp (provider) and TestSDK_DuplicateCreate_ResourceInUse (SDK). Fixed in 64c3a74.
| return ep, nil | ||
| } | ||
|
|
||
| m.beginEndpointTransition(name, driver.EndpointDeleting, now, settle.DefaultEndpointDeleteSettle, nil, "") |
There was a problem hiding this comment.
Under --async-settle the endpoint stays in the store with a Deleting transition in epTrans, which isn't persisted. If nobody reads it before shutdown, a --persist restart brings it back as InService (repro: create pe, wait, delete-endpoint pe, restart, describe-endpoint pe -> InService, list-endpoints shows it). tags.go:203 also lets AddTags hit it until the lazy purge. Could Snapshot skip or purge endpoints in Deleting, and the tag liveness check go through liveEndpoint?
There was a problem hiding this comment.
Reproduced and fixed. Snapshot now leaves out endpoints that have a Deleting transition, together with their tags, without touching the live mock (it is still Deleting and is purged on the next read). The tag liveness check goes through endpointStatus, so an endpoint whose Deleting window has elapsed is not taggable even before the lazy purge. Checked with the CLI against --async-settle --persist: create pe, wait, delete-endpoint pe, restart; describe-endpoint and add-tags both say not found, list-endpoints omits it, and the rest of the state survived. Fixed in 64c3a74.
| } | ||
| m.epMu.Unlock() | ||
|
|
||
| return listItems(observed, listSpec[driver.Endpoint]{ |
There was a problem hiding this comment.
ListEndpoints defaults SortOrder to Descending in AWS (API_ListEndpoints), but this spec sets no defaultOrder, so results come back oldest first and --max-results 1 returns the oldest endpoint. ListModels (inference.go:61), ListEndpointConfigs (inference.go:135) and ListTransformJobs (jobs.go:379) have the same problem.
There was a problem hiding this comment.
Confirmed (ListEndpoints / ListModels / ListEndpointConfigs / ListTransformJobs document SortOrder default Descending). All four specs now set defaultOrder: Descending; training and processing jobs keep their ascending default. list-endpoint-configs and --max-results 1 return the newest first. Fixed in 64c3a74.
| out := make([]driver.NotebookLifecycleConfig, 0, len(all)) | ||
| //nolint:gocritic,lll // large value params mirror the driver interface; SDK-shaped literal | ||
| func (m *Mock) ListNotebookInstanceLifecycleConfigs(_ context.Context, opts driver.ListOptions) (driver.ListPage[driver.NotebookLifecycleConfig], error) { | ||
| return listFrom(m.notebookLCs, listSpec[driver.NotebookLifecycleConfig]{ |
There was a problem hiding this comment.
There's no modified accessor here, so --sort-by LastModifiedTime (a documented value) returns a ValidationException, and --last-modified-time-after 2000-01-01 silently returns an empty list (inRange returns false for a nil accessor, list.go:277). ListCodeRepositories (line 467) has the same gap. AWS also defaults SortBy to Name for ListCodeRepositories and ListNotebookInstances.
There was a problem hiding this comment.
Added the modified accessors for lifecycle configs and code repositories (code repositories gained LastModifiedTime, equal to the creation time because there is no UpdateCodeRepository, and it is returned by Describe and the summaries), and the Name default sort for ListCodeRepositories and ListNotebookInstances. Fixed in 64c3a74.
|
|
||
| m.putMetrics(ctx, []mondriver.MetricDatum{ | ||
| m.datum(nsEndpoint, "Invocations", 1, unitNone, dims, now), | ||
| m.datum(nsEndpoint, "InvocationsPerInstance", 1, unitNone, dims, now), |
There was a problem hiding this comment.
AWS reports InvocationsPerInstance as 1/numberOfInstances per request (monitoring-cloudwatch), so a 2-instance variant is double-counted here. Small one, but it's the metric target-tracking scaling uses.
There was a problem hiding this comment.
Per request it is now 1/instances (the variant's serving count); a variant scaled to zero publishes no per-instance value. A 2-instance variant reports 0.5 for one call (checked through CloudWatch), and the test that pinned 1 is corrected. Fixed in 64c3a74.
| // states are not emitted, because the mock has no scheduler to fire them. | ||
| func (m *Mock) emitStates(ctx context.Context, detailType string, resources, states []string, detail func(state string) any) { | ||
| if m.opts.SettleDuration(settle.DefaultJobStageSettle) > 0 || m.opts.SettleDuration(settle.DefaultEndpointSettle) > 0 { | ||
| states = states[:1] |
There was a problem hiding this comment.
Under --async-settle only the first state is published, so a rule on EndpointStatus IN_SERVICE or a training job's Completed never fires, and that's the main thing people use these events for. Since the observe path already flips the status, could it emit the transition once when it settles?
There was a problem hiding this comment.
Done the way you suggested: the observe path publishes the settled state once. Endpoints publish IN_SERVICE the first time they are read after their Creating or Updating window elapsed, and the four job families publish their terminal status (Completed, Stopped, Failed) the first time they are read settled, each guarded so repeated reads never duplicate. Checked with an EventBridge rule on aws.sagemaker to SQS under --async-settle: CREATING at the call, IN_SERVICE once after it settles. Documented in the non-goals page: the mock has no scheduler, so it is the read that notices the settle, and state restored from a snapshot can publish its settled state again on its first read. Fixed in 64c3a74.
| } | ||
|
|
||
| now := m.opts.Clock.Now() | ||
| fires := sched.fires(parseTime(s.ActiveFrom), now, driver.MaxMonitoringExecutionsPerSchedule) |
There was a problem hiding this comment.
fires collects the oldest fire times and stops at 1000, so an hourly schedule stops producing new executions after about 41 days (or after a big FakeClock advance), and LastMonitoringExecutionSummary freezes. Keeping the newest N would avoid that.
There was a problem hiding this comment.
fires now keeps the newest N (it walks back from the latest possible fire and returns them oldest first), so an hourly schedule keeps producing current executions after a long run or a big FakeClock advance. Fixed in 64c3a74.
|
|
||
| if w.DesiredInstanceCount != nil { | ||
| if *w.DesiredInstanceCount < 1 { | ||
| return errors.Newf(errors.InvalidArgument, "desiredInstanceCount for variant %q must be at least 1", w.VariantName) |
There was a problem hiding this comment.
DesiredWeightAndCapacity allows DesiredInstanceCount down to 0 (scale to zero), but this rejects anything below 1.
There was a problem hiding this comment.
DesiredInstanceCount of 0 is accepted (scale to zero) and only a negative count is rejected. Fixed in 64c3a74.
| media := strings.ToLower(strings.TrimSpace(strings.SplitN(ct, ";", 2)[0])) //nolint:mnd // protocol constant | ||
|
|
||
| switch media { | ||
| case "text/html", "application/xhtml+xml", "image/svg+xml", "text/xml", "application/xml": |
There was a problem hiding this comment.
The denylist misses application/*+xml (e.g. application/rss+xml) and text/xsl, which browsers still render as active XML. Matching any +xml suffix, or adding Content-Security-Policy: sandbox on invocation responses, closes it.
There was a problem hiding this comment.
Both: any +xml media type and the XSL types (text/xsl, application/xslt+xml) are served as application/octet-stream, and invocation responses carry Content-Security-Policy: sandbox. The SDK test covers rss, atom, mathml, xsl and a mixed-case type with a charset. Fixed in 64c3a74.
…to feat/sagemaker-parity
Return ResourceInUse for duplicate creates of jobs, feature groups, Studio user profiles, spaces, apps and clusters, as the SDK models it, and keep the validation-style duplicate for operations without it. Do not persist endpoints that are being deleted, and check tag liveness through the endpoint observer. Default list order to Descending for endpoints, models, endpoint configs and transform jobs, add LastModifiedTime accessors and the Name default sort for the notebook family, and reject a filter on a field an operation lacks. Honor the caller's InferenceId and content-type header on InvokeEndpointAsync, accept a group ARN in CreateModelPackage, and apply the default variant weight in the provider. Move test-only seams to export_test.go. Emit InvocationsPerInstance as 1/instances, publish the settled state once under async settle, keep the newest monitoring fires, allow DesiredInstanceCount 0, and neuter any +xml type with a sandbox CSP header.
|
Thanks for the thorough review, and for running the Terraform and CLI checks yourself. Round 1 is pushed in 64c3a74 (and a merge of the latest Coverage after the fixes: |
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. I re-checked 64c3a74 against the AWS CLI and the Terraform AWS provider (6.66.0), with and without --async-settle, and across a --persist restart. The ResourceInUse regression and all seven Mediums are fixed, and most of the Lows are too. One new problem came in with the fix for settled-state events, though: the IN_SERVICE event is now published while the endpoint lock is held, so a Lambda target that reads the endpoint back deadlocks every endpoint operation. That's the only thing I'd block on.
Status of the earlier findings
| Earlier item | Status | Evidence |
|---|---|---|
| High: AlreadyExists mapped to ValidationException | Fixed | Duplicate create-training-job and create-feature-group now return ResourceInUse. Duplicate create-model still returns ValidationException. I compared the duplicate error of every Create op against the SDK error lists: every op whose list has ResourceInUse returns it, and none of the others do |
M1: deleted endpoint back InService after a --persist restart |
Fixed | With --async-settle --persist: create pe and keep, wait for InService, delete pe, restart. describe-endpoint pe returns not found, list-endpoints shows only keep, and pe's tags are gone. AddTags now goes through the observed status |
| M2: default SortOrder for 4 list ops | Fixed | list-endpoint-configs, list-models and list-transform-jobs return newest first. --max-results 1 returns the newest. --sort-order Ascending still works. ListTrainingJobs is unchanged |
| M3: LastModifiedTime on lifecycle configs and code repos, Name defaults | Fixed | --sort-by LastModifiedTime and --last-modified-time-after 2000-01-01 both work on these two ops. list-code-repositories and list-notebook-instances sort by name by default. An unsupported filter is now an error, not an empty list. I sent every documented filter and SortBy value to every list op the server serves, and only ListUserProfiles and ListSpaces still reject one (new Low 2) |
| M4: InvokeEndpointAsync ignores InferenceId | Fixed | --inference-id myid-1 comes back as myid-1 with output s3://b/async/myid-1.out. Without one an id is generated, and 65 characters gets the length ValidationError. The content type now comes from X-Amzn-SageMaker-Content-Type |
| M5: CreateModelPackage rejects a group ARN | Fixed | Passing the group ARN creates model-package/grp1/1. An ARN for a missing group gives not found |
| M6: test-only seams in production | Mostly fixed | The three seams are in export_test.go, and invFailures is cleared when an endpoint is purged. The state they feed is still production code, see Low 3 |
| M7: default weight only in the wire layer | Fixed | driver.ProductionVariant.InitialVariantWeight is now *float64, and the provider stores 1.0 when it's omitted. Describe shows 1 through serve, and the provider test covers the library path. See Low 4 about the API change |
| L1: InvocationsPerInstance always 1 | Fixed | On a variant with 2 instances, each request sends 0.5 |
L2: 4XX/5XX zero datapoints, nope variant, pipeline unit |
Not fixed (documented) | Listed as a non-goal now. A bad TargetVariant still creates Invocation4XXErrors under VariantName=nope, and pipeline metrics still use None |
L3: no terminal event under --async-settle |
Fixed, but see High 1 | Through SQS I see training InProgress then Completed, and endpoint CREATING then IN_SERVICE then UPDATING. Each event is sent once |
| L4: monitoring executions freeze after 1000 fires | Fixed | fires walks back from to and keeps the newest N. There's a test for it |
| L5: DesiredInstanceCount=0 rejected | Fixed | update-endpoint-weights-and-capacities ... DesiredInstanceCount=0 is accepted |
| L6: reflected-content denylist | Fixed | application/rss+xml and text/xsl come back as application/octet-stream, with Content-Security-Policy: sandbox and nosniff |
| L7: message wording | Not fixed (documented as a follow-up) | |
| L8: ARN casing, lineage id length | Not fixed (documented as a follow-up) | |
L9: ResolvedImage digest, "0" event times, notebook Pending, DeleteWorkteam |
Partly documented | ResolvedImage and notebook Pending are on the follow-up list. The "0" start/end times in training events and DeleteWorkteam returning Success: false on a missing team aren't mentioned |
| L10: DeleteDomain race, stale tags in events | Not fixed | The stale tags are documented. The DeleteDomain check-then-delete is unchanged |
| L11: structure | Not fixed | Fine as a follow-up |
| L12: "Real" in services.md, 1 MiB body cap | Fixed / documented | |
| L13: coverage | Partly | providers/aws/sagemaker is at 91.9% in my run and services/sagemaker at 98.5%. server/aws/sagemaker is still 73.3% |
Findings
High
1. The settled IN_SERVICE event is published while epMu is held, so a target that reads the endpoint deadlocks it. (providers/aws/sagemaker/endpoint.go:167 and :202-210)
observeEndpoint now calls publishEndpointSettled, and its comment says "The caller holds epMu". DescribeEndpoint, ListEndpoints, UpdateEndpoint and the AddTags liveness check all call it with that lock held. emitEndpointEvents delivers to EventBridge, and EventBridge invokes a Lambda target synchronously. If that target calls DescribeEndpoint (or anything else that takes epMu), it blocks on the non-reentrant mutex. The original Describe never returns, and every endpoint operation after it hangs.
Repro (Go library, aws.New(config.WithClock(clk), config.WithAsyncSettle())):
- Create model, config and endpoint
ep. - Add a rule on
{"source":["aws.sagemaker"],"detail-type":["SageMaker Endpoint State Change"],"detail":{"EndpointStatus":["IN_SERVICE"]}}with a Lambda target. Its registered handler callsp.SageMaker.DescribeEndpoint(ctx, "ep"). - Advance the clock past the settle window and call
DescribeEndpoint.
The handler fires once, and DescribeEndpoint never returns: I gave up after 5s. "When the endpoint is in service, look it up and do X" is the main reason people wire this event. Before this commit the endpoint events were only sent from CreateEndpoint, UpdateEndpoint and DeleteEndpoint after the lock was released (endpoint.go:37, :381, :474, :555). The ECS and Lambda event paths have the same rule and pin it in providers/aws/lifecycle_events_reentry_test.go.
Fix: have the observer only record that a settled event is due, for example by returning it from liveEndpoint or adding it to a slice under the lock. Publish it after epMu.Unlock(), with the caller's ctx instead of context.Background(). A test along the lines of lifecycle_events_reentry_test.go would pin it. The job paths (publishSettled in jobs.go) observe without a lock, so they're fine.
Medium
None.
Low
- Lows not fixed and not documented. Training-job events still send
"0"for an unset start or end time, andDeleteWorkteamon a missing team returns{"Success": false}(workforce.go:548). Either fix them or add them to the follow-up list next to the others. - ListUserProfiles and ListSpaces reject
--sort-by LastModifiedTime(providers/aws/sagemaker/studio.go:141,:212). These ops got list sorting in this PR.LastModifiedTimeis one of the two documented SortBy values (ListUserProfiles, ListSpaces), but both returnValidationException ... not supported by this operation.driver.UserProfileanddriver.Spacehave no LastModifiedTime, so Describe doesn't return one either. This is the same issue as the old Medium 3, on two more ops. - The model-failure state is still production code that only tests can reach. The
invFailuresmap,invMu,invocationFailure,clearInvocationFailure(sagemaker.go:128,runtime.go:175-186) and the 5XX/ModelError branch of InvokeEndpoint can only be set throughexport_test.go. Through serve, that branch never runs. If it's meant as a test-only behavior, it's cleaner to inject it as a hook. Otherwise expose it through the seed/admin surface. InitialVariantWeightchanging fromfloat64to*float64breaks callers of the public driver package. Any Go library caller that writesInitialVariantWeight: 1stops compiling. That's the right shape, but it needs a line in the release notes.- The new driver comment is wrong about AWS.
services/sagemaker/driver/notebook.go:136says "there is no UpdateCodeRepository". AWS has UpdateCodeRepository, and the emulator returnsUnknownOperationExceptionfor it, so a Terraformaws_sagemaker_code_repositoryupdate fails. The missing op predates this PR, but the comment should say the op isn't implemented yet, not that it doesn't exist. settledEmittedonly grows (events.go:43-51). A key is added for every settled job state and never removed, so a long-running server holds one entry per job forever. This is minor, but it could live on the job record (likesettledPublishedon the endpoint transition) instead of in a side map.
AWS docs divergences (new or still open)
- LastModifiedTime SortBy: ListUserProfiles, ListSpaces
- UpdateCodeRepository is a real op: UpdateCodeRepository
- Still open, now documented as non-goals or follow-ups: zero 4XX/5XX datapoints and the pipeline metric unit (monitoring-cloudwatch), message wording, ARN casing.
Terraform and CLI results
Terraform (hashicorp/aws 6.66.0) with aws_sagemaker_model, aws_sagemaker_endpoint_configuration, aws_sagemaker_endpoint and aws_sagemaker_notebook_instance, against a default server and against an --async-settle server:
- apply: 4 added (10s default, 8s async), no waiter hang.
plan -detailed-exitcode: 0.- Update notebook
instance_type, every tag, and the variant weight. The weight change replaces the config and sends UpdateEndpoint. Result: 1 added, 3 changed, 1 destroyed. The endpoint ends up InService ontfc-2with CurrentWeight 2, and the notebook is InService onml.t3.large. - plan: 0.
- destroy: 4 destroyed.
- Describe on all four afterwards returns the not-found ValidationException.
On the --async-settle --persist server I also ran apply, then a restart, then plan (0), then destroy (clean).
AWS CLI (--async-settle --persist):
- Duplicate training job, feature group and model.
- The endpoint delete and restart sequence above, plus tags.
- Default sort order on configs, models, endpoints and transform jobs,
--max-results 1, explicit Ascending. - LastModifiedTime sort and filter on lifecycle configs and code repos, and the Name defaults.
- A probe that sends every documented filter and SortBy value to every served list op.
invoke-endpoint-asyncwith, without, and with an oversized--inference-id.invoke-endpointwithapplication/rss+xmlandtext/xsl, and the response headers.- CreateModelPackage with a group ARN and with a missing-group ARN.
get-metric-statisticsfor InvocationsPerInstance, andlist-metrics.- An EventBridge rule to SQS for training job and endpoint events.
update-endpoint-weights-and-capacitiesto 0.
What I verified
- Clean build of 64c3a74.
go veton the touched packages.gofmtclean.go mod tidyleaves no diff.go run ./internal/coveragegenleaves no diff.- golangci-lint reports 0 new issues against 2f3f2eb and against the merge base, on the sagemaker packages and
providers/aws. go test -racepasses onproviders/aws/sagemaker,server/aws/sagemakerandservices/sagemaker, plus the SageMaker, lifecycle, re-entry, wiring and discovery tests inproviders/aws,persist, the root SageMaker tests, and the authorization and golden tests inserver/aws.- A concurrent probe ran 8 goroutines × 150 rounds of create, describe, list and delete endpoint, Snapshot, ListTrainingJobs, and clock advance under async settle, twice with
-race. No races. - CI is all green.
Verdict
Request changes for High 1 only. It's a small fix: publish the settled event after releasing epMu. Everything from the first round is fixed or documented, and the Lows can go in a follow-up.
|
|
||
| tr, ok := m.epTrans[ep.EndpointName] | ||
| if ok && !tr.active(now) { | ||
| m.publishEndpointSettled(tr, out) |
There was a problem hiding this comment.
This publishes the settled IN_SERVICE event from inside observeEndpoint, which runs with epMu held (DescribeEndpoint, ListEndpoints, UpdateEndpoint, the AddTags liveness check). EventBridge invokes a Lambda target synchronously, so a handler that calls DescribeEndpoint blocks on the non-reentrant epMu, and every endpoint op hangs from then on. Repro in the Go library with WithAsyncSettle: a rule on EndpointStatus IN_SERVICE to a Lambda whose handler calls p.SageMaker.DescribeEndpoint(ctx, "ep"), advance the clock, call DescribeEndpoint: the handler fires and the call never returns. Could the observer just record that the event is due, and the callers publish it after epMu.Unlock() with their ctx (as CreateEndpoint/UpdateEndpoint/DeleteEndpoint already do)? A test like lifecycle_events_reentry_test.go would pin it.
There was a problem hiding this comment.
Reproduced with a Lambda target on IN_SERVICE whose handler calls DescribeEndpoint, and fixed. The observer no longer publishes: observeEndpoint only queues the settled event (recordEndpointSettled), and every endpoint entry point that holds epMu now releases it through unlockEndpoints(ctx), which publishes the queued events after the unlock. Pinned by TestSageMakerEndpointSettledEventHandlerReentersWithoutDeadlock in lifecycle_events_reentry_test.go: for each of DescribeEndpoint, ListEndpoints, UpdateEndpoint, InvokeEndpoint and AddTags, a fresh settled endpoint is read first by that path while the handler reads the endpoint back; it was red (4 paths hit the 10s deadlock timeout) before the change and is green now. Job events observe without a lock, as you noted, so they are unchanged. Fixed in 71dff3e.
|
|
||
| tr.settledPublished = true | ||
|
|
||
| m.emitEndpointEvents(context.Background(), settled, driver.EndpointInService) |
There was a problem hiding this comment.
context.Background() drops the caller's ctx. Once this moves outside the lock, passing the request ctx through matches the other emit paths and what the re-entry tests expect from handlers.
There was a problem hiding this comment.
Done with the move above: the events are published with the caller's ctx wherever the entry point has one (Create, Describe, List, Update, UpdateEndpointWeightsAndCapacities, Delete, InvokeEndpoint and InvokeEndpointAsync). Two paths have no ctx to pass today and use context.Background(): the tag liveness check and the Search endpoint list; the non-goals note is unchanged because they only publish an event that was due anyway. Fixed in 71dff3e.
|
|
||
| return out, nil | ||
| //nolint:dupl,gocritic // per-resource shim mirrors the SDK shape; large value params mirror the driver interface | ||
| func (m *Mock) ListUserProfiles(_ context.Context, domainID string, opts driver.ListOptions) (driver.ListPage[driver.UserProfile], error) { |
There was a problem hiding this comment.
ListUserProfiles (and ListSpaces at line 212) document SortBy CreationTime | LastModifiedTime, but there's no modified accessor, so --sort-by LastModifiedTime returns ValidationException 'not supported by this operation'. Same issue as the lifecycle config / code repository lists, but UserProfile and Space don't carry a LastModifiedTime yet.
There was a problem hiding this comment.
driver.UserProfile and driver.Space now carry LastModifiedTime (equal to the creation time until an update operation exists), both lists have the accessor, and Describe and the summaries return it. --sort-by LastModifiedTime works on both (checked with the CLI) and old snapshots fall back to the creation time. Fixed in 71dff3e.
| @@ -73,24 +133,28 @@ type CodeRepository struct { | |||
| Branch string | |||
| SecretARN string | |||
| CreationTime string | |||
| // LastModifiedTime equals CreationTime: there is no UpdateCodeRepository. | |||
There was a problem hiding this comment.
AWS does have UpdateCodeRepository (the emulator just doesn't implement it yet, so a Terraform aws_sagemaker_code_repository update gets UnknownOperationException). Could the comment say it isn't implemented yet rather than that it doesn't exist?
There was a problem hiding this comment.
Reworded: "UpdateCodeRepository is not implemented yet". The same note is on the non-goals page, together with UpdateUserProfile and UpdateSpace, and it says a Terraform aws_sagemaker_code_repository update gets UnknownOperationException. Fixed in 71dff3e.
| InitialVariantWeight float64 | ||
| // InitialVariantWeight is nil when the caller omits it; the provider applies | ||
| // SageMaker's default of 1.0, so the Go library and the server agree. | ||
| InitialVariantWeight *float64 |
There was a problem hiding this comment.
Right shape, but float64 to *float64 on a public driver type breaks Go library callers that set InitialVariantWeight: 1. Worth a line in the release notes.
There was a problem hiding this comment.
Agreed. The non-goals page and the PR description now carry a "Library API change" line for InitialVariantWeight going from float64 to *float64 (the repo has no changelog file, so that is where release notes are drawn from). Fixed in 71dff3e.
| return false | ||
| } | ||
|
|
||
| m.settledEmitted[key] = struct{}{} |
There was a problem hiding this comment.
Keys are added here for every settled job state and never removed, so this grows for the life of the server. A flag on the job record (like settledPublished on the endpoint transition) would avoid the side map.
There was a problem hiding this comment.
Kept the side map for now and documented it: it is one small entry per settled job (a job has at most a few settled states) for the life of the process, and moving the flag onto the job record would add a bookkeeping field to the public driver job types. Happy to do that as a follow-up if you prefer; it is on the follow-up list on the non-goals page.
…to feat/sagemaker-parity # Conflicts: # internal/settle/settle.go
Publishing IN_SERVICE from inside the endpoint observer ran an event target (a Lambda that reads the endpoint back, for example) while epMu was held, so it deadlocked every endpoint operation. The observer now only queues the event, and each endpoint entry point publishes it with the caller's context after releasing the lock. Also accept LastModifiedTime as a sort and filter on user profiles and spaces (the types now carry the field), omit unset training start and end times from state-change events instead of sending "0", correct the UpdateCodeRepository driver comment, and document the remaining follow-ups and the InitialVariantWeight API change.
|
Thanks for catching the deadlock; that one was mine. Round 2 is in 71dff3e:
Verified with the full |
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Thanks, this round fixes the deadlock. I re-checked 71dff3e with the Go library, the AWS CLI and the Terraform AWS provider (6.66.0), with and without --async-settle, and across a --persist restart. The settled IN_SERVICE event no longer runs under the endpoint lock, the new test fails on the previous head and passes now, and nothing from the earlier rounds has regressed. I found two small things, both Low. I'm happy for this to go in.
Status of the round-2 findings
| Round-2 item | Status | Evidence |
|---|---|---|
High: settled IN_SERVICE published under epMu |
Fixed | The event is queued under the lock and published by unlockEndpoints after epMu.Unlock(), with the caller's ctx. The new re-entry test hangs on d415690 (all five read paths time out) and passes on 71dff3e. I also ran Lambda handlers that call UpdateEndpoint, DeleteEndpoint, UpdateEndpointWeightsAndCapacities and ListEndpoints from the IN_SERVICE event. None hang, each handler fires once, and the endpoint stays usable afterwards. With 16 concurrent Describe/List/Invoke/AddTags callers after settle, IN_SERVICE is delivered exactly once in 50 out of 50 runs under -race. See Low 1 for ordering |
L1: "0" training event times, DeleteWorkteam |
Fixed / documented | An InProgress training event now has no TrainingStartTime or TrainingEndTime, and the Completed event has both. DeleteWorkteam on a missing team is on the follow-up list |
| L2: ListUserProfiles and ListSpaces reject LastModifiedTime | Fixed | --sort-by LastModifiedTime (both orders) and LastModifiedTimeAfter/Before work on both ops. Describe and List now return LastModifiedTime |
| L3: model-failure state reachable only from tests | Documented | The non-goals page now says the 424 / Invocation5XXErrors branch runs only from Go tests. The code is unchanged, which is fine for a follow-up |
L4: InitialVariantWeight to *float64 |
Documented | It's on the coverage page. It still needs a line in the release notes when this ships |
| L5: wrong UpdateCodeRepository comment | Fixed | The comment now says the op isn't implemented yet. The non-goals page lists UpdateCodeRepository, UpdateUserProfile and UpdateSpace as missing |
L6: settledEmitted only grows |
Documented | Listed as a follow-up |
| Coverage | Not changed | providers/aws/sagemaker 91.9%, services/sagemaker 98.5%, server/aws/sagemaker still 73.3% |
Findings
High
None.
Medium
None.
Low
1. A settled IN_SERVICE event can arrive after the UPDATING or DELETING event that follows it. (providers/aws/sagemaker/endpoint.go:220-231)
unlockEndpoints releases epMu, then takes the queue, then publishes. If a read notices the settle and an UpdateEndpoint or DeleteEndpoint runs right after it, the mutation can lock, finish and publish UPDATING or DELETING before the reader gets to publish IN_SERVICE. I raced one DescribeEndpoint against one UpdateEndpoint right after settle, 400 times each. 16 runs delivered [UPDATING IN_SERVICE], and racing against DeleteEndpoint, 30 runs delivered [DELETING IN_SERVICE]. IN_SERVICE was still delivered exactly once every time. A related effect: whichever caller unlocks next flushes the whole queue, so an event noticed by one request can be published on another request's ctx.
EventBridge doesn't promise ordering, so this is minor. But a target that keys off the latest event would see an endpoint marked in service after it was deleted. Before this commit the ordering was strict, because the event went out under the lock.
Fix, if you want it: let one flusher drain the queue at a time. Push the call-time CREATING, UPDATING and DELETING events through the same queue under epMu. In unlockEndpoints, a caller that finds a flush already running just returns, and the running flusher loops until the queue is empty. A re-entrant handler then can't deadlock, since its unlock sees the flush in progress and returns. Otherwise, a sentence on the non-goals page that endpoint events can arrive out of order under concurrent calls would cover it.
2. modifiedOrCreated is defined twice (providers/aws/sagemaker/list.go:262 and server/aws/sagemaker/notebook.go:367). The provider already applies the fallback for list sorting. If it also filled LastModifiedTime from CreationTime when it returns a code repository, user profile or space, the wire handler could emit the field as stored and the second copy could go.
AWS docs divergences
None new. UpdateCodeRepository, UpdateUserProfile and UpdateSpace (UpdateCodeRepository, UpdateUserProfile, UpdateSpace) are still unimplemented and are now listed as follow-ups.
Terraform and CLI results
Terraform (hashicorp/aws 6.66.0) with aws_sagemaker_model, aws_sagemaker_endpoint_configuration, aws_sagemaker_endpoint and aws_sagemaker_notebook_instance:
- Default server: apply added 4 in 6s, plan 0. An update to the notebook
instance_type, every tag and the variant weight gave 1 added, 3 changed, 1 destroyed. The endpoint is InService on the new config with CurrentWeight 2, and the notebook is InService onml.t3.large. Plan 0 afterwards, destroy removed 4, and Describe on the model, endpoint and notebook returns not found. --async-settle --persistserver: the same sequence (apply 14s, no waiter hang), with the same results.--async-settle --persistwith a restart between apply and plan: plan 0 after the restart, the endpoint is InService, and destroy is clean.
AWS CLI (--async-settle --persist):
- EventBridge rule to SQS, then 12 parallel
describe-endpoint/list-endpointscalls after settle, then update and delete. The events wereCREATING, IN_SERVICE, UPDATING, IN_SERVICE, DELETING, with one IN_SERVICE per transition. - Earlier repros, all still fixed:
- Duplicate training job returns ResourceInUse, and duplicate model returns ValidationException.
list-modelsis newest first.invoke-endpoint-async --inference-id myid-1returnss3://b/async/myid-1.out.- CreateModelPackage with a group ARN creates
model-package/grp1/1. - An endpoint deleted under
--async-settlestays deleted across a restart.
- New in this round:
- ListUserProfiles and ListSpaces with
--sort-by LastModifiedTime. - DescribeUserProfile and DescribeSpace
LastModifiedTime. - Training-job event times.
- ListUserProfiles and ListSpaces with
What I verified
- Clean build of 71dff3e.
go veton the touched packages.gofmtclean.go mod tidyleaves no diff.go generate ./...leaves no diff.- golangci-lint reports 0 new issues against d415690 and against the merge base, on the SageMaker packages and
providers/aws. go test -racepasses onproviders/aws/sagemaker,server/aws/sagemaker,services/sagemaker,providers/awsandpersist. The SageMaker, authorization and golden tests inserver/awspass, and so do the root SageMaker tests.- The re-entry and concurrency probes above ran three times under
-race. No races and no hangs. - No AI attribution in the new commit or the PR body.
- CI is all green, and the branch merges cleanly.
Verdict
Approve. The deadlock is fixed and pinned by a test, every earlier High and Medium is still fixed, and the two Lows can go in a follow-up.
| m.epDue = nil | ||
| m.epDueMu.Unlock() | ||
|
|
||
| for _, ep := range due { |
There was a problem hiding this comment.
Once epMu is released, a mutation can get in before this loop runs. Racing one DescribeEndpoint against one UpdateEndpoint (or DeleteEndpoint) right after settle, about 4-8% of 400 runs delivered [UPDATING IN_SERVICE] or [DELETING IN_SERVICE]. IN_SERVICE was still sent exactly once every time. The queue is also drained by whichever caller unlocks next, so an event can go out on another request's ctx. EventBridge doesn't promise order, so this is minor. If you want strict order back: route CREATING/UPDATING/DELETING through the same queue under epMu, and have a single flusher drain it (a caller that finds a flush running just returns, which also keeps re-entrant handlers safe). Otherwise a sentence on the non-goals page would cover it.
|
|
||
| // modifiedOrCreated is a resource's LastModifiedTime, falling back to its | ||
| // creation time for state restored from a snapshot that predates the field. | ||
| func modifiedOrCreated(modified, created string) string { |
There was a problem hiding this comment.
This duplicates modifiedOrCreated in providers/aws/sagemaker/list.go. If the provider filled LastModifiedTime from CreationTime when it returns a restored code repository, user profile or space, the handler could emit the stored value and this copy could go.
2 participants
Summary
The problem (in plain words)
The SageMaker emulator diverged from the real service in 20 places (#1450): whole operation families were missing, Describe/List/endpoint/cluster responses were thin, jobs only ever showed a terminal state, two CloudWatch metrics were invented, and no EventBridge events were published. Anyone running the AWS SDK, CLI or Terraform against the emulator hit errors or perpetual diffs.
How this fixes it
The PR adds the missing families and fills in the response shapes so they match what the real service returns, replaces the invented metrics with the real ones, and publishes the documented state-change events. Details follow.
JobsCreated/ResourcesCreatedmetrics and no EventBridge events.ProductionVariantsand Creating/Updating states, full List filters/sort/paging, distinct job stage times, real invoke and job-utilization metrics, and state-change events on the default bus.Changes
ProductionVariants(+ Creating/Updating under--async-settle); clusters report InstanceGroup counts/status; job Describe echoes the create request and returns distinctSecondaryStatusTransitions; List operations take the real filters, sort andMaxResults/NextToken(default page 10, max 100), with the documented defaults (SortOrderDescending for ListEndpoints, ListModels, ListEndpointConfigs and ListTransformJobs;SortByName for ListCodeRepositories and ListNotebookInstances), and a filter on a field the operation lacks is an error, never an empty result; InvokeEndpoint honors Content-Type/Accept/TargetVariant.JobsCreated/ResourcesCreated; addedInvocations,InvocationsPerInstance,ModelLatency/OverheadLatency(Microseconds) withEndpointName+VariantName, 4XX/5XX/ModelError counters, per-job host utilization and pipeline metrics.aws.sagemakerstate-change events for training/processing/transform/tuning jobs, endpoints, model packages and pipeline executions.docs/services.md, addeddocs/coverage/nongoals/sagemaker.md.Behavior changes to existing behavior (deliberate, real-service aligned): a duplicate create is a
ValidationExceptionfor CreateModel, CreateEndpointConfig, CreateEndpoint and the other operations whose SDK error list has noResourceInUse, and staysResourceInUsefor jobs, feature groups, Studio user profiles, spaces and apps, clusters and the other operations that model it; Describe/Delete of a missing target is aValidationExceptionfor operations whose SDK output does not modelResourceNotFound; InvokeEndpoint on a missing endpoint is a 400ValidationError; Stop on a terminal job is a no-op 200; List order is deterministic with a default page of 10; deleting a model package group cascades to its versions; error messages no longer carry the internalNotFound:/InvalidArgument:prefix;CreateEndpointConfigrejects a variant whose model does not exist, more than 10 variants and negative counts or weights;driver.ProductionVariant.InitialVariantWeightis now a*float64(nil means omitted; the provider applies the default of 1.0), so Go library callers that wroteInitialVariantWeight: 1must pass a pointer;InvokeEndpointAsyncneeds an endpoint configured with an async output path and returns the result under it; every create enforces the 50-tag limit; a notebook instance must be Stopped (or Failed) before it is deleted; a domain with user profiles, spaces or apps cannot be deleted; deleting a pipeline deletes its executions;StopPipelineExecutionon a finished run is a no-op;DeleteWorkforcewith workteams is aValidationException(the SDK op models no typed error); jobInstanceCountis capped at 500;UpdateEndpointand theList*driver signatures changed (Go API); two goldens inserver/aws/testdata/authz_authoff_golden.jsonwere updated for the message/status changes.Deliberate deviations / non-goals (also in
docs/coverage/nongoals/sagemaker.md): nothing runs (no containers);InvokeEndpointWithResponseStreamandBatchGetRecordare not served;ListMonitoringAlertsis empty underserve(AWS creates alerts from Model Dashboard violations; the seeding hook is a Go test seam); no Step Status Change, Model, Feature Group or HyperPod events, and under--async-settlethe first state of a change is published at the call and the settled state once, on the first read after it settles (published after the endpoint lock is released, so an event target can read the endpoint back); the endpoint event status literal follows the AWS doc example (IN_SERVICE), which AWS does not document further; per-job utilization is skipped under--async-settle; some caps (500 projects, 25 workteams) and message texts are the emulator's choice. I walked the 15-point service checklist in the repo standards against this diff.Provider Coverage
Azure, GCP and OCI are not applicable: SageMaker is an AWS-only service.
Checklist
go test ./...)golangci-lint run --timeout=9m ./...)cloudemu_test.go- SDK round-trip tests added inserver/aws/sagemaker, event tests inproviders/aws/lifecycle_events_test.gogo generate ./...for coverage;go run ./internal/compatgenfor the compat matrix) and committed the resultTest Plan
ModelPackageName/AsyncInferenceConfig/InferenceIdwire gaps, an unboundedInstanceCountallocation, a reflected-content risk in the invoke echo, an OIDC secret kept in snapshots, unbounded control-plane bodies, and missing SDK round-trip, wrapper and integration tests.go test ./...green,-raceon the touched packages.-racebinary (0 orphans, no data race),--persistrestart, a snapshot written by thedevelopmentbinary loading in this branch,--enforce-authallow/deny, and a Terraform apply / no-diff plan / in-place update / destroy of 10 SageMaker resources. The E2E found and fixed: baseline-snapshot endpoints losing their variants, Terraform's not-found detection (notebook, project, workforce, image) and a missingDirectInternetAccess.providers/aws/sagemaker50.3% -> 89.0%,server/aws/sagemaker40.3% -> 74.9%;services/sagemaker13.7% -> 98.8%.contrib/dockerengine,contrib/realengine,internal/vtl,server/serverkit); none in the SageMaker code.Related Issues
Closes #1450