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

fix(nil pointer): storage class reference #92

Merged

Conversation

itamar-marom
Copy link
Collaborator

@itamar-marom itamar-marom commented Aug 14, 2023

Fixes #1

Description

This PR fixes the issue where the operator gets a nil pointer exception while trying to reach the storage class name of a storage class name template when it is not specified.

Important:
StorageClassName is generally not a required field (https://github.com/kubernetes/kubernetes/blob/0cabb55f7c05eb269ae6f7292271833f095657ec/pkg/apis/core/types.go#L461).
If a storage class is not provided, Kubernetes checks the default StorageClass (https://kubernetes.io/docs/concepts/storage/storage-classes/#default-storageclass).
This is tricky because there could be multiple storage classes and very dangerous to Druid (https://github.com/kubernetes/kubernetes/blob/0cabb55f7c05eb269ae6f7292271833f095657ec/test/e2e/framework/pv/pv.go#L822).
Therefore, we will require users to specify a storage class when they add a volume claim template to their StatefulSets.

Backward compatibility:
Not specifying storageClassName is a very unpopular behavior but it may happen. Such users will have to check their Druid workloads and add a storage class to their claim templates

This PR adds a feature of validation of storage class names inside a Node of kind StatefulSet. It also adds a nil check before trying to reach this field.

Tests are also added to check both logics.


This PR has:

  • been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
  • been tested for backward compatibility on a real K*S cluster by applying the changes introduced here on an existing Druid cluster. If there are any backward incompatible changes then they have been noted in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Key changed/added files in this PR
  • handler.go
  • volume_expansion.go
  • volume_expansion_test.go

@itamar-marom itamar-marom force-pushed the fix/storageClassName-nil-reference branch from c75da26 to da7bab9 Compare December 20, 2023 22:05
@itamar-marom itamar-marom marked this pull request as ready for review December 21, 2023 10:54
@AdheipSingh
Copy link
Contributor

@itamar-marom thanks for this.
@avtarOPS can you help in reviewing this one. Ill review this too

@avtarOPS
Copy link
Contributor

LGTM

@AdheipSingh
Copy link
Contributor

thanks @itamar-marom
Neat code and kudos for adding the test.

Just a nit pick, ideally i would want to get rid of all loggers and emit events instead.
Regardless will merge this.

@AdheipSingh AdheipSingh merged commit 99639e5 into datainfrahq:master Dec 22, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nil pointer dereference when storage class name not set
3 participants