Conversation
|
Images are ready for the commit at 87be64a. To use with deploy scripts, first |
for filtering out changes to deployment specs.
d2c0d1d to
87be64a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19194 +/- ##
==========================================
- Coverage 49.56% 49.55% -0.01%
==========================================
Files 2675 2675
Lines 201820 201859 +39
==========================================
+ Hits 100033 100040 +7
- Misses 94332 94357 +25
- Partials 7455 7462 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Images are ready for the commit at 9dec3f8. To use with deploy scripts, first |
to also work for unstructured.Unstructured.
| conditionsChanged(objOldT, objNewT, platform.ConditionAvailable) | ||
|
|
||
| return !statusControllerConditionsChanged | ||
| if statusControllerConditionsChanged { |
There was a problem hiding this comment.
Now that I think about it, it seems that this condition was and is wrong. We should be skipping reconciliation, not simply when these two conditions changed, but instead if nothing else but these two conditions changed. (Well, modulo the resource version.)
I mean, I don't know whether the system cannot guarantee that individual updates are always processed separately. What if we see two combined changes together in a single event, due to a temporary disconnection from API server? Then this line would make us miss an update we potentially care about.
Okay, maybe this is not the best example and we could assume that if there's a disconnection then there's an unconditional reconcile after cache is reset, rather than an update event, so this code always works in practice. But I don't think we should be making such assumptions here. We should keep things simple and not add implicit coupling.
WDYT?
There was a problem hiding this comment.
You are right in that this code is based on the assumption that the API server does not do any implicit batching of events. Back when I wrote it, I was convinced that this assumption holds...
Description
TwoThree fixes:spec.replicasfor deployments in case auto-scaling is enabled.Updateevents forDeploymentsfor the status controller unless they really change theStatussubresource. If they just change thespec(e.g. setting the number of replicas) we don't let the status controller react to it.SkipStatusControllerUpdatespredicate to work properly forunstructured.Unstructured.User-facing documentation
Not needed.
Testing and quality
Automated testing
How I validated my change
change me!