Revert FlinkBlueGreenDeployment lastReconciledSpec back to old spec during abort workflow - #26
Conversation
| // Read the active child's spec and write it back to the B/G CR so | ||
| // lastReconciledSpec stays consistent and the next reconciliation sees the | ||
| // correct diff. | ||
| BlueGreenDeploymentType activeType = |
There was a problem hiding this comment.
thinking out loud here, what if users modified the child spec prior to the abort, the B/G CR now holds something slightly different than the true lastreconciledSpec right? I guess does that matter?
There was a problem hiding this comment.
Hmm if the B/G controller overwrites the child spec on every reconciliation, that manual spec change would be overwritten anyway, right?
There was a problem hiding this comment.
Actually, you're right - the period of time before the next deploy and when this code runs, if someone changed the child spec manually, then there would be drift.
Two options:
- we could save the previous lastReconciledSpec before overwriting it
- we could only revert lastReconciledSpec (from the in-memory map), which means lastReconciledSpec (good) differs from CR spec (bad) → operator detects a diff → tries the transition again → fails → aborts → retries. The user sees the error on every cycle and needs to fix their spec manually.
Maybe we should go with 1?
There was a problem hiding this comment.
we discussed offline but option 1!
| : BlueGreenDeploymentType.GREEN; | ||
| FlinkDeployment activeChild = context.getDeploymentByType(activeType); | ||
| if (activeChild != null) { | ||
| context.getBgDeployment().getSpec().getTemplate().setSpec(activeChild.getSpec()); |
There was a problem hiding this comment.
we've confirmed that this will reflect the correct value in the spec?
be62410 to
7e9ebe9
Compare
|
|
||
| // Restore lastReconciledSpec and the B/G CR spec to the pre-transition state | ||
| // so they stay consistent with the active child that is still running. | ||
| String namespace = context.getBgDeployment().getMetadata().getNamespace(); |
There was a problem hiding this comment.
call out: if the BG gets deleted or we exit somewhere during the transition after failure, or without failure we don't seem to remove the entry in the previousReconciledSpecs at that point. Is that a risk? it's only of size 1 per namespace but do we want to keep it around, is there any chance we could inadvertently end up using the value in there resulting in an improper restore somehow? (i.e. we end up using the stale one to restore)
There was a problem hiding this comment.
it's not a risk because savePreTransitionSpec always overwrites the entry for that namespace before each transition. By the time abortDeployment runs, the map has the correct pre-transition spec for the current transition, not a stale one from a previous transition.
There was a problem hiding this comment.
right beginning of each round will wipe it out. okay perfect, thanks for talking through these with me!
7e9ebe9 to
7c594f9
Compare
|
Prior to merging, please make sure to run through a few more edge case scenarios in staging! also release this on monday! as friday isn't a good time for these to roll out , we want users to be around and be aware of these patches rolling out! |
2 participants
Simply setting the lastReconciledSpec after calling startTransition wasn't enough, because the workflow could still abort after that, so add the reverting logic in the abort workflow.
Fixes Shopify/streaming-compute#882