[MM-44651] Implement `MaxCallParticipants` config setting #93

streamer45 · 2022-06-02T10:35:31Z

Summary

PR implements a new MaxCallParticipants config setting to optionally limit the maximum number of participants in calls.

For on-prem the default is 0 meaning unlimited.
For Cloud the default is 8.

Config value can be also be overridden through the MM_CALLS_MAX_PARTICIPANTS environment variable if needed.

@cpoile I made various changes to the logic to avoid having two values for essentially the same thing. It would be great if you could do some QA on this to make sure I haven't missed anything.

Ticket Link

https://mattermost.atlassian.net/browse/MM-44651

streamer45 · 2022-06-02T10:36:33Z

server/api.go

 	}
 	ret := config{
-		clientConfig:         p.getConfiguration().getClientConfig(),
-		SkuShortName:         skuShortName,
-		CloudMaxParticipants: cloudMaxParticipants,


I realize this is a breaking change but given it only affects Cloud I think we can safely make it now.

I think... Good thing mobile isn't cut yet. :)

streamer45 · 2022-06-02T10:38:36Z

webapp/src/types/types.ts

@@ -58,16 +58,16 @@ export type CallsConfig = {
    ICEServers: string[],
    AllowEnableCalls: boolean,
    DefaultEnabled: boolean,
+    MaxCallParticipants: number,


Going snake case makes the server code more complex as we need yet another config wrapper, not sure if it's worth it to be honest but let me know.

Nah, it's fine. We're kind of stuck with it for now.

cpoile

Looks good code-wise, I went through it carefully. I will approve after I do QA on it (will take awhile to get the server set up and run through all the combinations).

plugin.json

cpoile · 2022-06-02T13:38:10Z

server/api.go

 	}
 	ret := config{
-		clientConfig:         p.getConfiguration().getClientConfig(),
-		SkuShortName:         skuShortName,
-		CloudMaxParticipants: cloudMaxParticipants,


I think... Good thing mobile isn't cut yet. :)

server/cloud_limits.go

cpoile · 2022-06-02T13:48:23Z

server/configuration.go

@@ -162,6 +172,10 @@ func (c *configuration) Clone() *configuration {
 		}
 	}

+	if c.MaxCallParticipants != nil {
+		cfg.MaxCallParticipants = model.NewInt(*c.MaxCallParticipants)


question for myself: why model.NewInt here but new(int) everywhere else?

Honestly, I feel like Hamlet now 💀

Part of me wants to avoid relying on model as much as possible but it's so tempting to save a line by using the util...

cpoile · 2022-06-02T13:55:34Z

webapp/src/components/channel_header_button/component.tsx

+        ${(props) => props.isCloudPaid && css`
+            cursor: not-allowed;
+        `}


cpoile · 2022-06-02T14:03:00Z

webapp/src/types/types.ts

@@ -58,16 +58,16 @@ export type CallsConfig = {
    ICEServers: string[],
    AllowEnableCalls: boolean,
    DefaultEnabled: boolean,
+    MaxCallParticipants: number,


Nah, it's fine. We're kind of stuck with it for now.

server/cloud_limits.go

Co-authored-by: Christopher Poile <[email protected]>

cpoile · 2022-06-02T15:32:21Z

server/activate.go

@@ -56,6 +47,16 @@ func (p *Plugin) OnActivate() error {
 	// On Cloud installations we want calls enabled in all channels so we
 	// override it since the plugin's default is now false.
 	if isCloud(p.pluginAPI.System.GetLicense()) {
+		cfg.MaxCallParticipants = new(int)
+		*cfg.MaxCallParticipants = cloudMaxParticipantsDefault
+		if maxPart := os.Getenv("MM_CALLS_MAX_PARTICIPANTS"); maxPart != "" {


Discovered in QA: Do we not want to allow on-prem to use the MM_CALLS_MAX_PARTICIPANTS flag?

I think it's more like we need it on Cloud, not so much on on-prem although given it's not possible to use common configuration overrides due to the plugin id issue I think it makes sense to allow this to be used on non-cloud installations as well. I can update 👍

cpoile · 2022-06-02T22:16:52Z

This has been a disaster to test. First, after setting up a mocked CWS system and a custom docker image, and spinning up a test server using that hash, you can only upload the plugin once. After that the plugin upload button is deactivated (but you can get around that by never leaving the system console). Second, often you'll get an error back from uploading a plugin:

If you get that, your server is toast -- it will no longer accept plugin uploads, so you have to spin up a new one.

Third, the MAX_CALLS_MAX_PARTICIPANTS env var wasn't working at all. Finally figured out that OnConfigurationChange is getting called after the onActivate has set the overrides, which overwrites the config with the one from the system console. So 36c67f6 fixes that and lets us have permanent overrides. (FYI: This happens with on-prem servers as well.) But that means cloud customers cannot turn off default calls, which we probably don't want. So we have to find a way to make the setOverrides override the plugin default, but not once the user has changed it manually.

Fourth, I could not test whether the manual max participants configuration setting works in cloud, because it wouldn't let me change any config settings. It would respond with:

Finally, I could not test the max 8 default for cloud. This is because I could not change other settings like Enable Open Server because it responded with:

What I was able to test:

with 36c67f6 the env var works correctly and is sent to webapp (and mobile) and the UI changes as expected.

What I was not able to test:

changing the config setting in the system console works as expected (i.e., is transmitted to the clients and changes the limits and the UI as expected)
that changing the config setting for default enabled in cloud is respected (it won't be, at the moment)
that the cloud max of 8 participants was respected (the point above about not being able to change Enable Open Server)

Issues found:

We should add the N/A cursor for the channel header button and channel header dropdown button.
Config changes require the webapp and mobile to reload before they get the new config values, which means they'll try to join a call and get no response. I think this is probably "ok" for beta, but not later.

So at the moment I think we need to:

fix the Default Enable Calls setting override issue,
and then just to ship it and hope?
And then we need to get proper cloud freemium test servers with plugin uploads.

streamer45 · 2022-06-03T08:05:36Z

Thanks @cpoile ,

I am sorry you had to go through all that trouble just to test this but if anything it helped to reinforce even further the need to have better tools/permissions when testing/debugging in Cloud. I'll make sure to raise these concerns in our future discussions with Cloud/SREs.

For the sake of this PR could we test whatever is feasible using cloud licenses on a local env? I understand it won't be 100% accurate (as we have seen already) but it's better than nothing.

Third, the MAX_CALLS_MAX_PARTICIPANTS env var wasn't working at all. Finally figured out that OnConfigurationChange is getting called after the onActivate has set the overrides, which overwrites the config with the one from the system console.

Interesting, there must be some racy behaviour with the two operations then given that locally I was consistently getting the activation after the initial config change trigger. I am almost regretting not going forward with the plugin id change since it would have prevented all this 😬

Anyhow, I think for the time being we can put in place a little check and apply the overrides as soon as the configuration has been loaded the first time. E.g.:

func (p *Plugin) setConfiguration(configuration *configuration) error {
	p.configurationLock.Lock()
	defer p.configurationLock.Unlock()

	if p.configuration == nil && configuration != nil {
		setOverrides(configuration)
	}

What do you think?

Config changes require the webapp and mobile to reload before they get the new config values, which means they'll try to join a call and get no response. I think this is probably "ok" for beta, but not later.

Yes this is expected and fine for now. Created MM-44781.

We should add the N/A cursor for the channel header button and channel header dropdown button.

I think there may still be some issues with on-prem I guess where we should show the blocked cursor anyway since there's nothing to click but it's a minor thing.

cpoile · 2022-06-03T14:13:27Z

No worries. :)

For the sake of this PR could we test whatever is feasible using cloud licenses on a local env? I understand it won't be 100% accurate (as we have seen already) but it's better than nothing.

I've done that, and that would be the "let's just hope" option. In the end I don't think there is a way to finish testing this at the moment, so it's the only option.

What do you think?

That won't work, unfortunately, because the pluginAPI isn't set at that point either.

I think that putting the overrides in the OnActivate works only when the server is started/restarted with the EnvVars. Which I can't test because this PR's plugin isn't in the prepackaged plugins list for a test server. I could try to make that happen, but that would be a lot of work.

Instead, when you upload a plugin, that's when the overrides in the OnActivate don't work, because the OnConfigurationChange is called once more after the OnActivate, and overwrites the OnActivate overrides.
Maybe this is not intended and it's a bug?

But either way, maybe we can keep it in the OnActivate and then just know that any time the plugin is updated (either manually, which isn't possible, right? -- or through the marketplace, which is possible, right?) the env var and defaultEnabled will be lost.

cpoile · 2022-06-03T16:35:25Z

Done. Was able to test everything, including mobile. I wasn't able to test that changing the config setting in the system console doesn't affect the overrides, because of the error I talked about above. But I'm assuming so, b/c of the code.

I think we're good to go @streamer45

streamer45 · 2022-06-03T16:56:29Z

Thanks @cpoile for the huge effort here 💯

streamer45 added 3 commits May 27, 2022 13:20

Review plugin's configuration

dc0154b

Merge remote-tracking branch 'origin/main' into MM-44560

94d9396

Implement MaxCallParticipants config setting

b25d752

streamer45 added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer labels Jun 2, 2022

streamer45 self-assigned this Jun 2, 2022

streamer45 requested review from cpoile and itao June 2, 2022 10:35

streamer45 commented Jun 2, 2022

View reviewed changes

streamer45 mentioned this pull request Jun 2, 2022

[MM-44651] Implement MaxCallParticipants config setting mattermost/mattermost-mobile#6334

Merged

1 task

cpoile reviewed Jun 2, 2022

View reviewed changes

cpoile added 2: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Jun 2, 2022

streamer45 added 2 commits June 2, 2022 16:18

Fix comment

3548002

Improve help text

ad4b096

cpoile reviewed Jun 2, 2022

View reviewed changes

server/cloud_limits.go Outdated Show resolved Hide resolved

Update server/cloud_limits.go

abfb43a

Co-authored-by: Christopher Poile <[email protected]>

cpoile reviewed Jun 2, 2022

View reviewed changes

itao approved these changes Jun 2, 2022

View reviewed changes

streamer45 and others added 2 commits June 2, 2022 18:28

Allow MaxCallParticipants env override on on-prem

894f444

fix cursor; put config overrides in setConfiguration

36c67f6

streamer45 removed the 1: PM Review Requires review by a product manager label Jun 3, 2022

cpoile added 3 commits June 3, 2022 10:39

MaxParticipants should not be settable on cloud, and env should override

2c60467

use proper cloud license detection

f2e408f

permanently override cloud with env vars and defaultEnabled

bceab79

streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: QA Review Requires review by a QA tester labels Jun 3, 2022

Base automatically changed from MM-44560 to main June 7, 2022 06:28

Merge remote-tracking branch 'origin/main' into MM-44651

8533208

streamer45 requested a review from cpoile June 7, 2022 07:26

cpoile approved these changes Jun 7, 2022

View reviewed changes

streamer45 merged commit 97b9801 into main Jun 7, 2022

streamer45 deleted the MM-44651 branch June 7, 2022 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-44651] Implement `MaxCallParticipants` config setting #93

[MM-44651] Implement `MaxCallParticipants` config setting #93

streamer45 commented Jun 2, 2022

streamer45 Jun 2, 2022

cpoile Jun 2, 2022

streamer45 Jun 2, 2022

cpoile Jun 2, 2022

cpoile left a comment

cpoile Jun 2, 2022

cpoile Jun 2, 2022

streamer45 Jun 2, 2022

cpoile Jun 2, 2022

cpoile Jun 2, 2022

cpoile Jun 2, 2022

streamer45 Jun 2, 2022

cpoile commented Jun 2, 2022 •

edited

Loading

streamer45 commented Jun 3, 2022

cpoile commented Jun 3, 2022

cpoile commented Jun 3, 2022

streamer45 commented Jun 3, 2022

[MM-44651] Implement MaxCallParticipants config setting #93

[MM-44651] Implement MaxCallParticipants config setting #93

Conversation

streamer45 commented Jun 2, 2022

Summary

Ticket Link

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpoile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpoile commented Jun 2, 2022 • edited Loading

streamer45 commented Jun 3, 2022

cpoile commented Jun 3, 2022

cpoile commented Jun 3, 2022

streamer45 commented Jun 3, 2022

[MM-44651] Implement `MaxCallParticipants` config setting #93

[MM-44651] Implement `MaxCallParticipants` config setting #93

cpoile commented Jun 2, 2022 •

edited

Loading