Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose "UpToDate" condition on all resources, not just during uptest #327

Open
mbbush opened this issue Jan 12, 2024 · 0 comments
Open

Expose "UpToDate" condition on all resources, not just during uptest #327

mbbush opened this issue Jan 12, 2024 · 0 comments
Labels
enhancement New feature or request v2
Milestone

Comments

@mbbush
Copy link
Contributor

mbbush commented Jan 12, 2024

What problem are you facing?

Sometimes managed resources are stuck in a constant reconciliation loop. I've seen this due to several different causes:

  • An error in the spec.forProvider that the cloud service corrects, but the terraform provider sees as a drift
    • For example, omitting the Version in an aws IAM policy document json string
  • An attribute which, due to the way the external cloud provider works, can only be applied if the resource is in a particular state
    • For example, in an Application.kinesisanalyticsv2.aws.upbound.io, the spec.forProvider.applicationConfiguration.runConfiguration.applicationRestoreConfiguration can only be applied if the application is in a running state, and if spec.startApplication is false, it won't get into that state until someone starts it manually.

The only real indication of these problems is an unusually high number of UpdatedExternalResource events. It would be really helpful if upjet exposed a condition as to whether the latest reconciliation loop found a diff or not. This is basically what the current "Test" condition does, although that's only applied if the resource has an uptest-specific annotation on it.

How could Upjet help solve your problem?

Rather than looking for the upjet.upbound.io/test=true annotation, exposing some version of this condition for all resources would be a useful way to improve visibility into whether a resource is actually in the desired state. I think we'd want a better name than Test, though, and a way to make the condition set to false when a diff is observed.

What about something like

Type: UpToDate
Reason: DiffDetected
Status: False

and

Type: UpToDate
Reason: NoDiffObserved
Status: True

I think eventually it would be great for upjet to apply some sort of exponential backoff when it's been observing the same diff for a long time, to reduce the number of ultimately-futile provider api calls, but that sounds like a much more complex feature to implement another time.

I read through the comments on #23, where this was introduced, but one question I was still left wondering about was why this was originally limited to test resources only.

I would be willing to contribute this feature, if the maintainers think it's a good idea.

@mbbush mbbush added the enhancement New feature or request label Jan 12, 2024
@mbbush mbbush mentioned this issue Jan 13, 2024
3 tasks
@jeanduplessis jeanduplessis modified the milestones: 1.2, 1.3 Feb 3, 2024
@jeanduplessis jeanduplessis removed this from the 1.4 milestone Oct 18, 2024
@jeanduplessis jeanduplessis added this to the 2.x milestone Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v2
Projects
None yet
Development

No branches or pull requests

2 participants