conditions Utility to let specify the entire condition

[srirammageswaran8]

What would you like to be added (User Story)?

A new conditions utility to upsert conditions with the given LastTransistionTime and state fields.

Detailed Description

The LastTransistionTime definition Says

Last time the condition transitioned from one status to another.
This should be when the underlying condition changed. If that is not known, then using the time when
the API field changed is acceptable.

Would be better to have a utility in conditions to let it specify the entire condition and upsert the condition when the condition type already exists. The Utility should add / update the condition with the given state fields which include

• Type
• Status
• Reason
• Severity
• Message

And the LastTransistionTime specified in the new condition to add.

Anything else you would like to add?

The Utility may be used to track the transition of the state in any cloud provider / any other resources.

Label(s) to be applied

/kind feature One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels.

[sbueringer]

@srirammageswaran8 Would conditions.Set work for your use case? (https://github.com/kubernetes-sigs/cluster-api/blob/4f1637e59c48528f7a5c1a3116054dbc96216f8c/util/conditions/setter.go#L41)

[srirammageswaran8]

@sbueringer , Thanks for the followup.

TheSet function does not let update the LastTransistionTime if the condition already exists. If the condition exists and the state changed it updates LastTransistionTime with the current time if the state does not change it updates LastTransistionTime with the existing condition's LastTransistionTime.

[sbueringer]

What is your use case for changing LastTransistionTime in a case where it shouldn't be changed?

[vincepri]

Chatted with @srirammageswaran8 offline, the main use cases we've chatted about, which might or might not be a fit for conditions, is when a transition is known to be happened at a different time and the condition is observing when the state has changed.

For example, say "ec2 instance created" the timestamp can be gathered from the APIs

[srirammageswaran8]

Thanks @vincepri . Instead of using the observed value of the resource I was trying to get the actual time of the transition of the resource to the conditions.

[sbueringer]

Ah now I got it. I think that makes sense based on the godoc of the LastTransitionTime field

[sbueringer]

Would be good to hear @fabriziopandini's opinion

[sbueringer]

Maybe we can just change the existing Set function to always "accept" the LastTransitionTime when it is set on the condition that is passed in

[fabriziopandini]

If I can give my two cents, changing the LastTransitionTime could be surprising for users, and in fact the definition of this field explicitly call out that using then using the time when the API field changed is acceptable.

In fact, by a quick check, api machinery condition utils do not allow changing last transition time too (but it seems they do allow changes of reason, message and observed generation 🤔).

Considering that the new Cluster API condition utils I'm working on are going to relies on upstream condition utils and thus do not allow changing last transition time, frankly speaking I don't think we should make it possible to change lasttransition time in existing CAPI condition utils.

[sbueringer]

I think then we should fix our godoc. It seems to clearly state that it's preferred that the lastTransitionTime "should be when the underlying condition changed"

Although the upstream metav1.Condition has the same godoc comment:

    // lastTransitionTime is the last time the condition transitioned from one status to another.
    // This should be when the underlying condition changed.  If that is not known, then using the time when the API field changed is acceptable.

It's very confusing to me if the "should" case is the one that we don't support

[fabriziopandini]

To me the godoc make sense. I read it as:

When you set a condition (or transition it from one state to another), if you know when the underlying "element" changed use this info. Otherwise, use the time when your controller detects the underlying "element" is changed (time.now)

[sbueringer]

I'm reading

This should be when the underlying condition changed

as the lastTranistionTime should be the time when "whatever is underlying this condition" changes (e.g. some change in the infrastructure)

And then

If that is not known, then using the time when the API field changed is acceptable.

(i.e. the time when the condition field changes) is just meant as a fallback if the transition time of the "underlying condition" is not known

[fabriziopandini]

I took some additional time to compare existing conditions utils and future/upstream aligned conditions utils

Those are the diffs:

1. existing conditions utils only allow to set lastTranistionTime to an arbitrary value only when a condition does not exists but not when a condition already exists, while future/upstream aligned conditions utils always allow this (both on new and existing conditions)
2. existing conditions utils changes lastTranistionTime whenever something changes in the condition, while future/upstream aligned conditions utils are changing lastTranistionTime only when status change

Both seems bug to me, and by just fixing 1 also this issue should be addressed (but I would fix both) @sbueringer opinions

[sbueringer]

Sounds good to me. I would also fix both.

It's really confusing if the lastTransitionTime changes just because e.g. the message of a condition changes (and that the semantic of our lastTransitionTime differs from the one upstream when folks use our condition utils)

[fabriziopandini]

/help

[k8s-ci-robot]

@fabriziopandini: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/11033): >/help Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.

[sbueringer]

cc @srirammageswaran8 @vincepri ^^ (in case you are interested to implement it)

[Karthik-K-N]

So the fix for this issue would be updating the Set() method

I took some additional time to compare existing conditions utils and future/upstream aligned conditions utils

Those are the diffs:

1. existing conditions utils only allow to set lastTranistionTime to an arbitrary value only when a condition does not exists but not when a condition already exists, while future/upstream aligned conditions utils always allow this (both on new and existing conditions)

2. existing conditions utils changes lastTranistionTime whenever something changes in the condition, while future/upstream aligned  conditions utils are changing lastTranistionTime only when status change

Both seems bug to me, and by just fixing 1 also this issue should be addressed (but I would fix both) @sbueringer opinions

So fix for this issue is to fix these two in existing condition utility or are you looking for something else as well.

[fabriziopandini]

Both changes should be implemented in https://github.com/fabriziopandini/cluster-api/blob/a8ae016c1800887ecd0683ac2e4720e6f9626d73/util/conditions/setter.go#L41-L78

[Karthik-K-N]

Both changes should be implemented in https://github.com/fabriziopandini/cluster-api/blob/a8ae016c1800887ecd0683ac2e4720e6f9626d73/util/conditions/setter.go#L41-L78

Thank you, Will try to submit a PR with change

[fabriziopandini]

/assign @Karthik-K-N