Function code + configuration updates applied in one change are no longer left partially unapplied - #237
Conversation
- Force requeue when both code and configuration are changed for a Function - Add regression test that validates that a single spec update that includes both code and configuration changes converges.
|
Skipping CI for Draft Pull Request. |
ack-prow
Bot
added
the
do-not-merge/work-in-progress
|
/test all |
ack-prow
Bot
removed
the
do-not-merge/work-in-progress
|
/hold |
ack-prow
Bot
added
the
do-not-merge/hold
| return updatedStatusResource, err | ||
| } | ||
| return rm.concreteResource(readOneLatest), nil | ||
| updated := rm.concreteResource(readOneLatest) |
There was a problem hiding this comment.
returning readOneLatest might be an issue here. I see that we're doing a ReadOne during the update. Can we avoid doing that?
There was a problem hiding this comment.
Hmm this PR doesn't add that ReadOne, but agree it isn't ideal. I've gone ahead and removed the ReadOne call. To retain a fresh post-update status read I've made the post-update requeue unconditional to force a re-read of the Function's status. Performing exploratory testing with Function updates it doesn't look like this was being used to suppress AWS defaults causing reconcile loops so I don't think we need to mark any additional fields as late_initialize.
knottnt
added
the
release/patch
- Remove call to rm.ReadOne(ctx, desired) - Unconditionally force a requeue after field updates to pick-up fresh status and reconcile any deferred updates.
|
/retest |
- Add wait for synced state before checking post-create status fields to avoid race between resource creation and assertion.
5c5463a to
4f36108
Compare
|
/lgtm |
ack-prow
Bot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knottnt, michaelhtm 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: * #237
2 participants
Issue #, if available: 1968
Description of changes:
Problem
When a single kubectl apply updates both a Function's code (spec.code.sha256 / architectures / code.imageURI) and its configuration (e.g. spec.environment), only the code change was applied. The configuration change was silently dropped until the next full resync (~10h) or the next manual spec edit, even though the resource reported ACK.ResourceSynced=True.
Root cause
customUpdateFunction intentionally issues either UpdateFunctionCode or UpdateFunctionConfiguration in a single reconcile, never both, because either async call puts the function into an updating state and Lambda rejects a second concurrent update. After issuing the code update the Function's status is read as Active which is considered a synced state by the controller. As a result the controller failed to requeue to apply the configuration updates and instead waited the default re-sync duration.
Fix
Derive whether both a code and a configuration change were requested directly from the delta passed into customUpdateFunction, and requeue only in that combined case:
Code-only, config-only, and architectures-only updates each apply everything in a single call and do not requeue, so their behavior is unchanged.
Fix is similar to work done in #151
Testing
Added e2e regression test test_function_update_code_and_environment_variable, which patches code.sha256 + code.s3Key + environment in one update and waits on ACK.ResourceSynced=True (long enough to span the 30s requeue) before asserting both the new CodeSha256 and the environment variables are applied. This test fails on main (env var dropped) and passes with the fix. Test is based on #204 work done by @willthames
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.