Ensure deplyoment await logic uses the latest deployment object

[rquitales]

Proposed changes

This PR ensures that we compare deployment revisions based on the old live object, and the most current deployment object.

This PR does the following:

• Add a repro test that fails before the change is applied
• Only watch for deployment events until the deployment object has been reconciled by the deployment controller
• Update existing unit tests to account for blocking unbuffered channels, and faked objects that did not have .status.observedGeneration

Related issues (optional)

Fixes: #2941

[github-actions]

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

[codecov]

Codecov Report

Attention: Patch coverage is 33.33333% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 29.16%. Comparing base (78ab387) to head (2ef5470).

Files	Patch %	Lines
provider/pkg/await/deployment.go	33.33%	14 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2943      +/-   ##
==========================================
+ Coverage   27.63%   29.16%   +1.52%     
==========================================
  Files          54       61       +7     
  Lines        7862     8194     +332     
==========================================
+ Hits         2173     2390     +217     
- Misses       5504     5600      +96     
- Partials      185      204      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[blampe]

I'm a little worried this introduces room for more race conditions on dia, because we can still be handling multiple events at once. Re: not blocking the main loop, do you have an idea of where we're getting bottlenecked?

I wonder if there's an alternative where we simply ignore replicaset events until we see a deployment event with a generation change. Something like:

select {
case event := <-deploymentEvents:
    deploying := dia.processDeploymentEvent(event)
case event := <-replicaSetEvents:
    if deploying {
        dia.processReplicaSetEvent(event)
    }
}

[rquitales]

Yes, the initial approach in this PR may be risky. We also shouldn't be discarding replicaset events since we need those events to inform us of status updates so I'm hesitant to go down the approach you suggested.

I've reworked this PR so that we continuously poll for deployment events first, and ensure that the deployment controller has informed us if a rollout was triggered or not before we proceed onto processing other events. This ensures that when we check the replicaset, the deployment controller would have informed us if a roll-out has been triggered.

[rquitales]

note: the unit test had the incorrect observedGeneration for this object.

[rquitales]

The event is reordered since we're using unbuffered channels which will block until there is a value receiver.

[EronWright]

LGTM

[blampe]

Would generation <= observedGeneration be technically safer here? I don't know if that's actually observable in practice.

[rquitales]

We need the observedGeneration to match exactly the generation of the deployment object so that the status matches the right manifest spec. generation != observedGeneration is currently why this bug exists, though it would be a bug in k8s if generation < observeredGeneration.

[EronWright]

I too stared at the comparison for a few minutes. Rollout code will sometimes have a 'planned' generation that they got from the call to Apply, and await an observed generation that is greater-or-equal to the planned generation. This code is not doing that; it is simply comparing generations within a given object and has no planned generation.

[blampe]

👍