Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Keep trying to sync informer until it success or done message #2134

Closed
wants to merge 1 commit into from

Conversation

lenriquez
Copy link
Contributor

Signed-off-by: Luis Enriquez [email protected]

What this PR does / why we need it: While loading some services the dynamic cache could return an empty array, this could make the resource viewer to update twice, the main idea is to avoid this behaviour

Which issue(s) this PR fixes

Special notes for your reviewer:
The git issue explains why I believed this was not a race condition problem between typedVisitors

In order to replicate the issue is necessary to shut down octant and use a GKE cluster or any cluster that takes some time to responde

Release note:

release-note

@wwitzel3
Copy link
Contributor

wwitzel3 commented Mar 10, 2021

I'm not sure this is the behavior we want. I think what we want is:

  • Wait for sync for a fixed period.
  • If we fail to sync, fall back to direct call
  • Repeat this for every call until we sync
  • Set hasSync to true

We don't want a long block on sync, what if this operation is very slow?

@lenriquez
Copy link
Contributor Author

lenriquez commented Mar 10, 2021

@wwitzel3 The function waitForSyncFunc runs on a go routine so it blocks the go routine but the main thread continue and fall back to the direct call here https://github.com/vmware-tanzu/octant/blob/master/internal/objectstore/dynamic_cache.go#L269
and when the go routine finish it will set hasSync variable to true

@lenriquez
Copy link
Contributor Author

Do you think the comment on code is misleading?

@wwitzel3
Copy link
Contributor

@lenriquez lenriquez changed the title Keep trying to sync informer case until it success or done message Keep trying to sync informer until it success or done message Mar 10, 2021
@mklanjsek
Copy link
Contributor

Unfortunately this is not solving the original problem, please check this workload and see how it keeps updating and changing the layout:
http://localhost:7777/#/workloads/namespace/ingress-nginx/detail/echo-ingress

@wwitzel3
Copy link
Contributor

What's the status of this one? Still in flight? I'm ok closing this PR and moving this out to 0.19 if we don't want to spend more time on it now.

This behavior exists in Octant today so it isn't a regression of any kind.

@lenriquez
Copy link
Contributor Author

Yes! we can close this, this solution is not working :/

@lenriquez lenriquez closed this Mar 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add root node to resource viewer schema
3 participants