Backup subcontroller's memory usage is unbounded when reconciling backups

[mx-sylveon]

From a quick glance at the source, it appears that the subcontroller just tries to naively get and process all backups in-memory all at once. That obviously means memory usage will increase in an unbounded manner as the number of backups increases.

The issue can of course be mitigated by limiting the number of backups retained, but I think it should be obvious that backup retention configuration is something that the user should be able to make decisions on themselves.

Alternatively, you can increase the memory allocated to the subcontroller (which unfortunately currently requires adjusting the resources on the vttablets, per #661), but that is just a band-aid and would need to increase over time.

Really the subcontroller should just be handling the backups reconciliation in a paginated manner so that the memory usage is capped at a reasonable level.

Additionally, given the current design where backups are near-constantly reconciled to maintain a sync between the VitessBackups and the backups stored in the remote backup location (AWS S3, etc;), it might be prudent to update documentation to give some guidelines on backup retention and its effect on resource usage by Vitess Operator, see: the subcontroller's memory usage, the subcontroller's reconciliation loop runtime increasing as backup count increases, the build up of VitessBackup k8s resources, etc; Not to mention the cloud provider API costs associated with making so many requests to reconcile the backups constantly. Honestly, there should probably be some guidance on expected ways to use vitess-operator without incurring unnecessary cloud costs due to the design of the backup system.

To be honest, this last part might be worth an issue on it's own. 😅

[frouioui]

Hi! Thank you for writing this.

Alternatively, you can increase the memory allocated to the subcontroller (which unfortunately currently requires adjusting the resources on the vttablets, per #661), but that is just a band-aid and would need to increase over time.

I am curious why changing the vttablet resource is required to allocate more resource for the subcontroller. The resource request and limit of the subcontroller pod are set by constants:

vitess-operator/pkg/controller/vitessbackupstorage/reconcile_subcontroller.go

Lines 185 to 193 in 373c791

	// Set resource requests specific to the subcontroller.
	// It doesn't need as much as the main operator process.
	container.Resources.Requests = corev1.ResourceList{
	corev1.ResourceCPU: *resource.NewMilliQuantity(subcontrollerCPUMillis, resource.DecimalSI),
	corev1.ResourceMemory: *resource.NewQuantity(subcontrollerMemoryBytes, resource.BinarySI),
	}
	container.Resources.Limits = corev1.ResourceList{
	corev1.ResourceMemory: *resource.NewQuantity(subcontrollerMemoryBytes, resource.BinarySI),
	}

Additionally, given the current design where backups are near-constantly reconciled

By default, and if no new planetscalev2.VitessBackup objects are created in the cluster, the reconciler will run every 60 seconds. This can be configured via the operator flag --vitessbackupstorage_subcontroller_resync_period.

[mx-sylveon]

I am curious why changing the vttablet resource is required to allocate more resource for the subcontroller. The resource request and limit of the subcontroller pod are set by constants:

I only glanced at the other issue, and assumed the subcontroller was also inheriting the resource configuration like the backup pods do. If the subcontroller pod just uses a hardcoded resource configuration, that's kind of worse since there is actually no way to control it as a user of the Vitess Operator.

By default, and if no new planetscalev2.VitessBackup objects are created in the cluster, the reconciler will run every 60 seconds. This can be configured via the operator flag --vitessbackupstorage_subcontroller_resync_period.

Unless something changed since I tried to address the frequent reconciliation on my setup (several months ago), I don't think it's possible to set that CLI flag when using the Operator - it wasn't surfaced in the VitessCluster spec - even if it is technically supported by the subcontroller pod. In fact, the only thing specific to the subcontroller that is made available is the ability to set the serviceAccountName it should use.

[frouioui]

I only glanced at the other issue, and assumed the subcontroller was also inheriting the resource configuration like the backup pods do. If the subcontroller pod just uses a hardcoded resource configuration, that's kind of worse since there is actually no way to control it as a user of the Vitess Operator.

To the best of my knowledge, we have not had any performance issue with the subcontroller with very large end-users running vitess-operator at a big scale. However, Pull Requests are welcome to make this configurable.

Unless something changed since I tried to address the frequent reconciliation on my setup (several months ago), I don't think it's possible to set that CLI flag when using the Operator - it wasn't surfaced in the VitessCluster spec - even if it is technically supported by the subcontroller pod. In fact, the only thing specific to the subcontroller that is made available is the ability to set the serviceAccountName it should use.

The flag I am mentioning is a CLI flag for the vitess-operator process, thus not configurable via any CRD.

[mx-sylveon]

To the best of my knowledge, we have not had any performance issue with the subcontroller with very large end-users running vitess-operator at a big scale.

All I know is that my on-prem setup that had built up over 12,000 VitessBackup resources across 3 vitess keyspaces started having subcontroller crashloops due to OOM problems while listing resources from an S3 location (actually a local MinIO deployment, but functionally works as an S3 endpoint as far as vitess is concerned).

I resolved the issue but culling most of the historical backups that weren't needed, which allowed the subcontroller to complete its reconciliation successfully after removing the VitessBackups associated with the backups purged from the S3 location.

Now I'm not saying that having 12k+ backups is normal or something you'd necessarily want - things only got to that point on my end from hourly backups not being expired out of storage for 6+ months. I was just pointing out that there is a limit to what the subcontroller can handle with the current design.

The simple action here that I think makes sense, would be to add a note to the backup configuration documentation on the limits that do exist, pointing out that end users will need to implement a proper retention policy on their backups to keep the count of backups from bloating to the point that the subcontroller fails.

[bluecrabs007]

Just saw this issue,
We run pretty large vitessclusters and I can confirm that if we don't purge backups and manage their retention, we do run into subcontroller crashloops due to OOM.