Make PlanLimits available for all installations

In !19033 (merged) we defined a Plan.default which could be used by self-hosted installation to define limits on EE installations. This will enable admins to set limits via the DB/rails console as this is an MVC.

In this issue we need to move the usage of PlanLimits to CE.

This would involve:

• move a minimal version of Plan to CE
• move PlanLimits entirely to CE
• move current limit enforcements to CE (pipeline size, pipeline activity, job activity)

Also ensure that:

• fixtures are created correctly for development and production environment (implement what was attempted here !20303 (closed))

This page may contain information related to upcoming products, features and functionality. It is important to note that the information presented is for informational purposes only, so please do not rely on the information for purchasing or planning purposes. Just like with all projects, the items mentioned on the page are subject to change or delay, and the development, release, and timing of any products, features, or functionality remain at the sole discretion of GitLab Inc.

added Category:Continuous Integration devopsverify grouppipeline execution pipeline typefeature labels

/cc @jlenny @grzesiek @ayufan

Thanks for submitting this issue. Since GitLab's development now happens in a single codebase, the canonical GitLab issue tracker is now at https://gitlab.com/gitlab-org/gitlab/issues.

This issue will automatically be moved there.

added automation:to-be-locked in GitLab FOSS label

@fabiopitino #3656 (closed)

marked this issue as related to #3656 (closed)

mentioned in issue #32985 (closed)

mentioned in issue #33512 (closed)

mentioned in issue #33969 (closed)

changed milestone to %12.6

changed milestone to %Backlog

changed milestone to %12.6

added [deprecated] Accepting merge requests label

added direction label

mentioned in issue #34634 (closed)

In !19033 (comment 234645138) we changed a bit the strategy: We are going to define a default plan which would be available for self-hosted installations. Admins can set instance-wide limits for the default plan. This is a very cheap way to propagate the limits to self-hosted users.

assigned to @fabiopitino

changed weight to 2

changed the description

marked this issue as related to #32823 (closed)

added security label

added workflowready for development label

removed [deprecated] Accepting merge requests label

removed workflowready for development label

@fabiopitino how important is this issue? I don't have a good understanding of it really, and capacity in %12.6 is pretty full.

This is required to solve https://gitlab.com/gitlab-org/gitlab/issues/33234 which is a security issue whose deadline is Nov 22nd.

@jlenny in %12.5 we are enabling pipeline limits for Gitlab.com. This issue is for propagating these limits to self-hosted installation. Aside from allowing to fix https://gitlab.com/gitlab-org/gitlab/issues/33234 it could be a nice feature for admins.

Further conversation sparked in #34634 (comment 234982006)

/cc @ayufan @joshlambert

Thanks @fabiopitino.

@jlenny - we'd like to expose both apply and expose these limits to self-managed customers so they too can benefit from this work. What we find as best practices on GitLab.com we should apply to self-managed.

@joshlambert @fabiopitino that makes sense but in terms of priority how critical is it? This is a confidential issue so it's hard to judge demand. There's a lot queued up that seems to be higher priority, but am I missing something?

@jlenny

We gonna expose Plan.default on EE on-premise, which will allow to define that limit via Rails console.

@ayufan I understand what we're doing pretty well. I'm unclear on how much demand there is for this compared to other things we might work on in 12.6.

@jlenny

This is effectively implemented already (as of before, but not in all cases). So, we just provide a generic mechanism for support any sorts of application limits, exposing this one as well.

Gotcha - so it's very small. That's good enough for me.

Given the changes we made to !19033 (merged) as part of closing #32823 (closed), this could possibly be closed as consequence. Keeping it open for tracking purpose. It may not need any additional work.

Does this issue actually need to be confidential? It was created that way by you @fabiopitino but I'm not sure what part of it needs to be kept secret, especially if this is just adding a feature from gitlab.com to self-managed.

@jlenny The reason why I made it confidential in the first place is because it's linked to #32823 (closed) which highlights the lack of limits on Gitlab.com as of today and these could be potentially exploited by us advertising it. I've seen we have not been consistent though with the confidentiality for all related issues.

removed the weight

@fabiopitino as I understand this issue will focus on the backend only separating it from affecting the application experience directly, correct?

@jlenny if that is the case, will a follow-up issue to affect the application experience be scheduled for the following milestone?

cc: @thaoyeager

@dimitrieh Yes, this is only to set the limits in the database and the backend will enforce them. In #34634 (comment 234982006) some conversations started in how to extend that across the application and even define a UI for admins.

I'm not sure we have a proper issue for it though.

@ayufan As I think of this again, the current implementation of PlanLimits with the addition of Plan.default will make limits already available to self-hosted, as long as they use EE, right?

Do we want to make this a CE feature? @jlenny WDYT?

Yes, but we want to be able to use Plan.default.limits also on CE to have the same behaviour everywhere I assume.

So, we might want to port minimal amount of Plan model, and PlanLimits model to CE.

As the usage limits are also applicable to CE to prevent misuse of the features :)

The fine-control of licenses, etc. are not, but everything else I believe is.

@ayufan thanks! I've updated title and description.

So, we might want to port minimal amount of Plan model, and PlanLimits model to CE.

I agree this would be ideal. The limits and their defaults should apply to CE instances, as well as the mechanism to change them.

The advanced features like multiple plans etc can remain EE.

cc @tipyn in case you have any concerns

Thanks for the ping @joshlambert - @timhey is handling the CI minutes add-ons at the moment. :)

cc @jackie_fraser

changed weight to 2

changed title from Make pipeline creation limits available for users to Make PlanLimits available for all installations

changed the description

added workflowready for development label

changed milestone to %12.7

mentioned in merge request !20303 (closed)

Porting here a conversation with @ayufan from: !20303 (comment 247635800)

Fabio

I think that adding both free and default is what is causing confusion as:

1. free must only exist in Gitlab.com
2. default must exist in self-hosted, if exists in Gitlab.com is not used

Maybe we need to have only 1 of them. How about in #32824 (closed) we remove Plan.default and use Plan.free on self-hosted? This way we simplify the model by having Plan.free to also be the default plan on self-hosted. The other paid hosted plans could be added to the fixture for all environments but they won't be used except for Gitlab.com.

This makes Plan.free the only plan where limits should be defined.

In Namespace#actual_plan we could have

def actual_plan
  strong_memoize(:actual_plan) do
    subscription = find_or_create_subscription

    subscription&.hosted_plan || Plan.free
  end
end

Ideally we should use Plan.default instead of Plan.free on self-hosted but Namespace#actual_plan_name is used a lot on Gitlab.com with the expectation that it could default to free. This is IMO another ~"technical debt" because we are using actual_plan_name as a mix of presentation logic and plan policy. If we could split that and have actual_plan_name to show Free in Gitlab.com UI and in the backend act as Plan.default we could invert that and have Plan.default instead of Plan.free. But this further refactoring concerns me, same as #35161 and #34975 (closed). E.g. Namespace#find_or_create_subscription creates a "free" subscription by default which could cause problems when on self-hosted is expected to be default.

I think a safer option is to use what we already have, which is free plan.

Kamil

Anything that is simpler. However, one thing that confuses me is the meaning of free. On self-hosted it does not make sense.

I guess, it really depends on how it looks. I consider the free to be a specific to GitLab.com, and we have to create it manually, because there's no proper interface to create it.

This leads me to another thought: the free does not differ from any other hosted plans, like silver and bronze. Why we have the special handling for free in such case? This confuses me more than introduction of Plan.default which is a fallback case in all other cases.

So, what I'm saying is not a problem that we create default+free, the problem is that free is created and considered to not be part of all_hosted_plans.

I would try to resolve the special handling of free and make it behave as any other plan that could be created.

Fabio

Anything that is simpler. However, one thing that confuses me is the meaning of free. On self-hosted it does not make sense.

Totally agree with that. I think that after #35161 and #34975 (closed) we should rename entirely free to default which is a name that makes sense for both self-hosted and Gitlab.com. Showing the default plan as Free in the UI is a presentation detail that should be handled differntly, as stated in !20303 (comment 247635800)

the free does not differ from any other hosted plans, like silver and bronze. Why we have the special handling for free in such case?

The free plan seems to be created/associated to the namespace by Namespace#generate_subscription by default, so it's not a special plan and it's also considered a fallback.

the problem is that free is created and considered to not be part of all_hosted_plans.

This is a problem indeed. Free should be part of ALL_HOSTED_PLANS

I'm moving the discussion to #32824 (closed) where we can decide how to proceed.

Some design concerns highlighted during a Slack conversation with @ayufan about ~"technical debt" around Plan model:

• When we calculate Namespace#actual_plan we call generate_subscription if one does not exist for the namespace. This defaults to creating a free subscription as fallback. On self-hosted this would end up associating namespaces to a free plan which should not exist on self-hosted. Instead the fallback should be default. This could actually cause a bypass of the limits that admins set for default plan, because any plan (free included) by default has no limits.
• We should probably only have 1 of either default or free plans. Both makes things confusing. The one being chosen should be the only fallback plan on both Gitlab.com, self-hosted EE and self-hosted CE.
  • Using free as fallback is the easiest because it's already widely used for Gitlab.com but the name "free" does not make sense for self-hosted. It means in the first iteration admins on self-hosted would set limits for a free plan. This would happen anyway in the rails console and not yet exposed via the UI.
  • Using default as fallback means that we need to break the dependency between the fallback plan and UI presentation logic where we display "Free plan". It would also involve a database migration to change instances of free plan to default. This would involve a larger refactoring.
  • 3rd options could be using free as MVC and work towards renaming it to default in the long term.
• The fact that we are calling generate_subscription on self-hosted it's a bad design. Namespace should probably be associated to a plan directly with an additional subscription association if plan different than free/default.

Outstanding ~"technical debt" issues that require attention in the same domain:

• #35161
• #34975 (closed)

changed the description

removed workflowready for development label

changed milestone to %12.8

Moving this to the backlog as we are focusing on other priorities in the %12.8 release.

@ayufan - is there another group that you could recommend who would be able to deliver this? Maybe groupmemory - since enabling our self-managed instances to apply limits would go a long way towards improving their own stability and isolation?

cc @craig-gomes

Not sure, but this is pretty straightforward now to move it CE. I'm reluctant to say that this is groupmemory, but someone needs to do it. It is related to general system stability/security/usage and there's very little related to memory optimisation.

What's the priority or urgency in shipping this? If it does go a long way towards improving stability and isolation, then it seems to align with our goals for availability and the isolation working group.

I see a weight of 2 and @ayufan comment that this is pretty straightforward, but when I read through the description and comments it looks like this could be test intensive to make sure we covered all of the permutations. Thoughts?

To @ayufan's point, this is not directly related to memory optimization. However, if this does help with customer experience by allowing them controls to improve stability, and potentially isolation, this seems to be a worthy effort to review for %12.8 for the memory team. Thoughts @joshlambert ?

I see a weight of 2 and @ayufan comment that this is pretty straightforward, but when I read through the description and comments it looks like this could be test intensive to make sure we covered all of the permutations. Thoughts?

@craig-gomes Tests are already done. I think it is more a matter of moving changes and specs from EE to CE. DB does not to be moved, as CE and EE have the same DB schema already, so we only talk about code-changes.

To @ayufan's point, this is not directly related to memory optimization. However, if this does help with customer experience by allowing them controls to improve stability, and potentially isolation, this seems to be a worthy effort to review for %12.8 for the memory team. Thoughts @joshlambert ?

I think we, as a company, should do this in %12.8 or %12.9. I don't know of a better group to tackle this, so if we can fit this in that would be great. I don't think this is higher priority than the import/export work, but if we have room in %12.8 after those items it would be great to do this.

@ayufan - one question I do have is how Plan.default maps to other plans. Is this effectively the plan that "Core" projects have on GitLab.com? So for example, if we ship the most limited plan to Core on GitLab.com, that would become the default for all self-managed projects?

one question I do have is how Plan.default maps to other plans. Is this effectively the plan that "Core" projects have on GitLab.com? So for example, if we ship the most limited plan to Core on GitLab.com, that would become the default for all self-managed projects?

# GitLab.com (today)
Plan.gold/silver/bronze/free

# EE (today)
Plan.default

# CE (once backported)
Plan.default

So, AFAIK on-premise EE user already have ability to configure limits.

@craig-gomes in the interest of getting our limits on sound footing, I will move to groupmemory so we can deliver this in %12.9.

For example, this will be important to deliver the offset based pagination limit in %12.10 for self-managed like we announced in %12.7.

changed milestone to %Backlog

mentioned in issue #196018 (closed)

changed milestone to %12.9

added devopssystems groupcloud connector labels and removed devopsverify grouppipeline execution labels

Weight: 3

Impact: ?

This sounds like it requires a decent amount of domain knowledge and I find it difficult to estimate this based just on what I've read here. Sounds like a great opportunity to fill in any potential knowledge gaps on @gl-memory as we pick this up.

I think this grows on importance, as more usage limits are desired to be implemented.

I would consider this to be related to GitLab.com availability to make it easier to propose and implement usage limits.

changed milestone to %12.10

@fabiopitino this is assigned to you, but also to groupmemory

Is this something you are planning on working on in the near future or should it be unassigned?

@craig-gomes As this was assigned to ~"group::continuous integration" before I was thinking of working on it as I had put some thoughts together with Kamil. However I'm not actively working on it so it should be unassigned.

I think all the details are reported in the issue but I'm happy to help if anything is not clear.

@craig-gomes We recently had an issue related to this #209303 (closed).

Pinging @matteeyah, just in case he was also doing something related.

Not working on this directly but I have another MR that implements application limits - !27004 (merged). I didn't add self-hosted limits not to create the same problem as last time.

This is just to say that we need this backported ASAP since we're going to be building more and more application limits that depend on the default plan.

unassigned @fabiopitino

assigned to @nmilojevic1

marked this issue as related to #29566 (closed)

marked this issue as related to #211479 (closed)

mentioned in issue #211479 (closed)

mentioned in merge request !26070 (merged)

mentioned in merge request !27574 (merged)

mentioned in merge request !27663 (merged)

@ayufan did this, while he was looking on something related to CI minutes:

• !27574 (merged): updates practices to make it easier to add limits
• !27663 (merged) backports limits to CE based on updated practices
• !27616 (closed) fix Plan understanding (optional)

@ayufan I will reassign this to you since you have completed 99% of it.

assigned to @ayufan and unassigned @nmilojevic1

mentioned in issue #34565 (closed)

changed milestone to %13.0

marked this issue as related to #34565 (closed)

This has become a blocker for #34565 (closed)

This is done.

added blocker label

removed the relation with #34565 (closed)

closed

made the issue visible to everyone

I unmarked this issue as confidential. There's nothing confidential here.

added Deliverable label

added quad-planningready label

added quad-planningcomplete-no-action label and removed quad-planningready label

added devopsdata stores label and removed devopssystems label

changed the description