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 a bug that would cause a crash if StorageClassName was left to nil #688

Merged
merged 4 commits into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/kindIntegTest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ jobs:
with:
repository: topolvm/topolvm
path: topolvm
ref: topolvm-chart-v15.0.0
ref: topolvm-chart-v15.2.0
- name: Create LVM from TopoLVM's example setup
run: |
cd topolvm/example
Expand Down Expand Up @@ -367,7 +367,7 @@ jobs:
run: |
IMG=${{ steps.load.outputs.operator_img }} LOG_IMG=${{ steps.load.outputs.logger_img }} make integ-test
- name: Archive k8s logs
# if: ${{ failure() }}
if: ${{ failure() }}
uses: actions/upload-artifact@v4
with:
name: k8s-logs-topolvm-test-${{ matrix.version }}
Expand Down
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ Changelog for Cass Operator, new PRs should update the `main / unreleased` secti

## unreleased

* [BUGFIX] [#687](https://github.com/k8ssandra/cass-operator/issues/687) Prevent a crash when when StorageClassName was not set in the CassandraDataVolumeClaimSpec

## v1.22.0

* [FEATURE] [#263]((https://github.com/k8ssandra/cass-operator/issues/263) Allow increasing the size of CassandraDataVolumeClaimSpec if the selected StorageClass supports it. This feature is currently behind a opt-in feature flag and requires an annotation ``cassandra.datastax.com/allow-storage-changes: true`` to be set in the CassandraDatacenter.
* [FEATURE] [#263](https://github.com/k8ssandra/cass-operator/issues/263) Allow increasing the size of CassandraDataVolumeClaimSpec if the selected StorageClass supports it. This feature is currently behind a opt-in feature flag and requires an annotation ``cassandra.datastax.com/allow-storage-changes: true`` to be set in the CassandraDatacenter.
* [FEATURE] [#646](https://github.com/k8ssandra/cass-operator/issues/646) Allow starting multiple parallel pods if they have already previously bootstrapped and not planned for replacement. Set annotation ``cassandra.datastax.com/allow-parallel-starts: true`` to enable this feature.
* [ENHANCEMENT] [#648](https://github.com/k8ssandra/cass-operator/issues/648) Make MinReadySeconds configurable value in the Spec.
* [ENHANCEMENT] [#184](https://github.com/k8ssandra/cass-operator/issues/349) Add CassandraDatacenter.Status fields as metrics also
Expand Down
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Build the manager binary
FROM golang:1.22 as builder
FROM golang:1.22 AS builder
ARG TARGETOS
ARG TARGETARCH

Expand Down
2 changes: 1 addition & 1 deletion apis/cassandra/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion apis/config/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion apis/control/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions logger.Dockerfile
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
FROM redhat/ubi8:latest as builder
FROM redhat/ubi8:latest AS builder
ARG VERSION
ARG TARGETPLATFORM

# Install Vector
ENV VECTOR_VERSION 0.39.0
ENV VECTOR_VERSION=0.39.0
RUN case ${TARGETPLATFORM} in \
"linux/amd64") VECTOR_ARCH=x86_64 ;; \
"linux/arm64") VECTOR_ARCH=aarch64 ;; \
Expand Down
10 changes: 5 additions & 5 deletions pkg/reconciliation/reconcile_datacenter.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,14 +162,14 @@ func (rc *ReconciliationContext) listPVCs(selector map[string]string) ([]corev1.
return pvcList.Items, nil
}

func storageClass(ctx context.Context, c client.Client, storageClassName string) (*storagev1.StorageClass, error) {
if storageClassName == "" {
func storageClass(ctx context.Context, c client.Client, storageClassName *string) (*storagev1.StorageClass, error) {
if storageClassName == nil || *storageClassName == "" {
storageClassList := &storagev1.StorageClassList{}
if err := c.List(ctx, storageClassList, client.MatchingLabels{"storageclass.kubernetes.io/is-default-class": "true"}); err != nil {
return nil, err
}

if len(storageClassList.Items) > 0 {
if len(storageClassList.Items) > 1 {
return nil, fmt.Errorf("found multiple default storage classes, please specify StorageClassName in the CassandraDatacenter spec")
} else if len(storageClassList.Items) == 0 {
return nil, fmt.Errorf("no default storage class found, please specify StorageClassName in the CassandraDatacenter spec")
Expand All @@ -179,7 +179,7 @@ func storageClass(ctx context.Context, c client.Client, storageClassName string)
}

storageClass := &storagev1.StorageClass{}
if err := c.Get(ctx, types.NamespacedName{Name: storageClassName}, storageClass); err != nil {
if err := c.Get(ctx, types.NamespacedName{Name: *storageClassName}, storageClass); err != nil {
return nil, err
}

Expand All @@ -188,7 +188,7 @@ func storageClass(ctx context.Context, c client.Client, storageClassName string)

func (rc *ReconciliationContext) storageExpansion() (bool, error) {
storageClassName := rc.Datacenter.Spec.StorageConfig.CassandraDataVolumeClaimSpec.StorageClassName
storageClass, err := storageClass(rc.Ctx, rc.Client, *storageClassName)
storageClass, err := storageClass(rc.Ctx, rc.Client, storageClassName)
if err != nil {
return false, err
}
Expand Down
24 changes: 24 additions & 0 deletions pkg/reconciliation/reconcile_datacenter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,14 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
v1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"k8s.io/utils/ptr"

"github.com/k8ssandra/cass-operator/pkg/mocks"
)
Expand Down Expand Up @@ -115,3 +119,23 @@ func TestDeletePVCs_FailedToDelete(t *testing.T) {

assert.EqualError(t, err, "failed to delete")
}

func TestStorageExpansionNils(t *testing.T) {
rc, _, cleanupMockScr := setupTest()
defer cleanupMockScr()
require := require.New(t)

rc.Datacenter.Spec.StorageConfig.CassandraDataVolumeClaimSpec.StorageClassName = nil
supports, err := rc.storageExpansion()
require.NoError(err)
require.False(supports)

storageClass := &storagev1.StorageClass{}
require.NoError(rc.Client.Get(rc.Ctx, types.NamespacedName{Name: "standard"}, storageClass))
storageClass.AllowVolumeExpansion = ptr.To[bool](true)
require.NoError(rc.Client.Update(rc.Ctx, storageClass))

supports, err = rc.storageExpansion()
require.NoError(err)
require.True(supports)
}
12 changes: 5 additions & 7 deletions pkg/reconciliation/reconcile_racks.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,6 @@ func isPVCStatusConditionTrue(pvc *corev1.PersistentVolumeClaim, conditionType c
func (rc *ReconciliationContext) CheckVolumeClaimSizes(statefulSet, desiredSts *appsv1.StatefulSet) result.ReconcileResult {
rc.ReqLogger.Info("reconcile_racks::CheckVolumeClaims")

supportsExpansion, err := rc.storageExpansion()
if err != nil {
return result.Error(err)
}

for i, claim := range statefulSet.Spec.VolumeClaimTemplates {
// Find the desired one
desiredClaim := desiredSts.Spec.VolumeClaimTemplates[i]
Expand All @@ -225,8 +220,6 @@ func (rc *ReconciliationContext) CheckVolumeClaimSizes(statefulSet, desiredSts *
currentSize := claim.Spec.Resources.Requests[corev1.ResourceStorage]
createdSize := desiredClaim.Spec.Resources.Requests[corev1.ResourceStorage]

// TODO This code is a bit repetitive with all the Status patches. Needs a refactoring in cass-operator since this is a known
// pattern. https://github.com/k8ssandra/cass-operator/issues/669
if currentSize.Cmp(createdSize) > 0 {
msg := fmt.Sprintf("shrinking PVC %s is not supported", claim.Name)
if err := rc.setCondition(
Expand All @@ -248,6 +241,11 @@ func (rc *ReconciliationContext) CheckVolumeClaimSizes(statefulSet, desiredSts *
return result.Error(fmt.Errorf(msg))
}

supportsExpansion, err := rc.storageExpansion()
if err != nil {
return result.Error(err)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] Are we moving this inside the loop on the assumption that volume expansions will be rare?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and this check is irrelevant in our other use cases right now.

if !supportsExpansion {
msg := fmt.Sprintf("PVC resize requested, but StorageClass %s does not support expansion", *claim.Spec.StorageClassName)
rc.Recorder.Eventf(rc.Datacenter, corev1.EventTypeWarning, events.InvalidDatacenterSpec, msg)
Expand Down
3 changes: 2 additions & 1 deletion pkg/reconciliation/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ func CreateMockReconciliationContext(

storageClass := &storagev1.StorageClass{
ObjectMeta: metav1.ObjectMeta{
Name: storageClassName,
Name: storageClassName,
Labels: map[string]string{"storageclass.kubernetes.io/is-default-class": "true"},
},
}

Expand Down
Loading