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

fix(ocis-office): disable scaling in collabora #669

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dschmidt
Copy link
Member

Description

This disables the autoscaling in the collabora online chart.
c.f. https://github.com/CollaboraOnline/online/blob/eb2526d8151edb8f481b3ddbc7ffcd17e5efa291/kubernetes/helm/collabora-online/values.yaml#L232C3-L232C10

Related Issue

  • Fixes <issue_link>

Motivation and Context

I had a lot of issues with locking/not correctly unlocking of files, all went away when I disabled the autoscaling.
As we don't enable scaling for our own services in the examples, I'd argue we can disable it here too.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

@wkloucek
Copy link
Contributor

wkloucek commented Aug 1, 2024

I had a lot of issues with locking/not correctly unlocking of files, all went away when I disabled the autoscaling.

That sounds scary. Does the ingress have the wopiSRC based sticky routing configured?

Does it also appear if we scale Collabora to a fixed replica count like eg. 3 ?

As we don't enable scaling for our own services in the examples, I'd argue we can disable it here too.

The deployment examples use default settings as much as possible to display a specific use case. Therefore we didn't bother enabling autoscaling for oCIS (because this requires also setting resources requests) and the default is off. Since collabora has it enabled by default, we did not bother to turn it off.

@dschmidt
Copy link
Member Author

dschmidt commented Aug 1, 2024

I'm not using ingress here, I read the comments in the example as if it was perfectly fine to connect directly via the cluster internal hostname

@wkloucek
Copy link
Contributor

wkloucek commented Aug 1, 2024

I'm not using ingress here, I read the comments in the example as if it was perfectly fine to connect directly via the cluster internal hostname

Could you please link that comment?

Actually you must use Collabora via some sort of properly configured ingress / reverse proxy: https://github.com/CollaboraOnline/online/blob/eb2526d8151edb8f481b3ddbc7ffcd17e5efa291/kubernetes/helm/collabora-online/values.yaml#L173-L205

Please note that the deployment examples do that for nginx only:

- ingress:
enabled: true
className: "nginx"
annotations:
nginx.ingress.kubernetes.io/upstream-hash-by: "$arg_WOPISrc"
nginx.ingress.kubernetes.io/proxy-body-size: "0"
nginx.ingress.kubernetes.io/proxy-read-timeout: "600"
nginx.ingress.kubernetes.io/proxy-send-timeout: "600"

If you use eg. Traefik, the ingress is not properly configured for having Collabora replicas > 1.

@dschmidt
Copy link
Member Author

dschmidt commented Aug 1, 2024

Oh! I'm sorry, I was half asleep when I wrote the last comment - I'm using ingress for Collabora ofc, especially as the user visible application is served by that after all. (what I was thinking of was the connection to the collaboration service which is ofc unrelated)

Ah indeed I'm using traefik as ingress controller in this cluster! Now I would argue as the readme doesn't say that nginx as lngress controller is required this change still makes sense.... or - of course - we add it to the README... or both

@wkloucek
Copy link
Contributor

wkloucek commented Aug 1, 2024

Ah indeed I'm using traefik as ingress controller in this cluster

Seems like Traefik can't be used: traefik/traefik#1139 (comment)

Now I would argue as the readme doesn't say that nginx as lngress controller is required

The ingress className is "nginx" so I wonder how your Traefik ingress controller picks that up.

EDIT: the Helm Chart used to have documentation in https://doc.owncloud.com/ocis/next/deployment/container/orchestration/orchestration.html#using-our-helm-charts-with-infinite-scale but this is no longer maintained. Therefore some context might be missing, too.

@dschmidt
Copy link
Member Author

dschmidt commented Aug 1, 2024

Yeah, well, I'd argue: it's not super explicit that nginx MUST be used. To me it looked like a recommendation and not like it would basically not work with traefik.

I would still suggest to disable the autoscaling by default, add it to the documentation and/or make it clearer in the readme that nginx is required.

@wkloucek
Copy link
Contributor

@d7oc what do you think?

@d7oc
Copy link
Contributor

d7oc commented Aug 30, 2024

I don't have a clear opinion whether to document that traefik cannot be used or to disable autoscaling, but I tend to go against traefik support. I also have it here (as k3d comes with traefik out of the box and nginx has to be installed via helm first) and in any case even right now you have to switch the default ingress class to make it work with traefik at all. So maybe it's time to drop traefik support.

@wkloucek
Copy link
Contributor

wkloucek commented Sep 2, 2024

I don't have a clear opinion whether to document that traefik cannot be used

I think this clearly belongs into the Collabora Chart and must not be added to the oCIS deployment examples. We must expect people also to look at the chart's documentation that are being used in the examples.

and in any case even right now you have to switch the default ingress class to make it work with traefik at all. So maybe it's time to drop traefik support.

Which again is about the point above: We can never prevent people changing the deployment examples. But if one does it, the mantra is: know what you're doing and read the documentation (of the component where you're changing config)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants