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

Handle alternate forms of resource status #233

Merged
merged 4 commits into from May 13, 2022

Conversation

scothis
Copy link
Contributor

@scothis scothis commented May 12, 2022

We previously assumed that a resource either conformed to our notion of
status or didn't have a status field at all. If a status field existed,
but either didn't have the structure we expected or did have the
structure but was a pointer, would err.

Now we gracefully unpack the status as best we can, and safely ignore
cases where the structure does not conform to our ideal structure. In
those cases the functionality that would be applied is skipped. This
includes:

  • calling status.InitializeConditions()
  • setting status.ObservedGeneration
  • normalizing the LastTransitionTime for conditions

Resolves #186 #108

We previously assumed that a resource either conformed to our notion of
status or didn't have a status field at all. If a status field existed,
but either didn't have the structure we expected or did have the
structure but was a pointer, would err.

Now we gracefully unpack the status as best we can, and safely ignore
cases where the structure does not conform to our ideal structure. In
those cases the functionality that would be applied is skipped. This
includes:
- calling `status.InitializeConditions()`
- setting `status.ObservedGeneration`
- normalizing the LastTransitionTime for conditions

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
Signed-off-by: Scott Andrews <andrewssc@vmware.com>
@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #233 (31534f6) into main (2bcf4ee) will increase coverage by 0.42%.
The diff coverage is 79.48%.

❗ Current head 31534f6 differs from pull request most recent head d3f7560. Consider uploading reports for the commit d3f7560 to get more accurate results

@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
+ Coverage   65.41%   65.83%   +0.42%     
==========================================
  Files           9        9              
  Lines        1067     1127      +60     
==========================================
+ Hits          698      742      +44     
- Misses        348      360      +12     
- Partials       21       25       +4     
Impacted Files Coverage Δ
internal/resources/dies/dies.go 0.00% <0.00%> (ø)
reconcilers/reconcilers.go 80.89% <86.11%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bcf4ee...d3f7560. Read the comment docs.

Copy link
Collaborator

@mamachanko mamachanko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[thought, non-blocking]: I am wondering if the best-effort initialization of conditions could benefit from logs. It may easily get noisy though. I will be able to observe if rr initializes my resource's conditions, but if it doesn't logs may help.

[issue/thought, non-blocking]: As far as I can tell, MyResourceStatus.InitializeConditions() is an undocumented convention. I discovered it by reading other project's code. Maybe we could elevate it to a more prominent feature of the API through an interface or another mechanism.

LGTM 👍🏻

We log when a ParentReconciler is setup when:
- there is no status
- the status is a pointer
- the status doesn't have a ObservedGeneration int64 field
- the status doesn't have a Conditions []metav1.Condition field
- the status doesn't have an InitializeConditions() method

None of these will block the system at runtime, if a field/method is
missing, the behavior is skipped.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
Signed-off-by: Scott Andrews <andrewssc@vmware.com>
@scothis scothis merged commit eed91e0 into vmware-labs:main May 13, 2022
@scothis scothis deleted the flex-status branch May 13, 2022 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panics when reconciling resources with empty MyResourceStatus struct
3 participants