-
Notifications
You must be signed in to change notification settings - Fork 249
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
Support for tagged servers (ready for review) #1049
Conversation
…ehavior if argument tag not passed
…t for find_available
app/models/server.rb
Outdated
if !tag.nil? && ids.none? { |myid| redis.hget(key(myid), 'tag') == tag } | ||
raise RecordNotFound.new("Couldn't find available Server", name, nil) if tag_required | ||
tag = nil # fall back to servers without tag | ||
end | ||
id, load, hash = 5.times do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I'm not sure for what scenario this 5-times loop has been added, I'm leaving it as is and repeat the server_load zrange query every time, to preserve the original behavior. For better performance and sanity, I'd rather do it once above and, instead of doing a 5-times loop here, iterate through the zrange until a server with hash.present? has been found.
EDIT: I implemented my suggestion in #1052
…d prevent tagging servers with empty string (turn it into nil)
@@ -160,9 +160,9 @@ def create | |||
params.require(:meetingID) | |||
|
|||
begin | |||
server = Server.find_available | |||
rescue ApplicationRedisRecord::RecordNotFound | |||
raise InternalError, 'Could not find any available servers.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This specific message is adopted in servers.rb, to not change the returned error message / break any tests.
if !tag.nil? && tag[-1] == '!' | ||
tag = tag[0..-2].presence # always returns String, if tag is String | ||
tag_required = true | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The block above works as expected for tag_arg = any string with length > 0, including the special case "!", and also "" and nil (all three are equivalent).
…ask to set only the tag
…reate API tests to implicitly check for correctly adjusted meta_server-tag
Thanks -- will review. |
@Ithanil Can this be rebased please? |
@farhatahmad I opted for a merge, because rebase is a bit convoluted in this case. |
* initial minimal implementation of server tags in app/models/server.rb * change defaulting mechanism in find_available to fully preserve old behavior if argument tag not passed * now use meta_server-tag parameter from create API call as tag argument for find_available * update basic servers.rake tasks for new server tags * add support for optional tags and fall back only to untagged servers * improved comments in server.rb * add tests for tagged server.find_available * now raise specific error message when no server with required tag could be found * add tests for tagged create API * add support for tags in server API + corresponding RSpec * now consequently handle requests/tasks with empty or ! tags as nil and prevent tagging servers with empty string (turn it into nil) * finalize tag support in all rake tasks (server sync / status) + add task to set only the tag * store the actual server tag in meeting metadata (not the requested one) * refactor tag-related Rails tests and and migrate to RSpec * now always override/delete tag in create metadata + refactor tagged create API tests to implicitly check for correctly adjusted meta_server-tag * fix typo .nil -> .nil? * update api-README for tags * update rake-README for tags * add README for server tags
Hello! The functionality with tags is interesting, we will use it to select a BBB server by region. We have 3 regions where bbb servers are located, and students and teachers are also located in different regions. And there are situations when a teacher or student has a poor connection with a certain region or, conversely, a good connection with only one region.
|
Description
This PR implements a new feature called "Server Tags" or "Tagged Servers".
It is supposed to work as follows:
This feature allows to provide users the choice of specially configured servers (e.g. optimized for very large conferences) or newer, potentially less stable, BBB versions, without the need of using dedicated LB + Frontend infrastructures. It might be very useful for the transition to BBB 3.0, of which I know many admins are afraid due to the amount of changes in the BBB backend.
Note: Because in the current handling of the create call, Server.find_available will always be executed before checking for existing meetings, any error raised in that method will lead to the create call failing (even if the meeting does already exist). This could happen before, if no more valid server are in server_load. Now it could also happen if no servers with a required tag are present, or no untagged servers when falling back from an optional tag. Merging #1050 would change that such that an existing meeting will be selected even if find_available would fail (for any reason).
TODO:
Testing Steps
For the already added code I have added corresponding automated tests. The code is also already used in actual production deployment, but in combination with #1050 and #1052.
Ideas for Greenlight
I imagine to have a per-role configuration of allowed tags, analogue to the allowed recording visibilities configuration. The difference would be however, that here we need a configurable list of possible tags, unlike the fixed/hardcoded list of possible recording visibilities.
Other ideas
It turned out that one could also "abuse" this feature to achieve per-tenant server pools by enforcing a certain meta_server-tag via OVERRIDE_CREATE_PARAMS for each (or some) tenants. But I think it would be better to have this as an explicit feature and it could be implemented in a very similar way to tags, by allowing to set a list of tenant IDs for a server to restrict the usage to them. But I think I'll wait for a review on the current PRs before working on this.
Merge with PR 1052
After merging #1052,
find_available
inapp/models/server.rb
will look like this: