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

[ocis] AppRegistry registration refresh #3832

Open
4 tasks
wkloucek opened this issue May 19, 2022 · 20 comments
Open
4 tasks

[ocis] AppRegistry registration refresh #3832

wkloucek opened this issue May 19, 2022 · 20 comments
Labels
Category:Enhancement Add new functionality Category:Technical Technical ehancements Priority:p2-high Escalation, on top of current planning, release blocker Topic:Hardening Type:Story User Story

Comments

@wkloucek
Copy link
Contributor

wkloucek commented May 19, 2022

Is your feature request related to a problem? Please describe.

AppProviders may register theirselves at the AppRegistry via the AddProvider call implemented in https://github.com/cs3org/reva/blob/master/pkg/app/registry/static/static.go#L180.

The AppProvider (currenlty WOPI only) registers itself once on startup in https://github.com/cs3org/reva/blob/master/internal/grpc/services/appprovider/appprovider.go#L141

Describe the solution you'd like

The registration should have a TTL, so that the registry can remove AppProviders that didn't refresh it's registration.

The AppProvider should be responsible to refresh the registration.

Describe alternatives you've considered

Have a static app registry. Downside is, that we also don't notice if the AppProvider is gone (eg. network segregation, ...)
But a static registry is easily scaleable. This isn't the case for the current in-memory registration based registry.

Technical Changes

  • The default app config remains static
  • Instead of "Adding the provider" to the registry we use the implicit registration mechanism of go-micro
  • The app provider metadata needs to be added as service metadata to the go-micro registry for the app provider
  • The app provider needs to refresh his registration. Otherwise it will drop out of the registry.
@wkloucek
Copy link
Contributor Author

@glpatcern what do you think about this idea?

@glpatcern
Copy link

+1, absolutely. I think this was the idea at the beginning and it was never implemented for lack of time.

Another reason why you want to refresh the registration is that the external application may become unavailable, may change URL, etc. Currently, we have to restart the whole chain to pick up such changes, whereas even a daily refresh could be enough here.

@micbar
Copy link
Contributor

micbar commented Jul 4, 2022

Good approach would be a static registry which would allow distributed setups.

@pmaier1 Please clarify the Prio regarding GA

@micbar
Copy link
Contributor

micbar commented May 15, 2023

@wkloucek @glpatcern we need to fix that in the scope of the release 3.0.0 now on reva edge.

@glpatcern
Copy link

Hi there, good to know that you're targeting a 3.0 release (breaking change with 2.x?) - we should maybe coordinate on this?

For info we had brought back this issue here and we'd actually tend towards a static registry, in the sense of making sure that the gateway component can get all it needs from the config remaining stateless, as opposed to having some in-memory state coming from the app providers. This does not address the need for a refresh of WOPI-related apps, where the discovery phase would need to be executed at least once a week or something like that.

@micbar
Copy link
Contributor

micbar commented May 16, 2023

Hi there, good to know that you're targeting a 3.0 release (breaking change with 2.x?) - we should maybe coordinate on this?

Happy to discuss ;-)

For info we had brought back this issue here and we'd actually tend towards a static registry, in the sense of making sure that the gateway component can get all it needs from the config remaining stateless, as opposed to having some in-memory state coming from the app providers. This does not address the need for a refresh of WOPI-related apps, where the discovery phase would need to be executed at least once a week or something like that.

We need to have something which is compatible with container enviromnents. We are using the go-micro registry in ocis which has all these capabilities. We are investigating how to use the go-micro registry for the app-registry.

@micbar
Copy link
Contributor

micbar commented Jun 20, 2023

unassigning @butonic because the issue is stale

@micbar
Copy link
Contributor

micbar commented Jun 21, 2023

@butonic Can you please link the PR and do a handover

@wkloucek
Copy link
Contributor Author

@dragonchaser @micbar I'm a little bit unsure about the current state. Does cs3org/reva#4060 mean that I can now scale the app provider and app registry? Or does it only mean that this hack is no longer needed: https://github.com/owncloud/ocis-charts/blob/8463ab529c5d750c319f117091b49ec93755dd9b/charts/ocis/templates/appprovider/deployment.yaml#L82-L87 ?

@dragonchaser
Copy link
Member

@wkloucek neither, this is not implemented on the ocis side yet, so still WIP.

@exalate-issue-sync exalate-issue-sync bot added the Type:Story User Story label Aug 7, 2023
@exalate-issue-sync exalate-issue-sync bot changed the title AppRegistry registration refresh [ocis] AppRegistry registration refresh Aug 7, 2023
@dragonchaser
Copy link
Member

This ticket is not actionable anymore, the reason is that the wopi server registers its mimeTypes directly through the cs3-api. It would be possible to use these functions to update the go-micro based api, but this would increase the complexity into non-maintainable-levels (risk of split-brains, race-conditions, complex locking structure to deal with the concurrent writes...).
Instead we suggest to replace the static in-memory registry with go-micro store (completely in reva) that uses a dedicated backend (nats, etc or comparable). App-providers should re-register through the cs3 endpoint every 30s.
Another option would be to adjust wopi to write directly to the go-micro registry (which might be an issue with the current python-based implementation)

/cc @micbar @butonic

@dragonchaser dragonchaser removed their assignment Aug 10, 2023
@glpatcern
Copy link

Another option would be to adjust wopi to write directly to the go-micro registry

FWIW, the discovery logic is now entirely in the wopi appprovider in Reva, not in the wopiserver, therefore any evolution for the discovery refresh (typically on a period of ~24h) would need to be developed in Reva.

@micbar
Copy link
Contributor

micbar commented Sep 1, 2023

Currently this ticket is blocked.

@dragonchaser @kobergj @butonic We should schedule a session to gather information and make this actionable.

@dragonchaser
Copy link
Member

We are coming up with an ADR soonish (tm) and afterwards we should be able to craft a new story.

@dragotin
Copy link
Contributor

I am not sure why this is still on hold - removing the status. ADR PR is available and should be reviewed. @micbar @tbsbdr the ticket is still in backlog - maybe it can be given a better prio?

@micbar
Copy link
Contributor

micbar commented Nov 23, 2023

This Ticket was in 3 sprints already. Turned out it was never actionable without a refactoring of the Reva app registry and the cs3 wopiserver.

there is an ADR draft from @dragonchaser

@wkloucek
Copy link
Contributor Author

I guess #8374 unblocks this?

@micbar
Copy link
Contributor

micbar commented Feb 20, 2024

No, the cs3 wopiserver is not compatible with the go micro registry.

@wkloucek
Copy link
Contributor Author

No, the cs3 wopiserver is not compatible with the go micro registry.

#8374 is about the "collaboration" service -> ocis builtin wopi server

@micbar
Copy link
Contributor

micbar commented Feb 20, 2024

Yes. This unblocks it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality Category:Technical Technical ehancements Priority:p2-high Escalation, on top of current planning, release blocker Topic:Hardening Type:Story User Story
Projects
Status: Qualification
Development

No branches or pull requests

6 participants