Groom conditions LastTransitionTime in postprocess #1403
Conversation
knative-prow-robot
added
the
size/M
| // groomConditionsTransitionTime ensures that the LastTransitionTime only advances for resources | ||
| // where the condition has changed during reconciliation. This also ensures that all advanced | ||
| // conditions share the same timestamp. | ||
| func groomConditionsTransitionTime(resource, oldResource duckv1.KRShaped) { |
There was a problem hiding this comment.
🤔 maybe I'm misunderstanding, but iiuc the entire point of VolatileTime is to avoid what this is doing 😅
There was a problem hiding this comment.
The VolatileTime strategy has a few weaknesses I've outlined in #1394
- Requires use of k8s specific comparison libraries
- The snapshot of now() is taken multiple times during reconciliation, but all written back at the same time. This can make it hard to tell which conditions are changed in a single reconciliation step (although this is optional, we could update them as we go).
- It fails to keep the LastTransitionTime stable if a single reconciliation sets conditions more than once (eg. flip-flopping but winds up in the same place).
This obviates the VolatileTime and condition manager strategy so that implementers don't really need to worry about how they are modifying the status struct - it just works.
There was a problem hiding this comment.
Let me comment on the issue, but it may be worth discussing more synchronous Weds (put on the agenda for API WG?).
There was a problem hiding this comment.
Yea let's just discuss in WG - that'll be easiest
knative-prow-robot
added
the
needs-rebase
knative-prow-robot
removed
the
needs-rebase
|
/assign @mattmoor @vagababov |
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
Yeah, this breaks forward edges, so it'll have to land in a few waves downstream. Thanks @Cynocracy for doing this for networking 😍 |
|
To clarify: manual changes aren't needed, but the sequencing of updates will need to follow the graph, so it'll likely take a couple applications of the automation before things are running smoothly again. |
|
Gotcha, I made a manual change in networking and am now poking around to
see why it doesn't seem to regenerate on its own.
…On Thu, Jun 18, 2020 at 9:17 PM Matt Moore ***@***.***> wrote:
To clarify: manual changes aren't needed, but the sequencing of updates
will need to follow the graph, so it'll likely take a couple applications
of the automation before things are running smoothly again.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1403 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAICPOW652OAR62SIWDCIEDRXLREHANCNFSM4N4WH3KA>
.
|
|
Ended up being #1287, I have a
potential fix that is very much in the spirit of the /hack/.
On Thu, Jun 18, 2020 at 10:06 PM Jonathan Donovan <jmd377@cornell.edu>
wrote:
… Gotcha, I made a manual change in networking and am now poking around to
see why it doesn't seem to regenerate on its own.
On Thu, Jun 18, 2020 at 9:17 PM Matt Moore ***@***.***>
wrote:
> To clarify: manual changes aren't needed, but the sequencing of updates
> will need to follow the graph, so it'll likely take a couple applications
> of the automation before things are running smoothly again.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1403 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAICPOW652OAR62SIWDCIEDRXLREHANCNFSM4N4WH3KA>
> .
>
|
Fixes #1394