On the 1st of the month, recalculate the remaining purchased minutes based on the previous month's usage. This should occur when we lazily create new records for Ci::Minutes::NamespaceMonthlyUsage.
Technical Approach
We currently recalculate remaining additional/purchased CI minutes in bulk. The logic is the same but needs to be done for the single namespace being lazily reset.
If the minutes consumption is greater than the monthly limit (ci_namespace_monthly_usage.amount_used > namespaces.shared_runners_minutes_limit) (or default limit from Gitlab::CurrentSettings) and namespace has any additional minutes available (namespaces.extra_shared_runners_minutes_limit > 0)
Skip the recalculation of extra_shared_runners_minutes_limit in Ci::Minutes::BatchResetService if the feature flag ci_use_new_monthly_minutes is enabled.
Thanks @amandarueda. I think that's simple enough for us to deal with it for now. We should probably involve @shreyasagarwal in the review process then.
When the customer purchase CI plan the extra_shared_runners_minutes_limit value is first fetched from the GI.com via API to find how much minutes the namespace has been left with and then adds the newly purchased quantity to extra_shared_runners_minutes_limit and is send again to the GL.com via API.
Super thanks @shreyasagarwal for the details. I see in the client side (customers-gitlab-com) that we update the namespace in general.
With moving towards monthly tracking for CI minutes we won't update the namespace directly with the extra minutes but we would put the extra minutes on the "current month" which we will be recalculated automatically on the "following month". That will change.
Also, the client fetches the namespace for the current extra minutes prior to adding the new purchased ones. Then sends the new amount to be updated in the GitLab-Rails database. Does that happen because customers-gitlab-com has to do something else? I'm wondering whether we should provide a separate parameter for the API that will take only the newly purchased minutes and we will sum them on our side, rather than taking extra_shared_runners_minutes_limit as absolute value to update.
@fabiopitino The way the extra bought CI minutes used currently is
Before running the CI Gitlab checks how many minutes are still left and it sums up the shared_runners_minutes_limit (value comes with the plan bought) and extra_shared_runners_minutes_limit ( bought as add on in multiples of 1000 )
After running the CI minutes it keeps the total seconds consumed in namespace_statistics table in shared_runners_seconds.
On the 1st of each month there is a cron job which runs in the background and checks if the shared_runners_seconds usage is more than shared_runners_minutes_limit and if yes then subtract that extra value from the extra_shared_runners_minutes_limit column and leave the rest for next months.
So when the customer buys an Add on for 1000 more minutes, CustomerDot then fetches the extra_shared_runners_minutes_limit and then add those 1000 to it and sends it back to Gitlab.
So when the customer buys an Add on for 1000 more minutes, CustomerDot then fetches the extra_shared_runners_minutes_limit and then add those 1000 to it and sends it back to Gitlab.
@shreyasagarwal I was wondering whether the API should take as parameter the minutes the customer bought rather than the full amount to replace in the database. This would (1) save 1 query on CustomerDot as you would not need to first fetch the current extra_shared_runners_minutes_limit before adding the Add-on from the purchase. You would send how many minutes should be added instead and on GitLab side we would take care of that.
We are moving away from storing minutes in namespace_statistics, in favor of monthly tracking. So, extra_shared_runners_minutes_limit would also become another column on a different table. I believe CustomerDot portal should not be coupled to these details and instead should send to Rails backend how many minutes to add to a given namespace.
You would send how many minutes should be added instead and on GitLab side we would take care of that.
There are some issues discussed around improving on the way the CI minutes are handled as of now. It makes total sense as you suggested to send additional values instead of having to retrieve and then add and then sync the new value.
So, extra_shared_runners_minutes_limit would also become another column on a different table.
That can work out well in parallel development even before the monthly tracking is introduced. As of now we also send the shared_runners_minutes_limit from the CustomerDot and can be avoided as we already have plan_limits table in the GL.com database and the CustomerDot can make an API call to increase the value to a separate API than the CRUD API we are calling to update the complete GL.com extra_shared_runners_minutes_limit value for a namespace.
There are some issues discussed around improving on the way the CI minutes are handled as of now.
@shreyasagarwal Do you have a list of issues/epics I can follow? We are also making some improvements so it would be great to coordinate the work here.
@shreyasagarwal I've updated the proposal in this issue description. We are essentially changing the place where extra_shared_runners_minutes_limit is being stored. Could you confirm that, based on this sequence flow as long as the GET /api/v4/namespaces/:id exposes the extra_shared_runners_minutes_limit (even thought the value comes not from namespaces table but from another one) and the PUT /api/v4/namespaces/:id accepts the same extra_shared_runners_minutes_limit param is all fine from Customer Portal?
I want to make sure that we keep the interface the same while we change where the values are read/written. Also, to confirm that Customer Portal with the PUT /api/v4/namespaces/:id will actually override the extra_shared_runners_minutes_limit.
As seen from the codebase here, the service makes a call to the/api/v4/namespaces/:id needed data and then syncs the data back to the namespaces table.
(even thought the value comes not from namespaces table but from another one)
I am not sure about that because the namespaces table do have column extra_shared_runners_minutes_limit as seen here
I am not sure about that because the namespaces table do have column extra_shared_runners_minutes_limit as seen here
Yes, the goal is to remove namespaces.extra_shared_runners_minutes_limit column and read/write data from/to ci_namespace_monthly_usages. The API would still return the attribute extra_shared_runners_minutes_limit in the JSON response and will still accept the same attribute as parameter when updating the minutes. Under the hood we would persist the data in a different table.
Does it make sense?
On a side note I think we should eventually change the way this updates occur where Customer Portal does not overwrite the extra CI minutes but will send the purchased value. Then GitLab Rails will figure out where to add that. I will create a separate issue for that.
@shreyasagarwal I've changed the proposal a bit. We can postpone the removal of namespaces.extra_shared_runners_minutes_limit for now until we know better how to implement customers-gitlab-com#2172 (closed). So, no changes are impacting customer portal for now.
We can even have a separate API to update the CI minutes as the next iteration over it so have an independent entity to be called only for CI minutes updates in future.
@fabiopitino No we dont as of now. The use for having a new API would definitely needed as a follow up after the initial implementation. If you agree otherwise happy to create an issue for it
@fabiopitino Looking at the blocked by / blocks relationships of the issues listed in &4840 (comment 502655475), we probably want to start on this one next, right?
@cheryl.li Yes. We should be work on this too. I'm going to set some time today to update the issues in the Epic with clear proposal so anyone can start working on this. Did we decide whether this and the rest of &4840 (closed) is going to be done right away?
@fabiopitino Let's put forth a proposal so that I can discuss the plans with @thaoyeager to see whether we can prioritize some of this effort. It might be tricky to balance our other priorities on CI given our limited capacity on the team right now.
@cheryl.li@thaoyeager I updated the description and re-weighted it to 5. Maybe we could break it down into 2 separate issues but I wanted to make clear that this issue requires more effort. Also note that I marked it as blocked by#290961 (closed).
@fabiopitino Why would this issue be blocked by #290961 (closed)? I would think it would be the other way around, that this issue (to recalculate the remaining purchased minutes based on the previous month's usage) blocks #290961 (closed) (for notifying the user when CI usage falls below a threshold, based on recalculations in this issue).
@fabiopitino Just following up, I am not sure we absolutely need #290961 (closed) completed first prior to working on this issue. If I understand the issues accurately, it seems more like a nice to have that we notify the user when they're low on CI Minutes. WDYT?
@cheryl.li This issue is blocked by #290961 (closed) because when a customer purchases additional minutes we also reset the information about the notifications to ensure we can send again notifications when the usage goes below certain thresholds again. The reset information is added in #290961 (closed) (migrated for the new monthly model). In this issue we would reset that same information while updating additional minutes.
@jreporter could we schedule this for %14.5? This is pretty important to be able to enable the feature flag #341730 (closed) which we could potentially target for November 1st to switch to the new tracking.
Since this issue was in %Backlog, but with VerifyP2, does Product keep track of ~Verify::P* labels regardless of the milestone? I'm only curious about the process.
Yes, usually during the planning I filter on all Verify::Px and schedule the future VerifyP1s before kick off @fabiopitino - sometimes these issues will stay in VerifyP2 status in the event they can’t fit into the milestone