diff --git a/.github/workflows/crds-verify-kind.yaml b/.github/workflows/crds-verify-kind.yaml index 13b6fbbd26..15260aa2bf 100644 --- a/.github/workflows/crds-verify-kind.yaml +++ b/.github/workflows/crds-verify-kind.yaml @@ -14,7 +14,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v5 with: - go-version: '1.22.5' + go-version: '1.23.0' id: go # Look for a CLI that's made for this PR - name: Fetch built CLI diff --git a/.github/workflows/e2e-test-kind.yaml b/.github/workflows/e2e-test-kind.yaml index f71f2c5630..fd4e17a24f 100644 --- a/.github/workflows/e2e-test-kind.yaml +++ b/.github/workflows/e2e-test-kind.yaml @@ -14,7 +14,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v5 with: - go-version: '1.22.5' + go-version: '1.23.0' id: go # Look for a CLI that's made for this PR - name: Fetch built CLI @@ -82,7 +82,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v5 with: - go-version: '1.22.5' + go-version: '1.23.0' id: go - name: Check out the code uses: actions/checkout@v4 diff --git a/.github/workflows/pr-ci-check.yml b/.github/workflows/pr-ci-check.yml index bbf9fb1dd3..2e7259d446 100644 --- a/.github/workflows/pr-ci-check.yml +++ b/.github/workflows/pr-ci-check.yml @@ -10,7 +10,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v5 with: - go-version: '1.22.5' + go-version: '1.23.0' id: go - name: Check out the code uses: actions/checkout@v4 diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 9a19477384..287e580de4 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -18,7 +18,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v5 with: - go-version: '1.22.5' + go-version: '1.23.0' id: go - uses: actions/checkout@v4 diff --git a/Dockerfile b/Dockerfile index 7c5905caeb..b52d06fb56 100644 --- a/Dockerfile +++ b/Dockerfile @@ -13,7 +13,7 @@ # limitations under the License. # Velero binary build section -FROM --platform=$BUILDPLATFORM golang:1.22.5-bookworm as velero-builder +FROM --platform=$BUILDPLATFORM golang:1.23.0-bookworm as velero-builder ARG GOPROXY ARG BIN @@ -47,7 +47,7 @@ RUN mkdir -p /output/usr/bin && \ go clean -modcache -cache # Restic binary build section -FROM --platform=$BUILDPLATFORM golang:1.22.5-bookworm as restic-builder +FROM --platform=$BUILDPLATFORM golang:1.23.0-bookworm as restic-builder ARG BIN ARG TARGETOS diff --git a/Tiltfile b/Tiltfile index 6be5057f08..c2fc9dd389 100644 --- a/Tiltfile +++ b/Tiltfile @@ -52,7 +52,7 @@ git_sha = str(local("git rev-parse HEAD", quiet = True, echo_off = True)).strip( tilt_helper_dockerfile_header = """ # Tilt image -FROM golang:1.22.5 as tilt-helper +FROM golang:1.23.0 as tilt-helper # Support live reloading with Tilt RUN wget --output-document /restart.sh --quiet https://raw.githubusercontent.com/windmilleng/rerun-process-wrapper/master/restart.sh && \ diff --git a/hack/build-image/Dockerfile b/hack/build-image/Dockerfile index 7521f72450..650d0e4e72 100644 --- a/hack/build-image/Dockerfile +++ b/hack/build-image/Dockerfile @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -FROM --platform=$TARGETPLATFORM golang:1.22.5-bookworm +FROM --platform=$TARGETPLATFORM golang:1.23.0-bookworm ARG GOPROXY diff --git a/pkg/podvolume/backupper.go b/pkg/podvolume/backupper.go index 8d06cb2225..10524dcb43 100644 --- a/pkg/podvolume/backupper.go +++ b/pkg/podvolume/backupper.go @@ -121,8 +121,8 @@ func newBackupper( b.handlerRegistration, _ = pvbInformer.AddEventHandler( cache.ResourceEventHandlerFuncs{ - UpdateFunc: func(_, obj interface{}) { - pvb := obj.(*velerov1api.PodVolumeBackup) + UpdateFunc: func(oldObj, newObj interface{}) { + pvb := newObj.(*velerov1api.PodVolumeBackup) if pvb.GetLabels()[velerov1api.BackupUIDLabel] != string(backup.UID) { return @@ -133,8 +133,24 @@ func newBackupper( return } + // Check if the previous state was already in a final status + statusChangedToFinal := true + if oldPvb, ok := oldObj.(*velerov1api.PodVolumeBackup); ok { + // If the PVB was already in a final status, no need to call WaitGroup.Done() + if oldPvb.Status.Phase == velerov1api.PodVolumeBackupPhaseCompleted || + oldPvb.Status.Phase == velerov1api.PodVolumeBackupPhaseFailed { + statusChangedToFinal = false + } + } + b.result = append(b.result, pvb) - b.wg.Done() + + // Call WaitGroup.Done() once only when the PVB changes to final status the first time. + // This avoids the cases where the handler gets multiple update events whose PVBs are all in final status + // which causes panic with "negative WaitGroup counter" error + if statusChangedToFinal { + b.wg.Done() + } }, }, ) diff --git a/pkg/podvolume/backupper_test.go b/pkg/podvolume/backupper_test.go index ade7274df8..4d76abeedf 100644 --- a/pkg/podvolume/backupper_test.go +++ b/pkg/podvolume/backupper_test.go @@ -678,3 +678,143 @@ func TestPVCBackupSummary(t *testing.T) { assert.Empty(t, pbs.Skipped) assert.Len(t, pbs.Backedup, 2) } + +func TestBackupperEventHandlerWaitGroupLogic(t *testing.T) { + scheme := runtime.NewScheme() + velerov1api.AddToScheme(scheme) + corev1api.AddToScheme(scheme) + + tests := []struct { + name string + oldPVBStatus *velerov1api.PodVolumeBackupStatus + newPVBStatus *velerov1api.PodVolumeBackupStatus + expectedDoneCalls int + description string + }{ + { + name: "first transition to completed status", + oldPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseInProgress}, + newPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseCompleted}, + expectedDoneCalls: 1, + description: "WaitGroup.Done() should be called once when transitioning to completed", + }, + { + name: "first transition to failed status", + oldPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseInProgress}, + newPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseFailed}, + expectedDoneCalls: 1, + description: "WaitGroup.Done() should be called once when transitioning to failed", + }, + { + name: "multiple updates with completed status", + oldPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseCompleted}, + newPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseCompleted}, + expectedDoneCalls: 0, + description: "WaitGroup.Done() should NOT be called when PVB is already completed", + }, + { + name: "multiple updates with failed status", + oldPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseFailed}, + newPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseFailed}, + expectedDoneCalls: 0, + description: "WaitGroup.Done() should NOT be called when PVB is already failed", + }, + { + name: "transition from new to completed", + oldPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseNew}, + newPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseCompleted}, + expectedDoneCalls: 1, + description: "WaitGroup.Done() should be called when transitioning from new to completed", + }, + { + name: "transition from new to failed", + oldPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseNew}, + newPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseFailed}, + expectedDoneCalls: 1, + description: "WaitGroup.Done() should be called when transitioning from new to failed", + }, + { + name: "no transition for non-final status", + oldPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseNew}, + newPVBStatus: &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseInProgress}, + expectedDoneCalls: 0, + description: "WaitGroup.Done() should NOT be called for non-final status transitions", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Create backup with UID for label matching + backup := &velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-backup", + Namespace: velerov1api.DefaultNamespace, + UID: "test-backup-uid", + }, + } + + // Create old and new PVB objects + oldPVB := &velerov1api.PodVolumeBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pvb", + Namespace: velerov1api.DefaultNamespace, + Labels: map[string]string{ + velerov1api.BackupUIDLabel: string(backup.UID), + }, + }, + Status: *test.oldPVBStatus, + } + + newPVB := &velerov1api.PodVolumeBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pvb", + Namespace: velerov1api.DefaultNamespace, + Labels: map[string]string{ + velerov1api.BackupUIDLabel: string(backup.UID), + }, + }, + Status: *test.newPVBStatus, + } + + // Track Done() calls using a counter + doneCallCount := 0 + + // Create a custom UpdateFunc that mirrors the actual implementation + updateFunc := func(oldObj, newObj interface{}) { + pvb := newObj.(*velerov1api.PodVolumeBackup) + + if pvb.GetLabels()[velerov1api.BackupUIDLabel] != string(backup.UID) { + return + } + + if pvb.Status.Phase != velerov1api.PodVolumeBackupPhaseCompleted && + pvb.Status.Phase != velerov1api.PodVolumeBackupPhaseFailed { + return + } + + // Check if the previous state was already in a final status + statusChangedToFinal := true + if oldPvb, ok := oldObj.(*velerov1api.PodVolumeBackup); ok { + // If the PVB was already in a final status, no need to call WaitGroup.Done() + if oldPvb.Status.Phase == velerov1api.PodVolumeBackupPhaseCompleted || + oldPvb.Status.Phase == velerov1api.PodVolumeBackupPhaseFailed { + statusChangedToFinal = false + } + } + + // Call WaitGroup.Done() once only when the PVB changes to final status the first time. + // This avoids the cases where the handler gets multiple update events whose PVBs are all in final status + // which causes panic with "negative WaitGroup counter" error + if statusChangedToFinal { + doneCallCount++ + } + } + + // Test the UpdateFunc directly + updateFunc(oldPVB, newPVB) + + // Verify the expected number of Done() calls + assert.Equal(t, test.expectedDoneCalls, doneCallCount, test.description) + }) + } +}