feat(statemachine): publish a version on change via spec.publish - #122
ack-prow[bot] merged 1 commit into
Conversation
8299647 to
bf2ed53
Compare
|
/label release/minor |
ack-prow
Bot
added
the
release/minor
knottnt
left a comment
knottnt
left a comment
There was a problem hiding this comment.
@gustavodiaz7722 overall looks good, just had a note about the behavior when flipping spec.publish from true -> false.
| Setting this back to false stops future versions being cut but does not delete | ||
| any that already exist, and leaves stateMachineVersionARN naming the last one |
There was a problem hiding this comment.
and leaves stateMachineVersionARN naming the last one
Maybe worth noting that the next call to UpdateStateMachine with spec.publish:false will clear status.stateMachineVersionARN. For the most part this is desirable since after the update stateMachineVersionARN will no longer reflect the desired state. However, doing some exploratory testing I did note that there's a pre-existing issue where the controller will repeatedly detect a delta for LoggingConfiguration, TracingConfiguration, and Type when they are not set at creation time. When an update is triggered due to these erroneous (nil -> default) deltas it doesn't look like the actual StateMachine is mutated in AWS and no new version is published, but a "settled" StateMachine with spec.published:false would unset status.stateMachineVersionARN.
Might be worth making sure those fields are late initialized to avoid spurious deltas or at least creating a new issue to track.
There was a problem hiding this comment.
I created a tracking issue for now aws-controllers-k8s/community#3035
I'd lean towards fixing this outside scope of this PR. The delta does not cause sfn to publish new versions, since the update doesn't actual contain any changes. The published version would remain static even with spurious updates. If you think otherwise, I would also be okay with fixing it in this PR, don't have a strong preference.
|
/label release/minor |
7c01891 to
38def67
Compare
StateMachine could not publish versions at all: CreateStateMachineInput.Publish
and CreateStateMachineOutput.StateMachineVersionArn were both ignored, so a user
had no way to opt into versioning and no way to learn the ARN of a version once
one existed.
Surface Publish as a spec field and StateMachineVersionArn as status. Publishing
is a parameter of Create/Update rather than a separate call, so a version is cut
whenever the update payload differs from the current revision. AWS deduplicates
per revision, so repeat reconciles of an unchanged spec cut nothing.
newUpdateRequestPayload maps Spec.Publish onto UpdateStateMachineInput.Publish.
That builder is a manual copy of generated code -- update_operation carries a
custom_method_name, so code generation emits only a thin sdkUpdate wrapper and
never regenerates it -- which is noted on the function, because a Spec field
that reaches the Create payload automatically is silently absent from Update.
Publish is excluded from the generated comparison: publish modifies a request,
it is not state of the state machine, so its value changing is not by itself a
reason to call Update.
That leaves one case the payload comparison cannot see. Turning publish on for a
state machine that is already settled produces no spec difference, so nothing
would ever be published. Status.StateMachineVersionARN carries that signal
instead of a dedicated flag: it names the version holding the configuration last
pushed, so a nil ARN under Spec.Publish means no version holds it. The read path
clears the ARN when the version it names is gone, and customPreCompare turns a
nil ARN into a Spec.Publish delta -- registered under a Spec path because the
runtime only calls Update when delta.DifferentAt("Spec") holds. One condition
covers both a state machine that has never published and one whose version was
deleted out of band. An update with an unchanged payload does publish when no
version exists for the current revision, so the ARN is recorded again on the
next read and the resource settles.
The probe behind that is confined to clearDeletedVersionARN. A missing version
answers with StateMachineDoesNotExist, the code this resource maps to 404, so
allowing it to escape would make the read report the state machine itself as
gone. Only that code clears the ARN; other failures leave it alone rather than
manufacturing a publish. The probe is skipped entirely when publish is false or
no version is recorded, which keeps the extra read off the path of users who do
not version.
customUpdateStateMachine keeps the update response in order to record the version
ARN, replacing a discarded response and a copy that was built and dropped. The
ARN mirrors the response including its absence, so an update that does not
publish clears it. Left stale, a later flip of Spec.Publish would find a version
that exists but no longer matches the state machine, and publish nothing.
Tests cover the field end to end: publish at create, publish absent, and a
definition change cutting exactly one further version. Two cases carry most of
the value. Enabling publish on a settled state machine has no spec difference to
act on, so without the delta derived from a missing version ARN it silently does
nothing. A version deleted outside ACK is republished, and because version
numbers are never reused the assertion is that the recorded ARN changes rather
than returning to its old value. Each case then forces another reconcile and
asserts no further version appears, since the failure mode of a delta that never
settles is publishing on every pass.
Recovery is triggered by patching spec.tags: the default resync period is ten
hours, so a change made only in AWS is not otherwise observed within a test run.
It has to be a spec change, because ACK filters watch events with
GenerationChangedPredicate unless the ignore-field-drift gate is on and
annotations do not bump metadata.generation. Tags are used because a tags-only
delta is synced and then short-circuits before UpdateStateMachine, so it cannot
publish a version by itself -- which is also what makes the "nothing further was
published" assertions mean something.
The version-listing helper pages ListStateMachineVersions by hand, since
botocore registers no paginator for that operation, and lets errors propagate
because callers assert on an empty list to mean no versions exist.
The probe records under its own operation type so it stays separable from the
resource's own read, which calls the same API, and treats a missing version as a
success: the error counter is keyed on the API name alone, so reporting the
expected 404 would make ordinary reads look broken.
documentation.yaml replaces the field description AWS supplies for Publish, which
describes only the create case. The field also governs updates, and setting it
back to false stops future versions without deleting existing ones or clearing
the recorded ARN.
Status.RevisionID surfaces DescribeStateMachine's revisionId, which the CRD did
not expose because it is absent from CreateStateMachineOutput. It is the
observable that makes publishing legible: a version is cut when the revision
changes, so the recorded version ARN and the revision it holds can be read
together. AWS also treats the revision as the idempotency key for publishing.
38def67 to
1a806a6
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gustavodiaz7722, knottnt The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Releasing changes: * #122
2 participants
Adds
spec.publishso a StateMachine can cut immutable versions.CreateStateMachineInput.PublishandCreateStateMachineOutput.StateMachineVersionArnwere both in the ignore list, so there was no way to opt into versioning and no way to learn a version's ARN once one existed.spec.publishstatus.stateMachineVersionARNstatus.revisionIDDescribeStateMachine)Behaviour
Publishing is a parameter of Create/Update rather than a separate call, so a version is cut whenever the update payload differs from the current revision.
PublishStateMachineVersionis idempotent per revision, so repeat reconciles of an unchanged spec cut nothing.publishis excluded from the generated comparison: it modifies a request, it is not state of the state machine, so its value changing is not by itself a reason to call Update.That leaves one case the comparison cannot see — turning
publishon for an already-settled state machine produces no spec difference.status.stateMachineVersionARNcarries that signal, since a nil ARN underpublish: truemeans no version holds the configuration last pushed. The read path clears the ARN when the version it names is gone, andcustomPreCompareturns a nil ARN into aSpec.Publishdelta, registered under aSpecpath because the runtime only calls Update whendelta.DifferentAt("Spec")holds. One condition covers both a state machine that has never published and one whose version was deleted out of band. The probe is confined toclearDeletedVersionARN, clears only onStateMachineDoesNotExist, and is skipped whenpublishis false.Why a field rather than a StateMachineVersion resource
#104 proposes the resource shape, and it is worth discussing these side by side rather than one in place of the other. Two reasons I went this way:
UpdateStateMachinecall that sets the definition, so it cannot hold a different revision. A separate CR publishes whatever revision is current when it reconciles, and ACK gives no ordering guarantee between independent CRs.CloudFormation's
AWS::StepFunctions::StateMachineVersionavoids both, but only because every property isUpdate requires: Replacement— one template block, replaced each deploy, which both sequences the publish and cleans up the old version. ACK has no replacement primitive, so that mechanism does not port.Known gap
No retention. Versions accumulate and AWS caps them at 1000 with manual deletion, so this wants a follow-up such as
keepLastVersionson the parent. Flagging rather than hiding it.Testing
E2E in
test/e2e/tests/test_state_machine.pycovers publish at create, publish absent, a definition change cutting exactly one further version, enablingpublishon an already-settled state machine, and republishing after a version is deleted outside ACK. Each case then forces a reconcile and asserts nothing further is published, since the failure mode of a delta that never settles is publishing on every pass. The nudge patchesspec.tags: ACK filters events withGenerationChangedPredicateso an annotation is never delivered, and a tags-only delta short-circuits beforeUpdateStateMachineso it cannot publish by itself.Confirmed against the live API: publish is idempotent per revision; an update with an identical payload and
publish: truedoes publish when no version exists for the current revision; a missing version returnsStateMachineDoesNotExist; version numbers are never reused after deletion.versionDescriptionis left out deliberately — it participates in the revision fingerprint but is unreadable from the state machine, so it needs its own comparison story.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.