Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't panic when storageClassName is not set in stateful sets #5247

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

divolgin
Copy link
Contributor

@divolgin divolgin commented Aug 25, 2022

Thank you for contributing to Velero!

Please add a summary of your change

When reassigning storage class names, there needs to be a nil pointer check because the value is optional and the variable is a pointer to a string.

Does your change fix a particular issue?

Fixes #4782

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@blackpiglet
Copy link
Contributor

@divolgin
Thanks for contribution.
StorageClass is optinal for VolumeTemplate, but just for curiosity, is there an example of STS can trigger this issue?
Velero code looks like never assuming this will happen. It would be helpful to know a use case for this change.

@divolgin
Copy link
Contributor Author

@blackpiglet StorageClassName is documented as optional, so there must be a nil pointer check regardless of usage. It's also a good practice in general. Here's the relevant part of the documentation.

	// Name of the StorageClass required by the claim.
	// More info: https://kubernetes.io/docs/concepts/storage/persistent-volumes#class-1
	// +optional
	StorageClassName *string `json:"storageClassName,omitempty" protobuf:"bytes,5,opt,name=storageClassName"`

Here's an example. Note that there is no storage class name.

apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: test-postgres
spec:
  selector:
    matchLabels:
      app: test-postgres
  serviceName: ""
  template:
    metadata:
      labels:
        app: test-postgres
    spec:
      containers:
      - env:
        - name: POSTGRES_PASSWORD
          value: password
        image: postgres:14.5-alpine
        imagePullPolicy: IfNotPresent
        name: test-postgres
        volumeMounts:
        - mountPath: /var/lib/postgresql/data
          name: test-postgres
          subPath: postgres
      volumes:
      - name: test-postgres
        persistentVolumeClaim:
          claimName: test-postgres
  volumeClaimTemplates:
  - metadata:
      name: test-postgres
    spec:
      accessModes:
      - ReadWriteOnce
      resources:
        requests:
          storage: 1Gi

@divolgin divolgin force-pushed the storage-class-panic branch from 74e5e93 to ad4e733 Compare August 25, 2022 15:02
@sseago
Copy link
Collaborator

sseago commented Aug 25, 2022

@blackpiglet Yes, since this is a string pointer instead of a string, Velero needs to be prepared for both nil and "" values. As a pointer, the golang "zero" value will be nil rather than empty string.

@sseago sseago merged commit 7164875 into vmware-tanzu:main Aug 29, 2022
@divolgin
Copy link
Contributor Author

@sseago @blackpiglet Thanks for merging this! Do you have any idea when this fix will be shipped and what version it will be?

@sseago
Copy link
Collaborator

sseago commented Aug 30, 2022

@divolgin It will certainly be in 1.10. At present, there is no 1.9.2 release planned, but if one does happen, this could possibly make it in that one as well,

@blackpiglet
Copy link
Contributor

It should be included in coming Velero version v1.10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Velero fails to restore statefulsets.apps
3 participants