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

sftd: add support for multiple SFT servers #1325

Merged
merged 13 commits into from Feb 17, 2021
Merged

sftd: add support for multiple SFT servers #1325

merged 13 commits into from Feb 17, 2021

Conversation

arianvp
Copy link
Contributor

@arianvp arianvp commented Jan 13, 2021

  • The ingress assigns an SFT allocation request to a random SFT
  • Each sftd pod is made aware of an URL on which it is directly
    reachable, and will return the URL in the response to the client. e.g.
    Pod sftd-0 will be assigned https://sft.example.com/sfts/sftd-0
  • The client tells this URL to other clients willing to join the call
  • Other clients make a request to this URL
  • The ingress points requests to /sfts to the join-call deployment,
    which will redirect to the specific pod, such that the client can join
    the conference call of the other client

Copy link
Contributor

@lucendio lucendio left a comment

Choose a reason for hiding this comment

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

It's a very interesting approach. Amazing work! Some nits and concerns on my end. Not necessarily related to the approach though.
I still have some questions, but that's something for a coffee.

charts/sftd/README.md Outdated Show resolved Hide resolved
charts/sftd/README.md Outdated Show resolved Hide resolved
charts/sftd/README.md Outdated Show resolved Hide resolved
charts/sftd/README.md Outdated Show resolved Hide resolved
charts/sftd/README.md Outdated Show resolved Hide resolved
@@ -1,5 +1,8 @@
# SFTD Chart

In theory the `sftd` chart can be installed on its own, but it's usually
installed as part of the `wire-server` umbrella chart.
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought: I understand it might be nice to just say "helm install wire-server", though often "more complex" services (stateful ones or those with slower update cycles like SFT due to e.g. not wanting to cut ongoing calls out for 6 hours or so) being bundled into an otherwise fast-to-upgrade chart may slow any wire-server upgrades down going forward. I presume some scripts and/or helmfiles to be in use anyway, as more than a single chart needs to be installed.
Not sure if there are benefits otherwise that I might be missing? Or do I overestimate potential downsides?

(feel free to go ahead with the wrapper chart approach, just wondering about the motivations)

Copy link
Contributor Author

@arianvp arianvp Feb 15, 2021

Choose a reason for hiding this comment

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

I'm really on the fence between the two options. It was originally the way you just described and I only changed it today...

Motivation was:

I wanted to do a bit of refactoring in the wire-server umbrella chart in a follow-up PR so that there is a globals.baseDomain option that all charts can use to remove all the domain duplication in our values.yml. As a form of experiment I wanted to make the SFT chart (which needs to know the base domain; and the domain name of the webapp chart) use this method of configuration to set its host and allowOrigin options.

Also helm upgrade --wait only waits for new pods to be spawned; it doesn't wait for old pods to be terminated; so even with a slow update cycle (due to setting terminationGracePeriodSeconds to a few hours) upgrades would still be "snappy".

However now I just realised that's only true for Deployment; not for StatefulSet; as StatefulSet replaces pods one by one (See Future Work section at the bottom of the README) so it might take longer for --wait to return.

I'm going to sleep on it and revisit tomorrow. I might end up reverting this again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jschaul I have made it clear in the docs that both ways are possible. docs.wire.com currently still instructs to not use the umbrella chart

Copy link
Member

Choose a reason for hiding this comment

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

@jschaul I have made it clear in the docs that both ways are possible. docs.wire.com currently still instructs to not use the umbrella chart

Great! Thanks!

arianvp added a commit to wireapp/wire-server-deploy that referenced this pull request Feb 15, 2021
arianvp added a commit to wireapp/wire-server-deploy that referenced this pull request Feb 15, 2021
arianvp and others added 11 commits February 15, 2021 22:23
* The ingress assigns an SFT allocation request to a random SFT
* Each sftd pod is made aware of an URL on which it is directly
reachable, and will return the URL in the response to the client.  e.g.
Pod `sftd-0` will be assigned `https://sft.example.com/sfts/sftd-0`
* The client tells this URL to other clients willing to join the call
* Other clients make a request to this URL
* The ingress points requests to `/sfts` to the `join-call` deployment,
which will redirect to the specific pod, such that the client can join
the conference call of the other client
some kubernetes clusters (like Scaleway) don't name the DNS server
kube-dns but core-dns.  /etc/resolv.conf is guaranteed to point to the
correct thing though.
This makes it quicker to scale up and down. As sft is actually not
stateful and does not require ordered restarts. We're just using
StatefulSet to get a persistent DNS name so we can join existing calls.
Co-authored-by: Lucendio <gregor.jahn@wire.com>
We can make deployment of sftd a bit easier in the future. Added a
section on this so I do not forget :)
Also make the docs a lot nicer.
@arianvp arianvp merged commit 3f819a6 into develop Feb 17, 2021
@arianvp arianvp deleted the sftd-ha-2 branch February 17, 2021 18:04
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.

None yet

3 participants