stashed / stash

🛅 Backup your Kubernetes Stateful Applications
https://stash.run
Other
1.32k stars 86 forks source link

BackupConfiguration tempDir doesn't seem to be used? #1493

Closed sgielen closed 1 year ago

sgielen commented 2 years ago

I have this in my BackupConfiguration:

apiVersion: stash.appscode.com/v1beta1
kind: BackupConfiguration
spec:
  tempDir:
    medium: "Memory"
    sizeLimit: "512Mi"

According to https://stash.run/docs/v2022.09.29/concepts/crds/backupconfiguration/:

Stash mounts an emptyDir for holding temporary files. It is also used for caching for faster backup performance. You can configure the emptyDir using spec.tempDir section.

However, the Pod has this as its tmp-dir Volume:

  - emptyDir:
      medium: Memory
      sizeLimit: 256Mi
    name: tmp-dir

I.e. the sizeLimit of 512Mi is not used when patching the Pod definition. Am I doing this wrong or is it a bug?

This is on Stash v0.22.3.

hossainemruz commented 2 years ago

@hmsayem, @piyush1146115 can you take a look?

hmsayem commented 2 years ago

@sgielen can you please share the yaml of the BackupConfiguration ?

sgielen commented 2 years ago

@sgielen can you please share the yaml of the BackupConfiguration ?

Yes, here it is: https://pastebin.com/pd4Rsbfx (set to expire in a week)

hmsayem commented 2 years ago

I couldn't reproduce the issue in the latest release. Can you please try the latest release of Stash (v2022.09.29 ) and let us know if the issue persists.

You can check the following link to upgrade Stash: https://stash.run/docs/v2022.09.29/setup/upgrade/

sgielen commented 2 years ago

I'm running v0.22.3 currently, I suppose that upgrade page mentions Helm chart versions and not Stash versions? I followed those instructions, and with the new Helm chart I'm now running Stash v0.23.1. Initially, there was no change to the emptyDir of the StatefulSet, even as it was updated to the new Stash version:

$ kubectl describe statefulset -n samba samba | tail -n4
  Normal  Sidecar Injection Succeeded  7m19s  Workload Controller     Successfully injected stash sidecar into StatefulSet samba/samba.
  Normal  SuccessfulDelete             4m54s  statefulset-controller  delete Pod samba-0 in StatefulSet samba successful
  Normal  Sidecar Injection Succeeded  4m53s  Workload Controller     Successfully injected stash sidecar into StatefulSet samba/samba.
  Normal  SuccessfulCreate             39s    statefulset-controller  create Pod samba-0 in StatefulSet samba successful
$ kubectl get backupconfiguration -n samba backup -o yaml | grep sizeLimit:  
    sizeLimit: 512Mi
$ kubectl get statefulset samba -n samba -o yaml | grep sizeLimit:
          sizeLimit: 256Mi

I then tried to actively change the BackupConfiguration to see if the change would be synced then, and something was synced to the StatefulSet, but the sizeLimit was still set incorrectly:

$ kubectl apply -f backup.yaml                                  
repository.stash.appscode.com/samba-to-b2 unchanged
backupconfiguration.stash.appscode.com/backup configured
$ kubectl describe statefulset -n samba samba | tail -n4
  Normal  Sidecar Injection Succeeded  7m15s              Workload Controller     Successfully injected stash sidecar into StatefulSet samba/samba.
  Normal  SuccessfulDelete             44s                statefulset-controller  delete Pod samba-0 in StatefulSet samba successful
  Normal  Sidecar Injection Succeeded  42s                Workload Controller     Successfully injected stash sidecar into StatefulSet samba/samba.
  Normal  SuccessfulCreate             8s (x2 over 3m1s)  statefulset-controller  create Pod samba-0 in StatefulSet samba successful
$ kubectl get backupconfiguration -n samba backup -o yaml | grep sizeLimit:
    sizeLimit: 510Mi
$ kubectl get statefulset samba -n samba -o yaml | grep image:
        image: dperson/samba:aarch64
        image: stashed/stash:v0.23.1
$ kubectl get statefulset samba -n samba -o yaml | grep sizeLimit:
          sizeLimit: 256Mi

So I can still reproduce the issue on the newest version, v0.23.1.

hmsayem commented 1 year ago

Can you please delete the current BackupConfiguration and create a new one with the new sizeLimit and check if the tmp-dir Volume of the pod is updated?

sgielen commented 1 year ago

Can you please delete the current BackupConfiguration and create a new one with the new sizeLimit and check if the tmp-dir Volume of the pod is updated?

Would this delete/invalidate my existing backups or can Stash continue backing up as long as the BackupConfiguration and Repository are the same?

hmsayem commented 1 year ago

Would this delete/invalidate my existing backups or can Stash continue backing up as long as the BackupConfiguration and Repository are the same?

It won't affect your previous backups as long as the Repository is same. But if you delete the Repository and the .spec.wipeOut is set to True, the backups will be deleted from the backend.

sgielen commented 1 year ago

@hmsayem Ah, I think this might be it: I removed the BackupConfiguration and watched how the stash sidecar was removed from the StatefulSet. Indeed, it was; only one container remained in the StatefulSet. However, the tmp-dir volume did remain in the StatefulSet, even if it did not exist in my original StatefulSet:

$ kubectl get statefulset samba -n samba -o yaml | grep -B3 tmp-dir
      - emptyDir:
          medium: Memory
          sizeLimit: 256Mi
        name: tmp-dir
$ grep tmp-dir samba-ebs.yaml
$

Indeed, when I reapply the BackupConfiguration the sidecar container is added back in, but the tmp-dir remains at 256Mi.

I then removed the BackupConfiguration again (again, sidecar container was removed, tmp-dir volume remained) and manually removed the tmp-dir volume. Then, I recreated the BackupConfiguration, and indeed, it is created with the correct size:

$ kubectl get statefulset samba -n samba -o yaml | grep -B3 tmp-dir
        terminationMessagePolicy: File
        volumeMounts:
        - mountPath: /tmp
          name: tmp-dir
--
      - emptyDir:
          medium: Memory
          sizeLimit: 510Mi
        name: tmp-dir

So it looks like the issue is that a BackupConfiguration creates a tmp-dir volume, but does not delete it, and also does not update it to reflect new values.

To be honest, I would be concerned if stash would remove global volumes with a name as generic as tmp-dir -- but if the volume was named e.g. stash-tmp-dir, IMO it's more clear that stash controls it and can remove it freely.

hossainemruz commented 1 year ago

if the volume was named e.g. stash-tmp-dir, IMO it's more clear that stash controls it and can remove it freely.

Yes. That makes sense. We will look into it.

hmsayem commented 1 year ago

Hello @sgielen, The issue has been fixed in the latest release of Stash. Please upgrade the Stash version.

https://blog.byte.builders/post/stash-v2023.01.05/ https://stash.run/docs/v2023.01.05/setup/upgrade/

sgielen commented 1 year ago

I can confirm that it's working on the newest version of Stash. Just changing the BackupConfiguration now properly updates the StatefulSet.

$ kubectl get statefulset samba -n samba -o yaml | grep -B3 tmp-dir
        terminationMessagePolicy: File
        volumeMounts:
        - mountPath: /stash-tmp
          name: stash-tmp-dir
--
      - emptyDir:
          medium: Memory
          sizeLimit: 510Mi
        name: stash-tmp-dir
backupconfiguration.stash.appscode.com/backup configured
$ kubectl get statefulset samba -n samba -o yaml | grep -B3 tmp-dir
        terminationMessagePolicy: File
        volumeMounts:
        - mountPath: /stash-tmp
          name: stash-tmp-dir
--
      - emptyDir:
          sizeLimit: 512Mi
        name: stash-tmp-dir

Thank you!