With the organization model added in #394796 (closed) we want to add some setting that is available in admin already to the organization model that will be applied to the groups in the organization.
If you are unsure about the correct group, please do not leave the issue without a group label, and refer to
GitLab's shared responsibility functionality guidelines
for more information on how to triage this kind of issue.
I think visibility control is a good place to start. If all of these settings are too big too start with I think Restricted visibility levels would be a good choice from this section.
We could probably even exclude internal if we want to start even simpler since I am hoping internal will be possible with organizations, but we have not had that conversation yet.
@alexpooley Yes, this setting sounds like a good first option. With the changes we are releasing in %"16.0" we are removing the ability for groups to override it, so it should also become less complex. For the organization level this probably does not make a difference, though.
Alex Pooleychanged title from Add some admin level setting to organization to Restricted visibility levelsAdd some admin level setting to organization
changed title from Add some admin level setting to organization to Restricted visibility levelsAdd some admin level setting to organization
Alex Pooleychanged title from Restricted visibility levelsAdd some admin level setting to organization to Add restricted visibility levels setting to organization
changed title from Restricted visibility levelsAdd some admin level setting to organization to Add restricted visibility levels setting to organization
Alex Pooleychanged the descriptionCompare with previous version
Proposal: OrganizationSettings similar to current ApplicationSettings
Create an organization_settings table that uses the same approach (1 column for each setting) as application_settings
Steps to migrate restricted_visibility_levels
Create a new organization_settings table with organization_id and restricted_visibility_levels columns (and the usual id, created_at, updated_at). The organization_id is required. restricted_visibility_levels has no default and can be NULL
Create Organizations::OrganizationSettings model that has the same validation for restricted_visibility_levels as the one from ApplicationSettings
Create initial Organizations::OrganizationSettings record for the Default Organization based on current setting in ApplicationSetting (a small data migration, just one record)
Ensure that changes to restricted_visibility_levels in ApplicationSettings are also saved in OrganizationSettings
Using triggers or ActiveRecord Hooks or a periodically 'cron' job or a Sidekiq worker, we need to decide on that
Adapt Gitlab::CurrentSettings code so it will read from both OrganizationSettings and ApplicationSettings using this order of preference:
OrganizationSettings
ApplicationSettings from database
ApplicationSettings from default values
Move the setting from Admin/Settings UI to Organization/Settings UI
ApplicationSettings will not go. It will be the home for Instance Settings
Considerations:
We can copy current ApplicationSettings validation logic
The hard work is changing the caching logic
We will end up with a few hundred columns few hundred columns in organization_settings table.
For now, I don't go for this. It was the most boring solution but Alternative 1 (one jsonb column) can also address the 500+ column count in ApplicationSetting so let's go for that.
Alternative 1: OrganizationSettings using jsonb column
Current application_settings table has almost 530 columns. When we need a new setting, we add a column. Since OrganisationSettings is a new thing, we could consider following a different approach for the OrganizationSettings
One approach could be to store all settings for an organization in one column as a JSON object ({'restricted_visibility_levels': [20,30]})
Steps to migrate restricted_visibility_levels
Create new table organization_settings (id, organization_id, settings, created_at, updated_at)
Create new model Organizations::OrganizationSettings that validates this JSON settings, initally only restricted_visibility_levels, we loose database level validations:
Using json_schemer gem (already included in Gitlab project)
Using custom Rails validations (we need to preserve the validation error messages)
Create initial Organizations::OrganizationSettings record for the Default Organization based on current setting in ApplicationSetting (a small data migration, just one record)
Ensure that changes to restricted_visibility_levels in ApplicationSettings are also saved in OrganizationSettings
Using triggers or ActiveRecord Hooks or a periodically 'cron' job or a Sidekiq worker, we need to decide on that
Adapt Gitlab::CurrentSettings code so it will read from both OrganizationSettings and ApplicationSettings using this order of preference:
OrganizationSettings
ApplicationSettings from database
ApplicationSettings from default values
Move the setting from Admin/Settings UI to Organization/Settings UI
Considerations:
Needs more work compared to other solutions
Get rid of 'each setting is a column' approach (more scalable)
Cache logic also needs to be changed
We need a good alternative for existing ApplicationSettings validations
Performance: I wanted to use the existing interface Gitlab::CurrentSettings which is using caching, both in-process and in Redis.
About the nesting, looks like we already store arrays and json in the application_settings. We also store serialized data (YAML) in text columns. A good example is default_branch_protection_defaults: it is using a jsonb column which is validated against this schema.
This is the list of data types that we currently have for application_settings
Alternative 2: Add OrganizationId to ApplicationSettings
We will add a new column organization_id to application_settings table. It will default to 1. All settings are then related to an Organization (initally the default one)
If we add a new organization, we create a new ApplicationSettings Record from the default and store it in the database
Steps to migrate:
Add the organization_id default 1 to application_settings (simple operation because there is only one record for each GitLab installation)
Adapt Gitlab::CurrentSettings and ApplicatoinSettings so the caching will be based on organization_id
Move the setting from Admin/Settings UI to Organization/Settings UI
Considerations:
No distinction between 'Instance settings' and 'Organization settings': also settings like "Maximum attachment size" become organization settings
Caching needs to changed (now based on the fact that we have 1 setting record per instance)
I decided not to propose this one because we can not make a distinction between Instance Settings and Organization Settings.
There's one thing to consider. Currently we can expect to have many application_settings saved, so adding anything with unique constraint will fail. The code I think is currently leaky and does not ensure that there's only one ApplicationSetting entry created.
The Big Question for me: do we consider the 530 columns in application_settings to be a problem that should be addressed? Because now, we have an opportunity for that!
I choose the proposal because it seems to be most straightforward and logical solution. We can re-use code from ApplicationSettings like the validation logic. It is more a 'boring solution'.
I personally prefer alternative 1: store settings in a JSONB column so we do not have to add columns for each setting anymore. But that is more work and therefore, might not fit the 'boring solution' approach.
Alternative 2 is appealing (because it has not much impact on the Backend side) but I think the lack of separation between Instance Settings and Organization Settings is a blocker: all settings are then on the Organization level, including upload limits, signup/in restrictions, some third party settings.
@gitlab-org/tenant-scale-group I am looking forward to find out preferences and most importantly, what reasoning to follow! If you have questions / suggestions related to one of the solutions, please add it to the relevant thread. And you can use this thread for the reasoning what to pick.
The Big Question for me: do we consider the 530 columns in application_settings to be a problem that should be addressed? Because now, we have an opportunity for that!
Since there is a preference for simplifying the database structure for the settings, we can start with Alternative 1: One JSONB store for 1 organization. It is performant and extensible
It's a creative solution but we'll lose things like data type validation which might bite us in the future. Also, it seems we're moving the responsibility of organizing data from the database to the application. Does this offer any advantages over storing data in jsonb which is validated against a schema #394801 (comment 1424798535)?
@tkuah I did a benchmark: compared 1 jsonb column against 1 jsonb per setting name:
user system total realALL settings, one jsonb: 0.042869 0.002839 0.045708 ( 0.061177)ALL settings, one jsonb per key: 0.201791 0.008626 0.210417 ( 0.775616)ALL settings, application_settings: 0.249612 0.009702 0.259314 ( 0.423481)ONE settings one jsonb: 0.044057 0.002863 0.046920 ( 0.061569)ONE settings, one jsonb per key: 0.040834 0.003514 0.044348 ( 0.500781)ONE settings, application_settings: 0.029989 0.001561 0.031550 ( 0.042626)
Summary:
Retrieving one setting is fastest in current application_settings model.
Retrieving one setting does not really matter for both jsonb options
Retrieving all settings using one jsonb outperforms both the current implementation and the one-setting-per-key approach
The current implementation is using ApplicationSetting.last and then cache everything.
I think for now the most viable option is to go for 1 jsonb column for each organization.
@abdwdd The validation is a valid concern. But in the current setup, the only validation that is in the database is the datatype. All the other validation is in application_setting.rb, which is well under test and we can use that code to build OrganizationSettings class.
All the other validation is in application_setting.rb, which is well under test and we can use that code to build OrganizationSettings class.
@rutgerwessels I think with a transposed solution we end up with a giant case setting_name statement.
I agree with @abdwdd, this runs counter to the Rails architecture and we will be pushing against the grain. We can represent a JSONB column as a new serialized class and/or custom type that includes ActiveModel. This would be a more simple port and include validations as we know them.
@alexpooley Yes that was my plan. I wanted to find a way to port code from ApplicationSetting to OrganizationSetting, not to inherit or something like that. There will be be no code sharing between ApplicationSetting and OrganizationSetting model.