OADP-6536: Call WaitGroup.Done() once only when PVB changes to final status the first time to avoid panic

[kaovilai]

Fix: Call WaitGroup.Done() once only when PVB changes to final status + Update Go to 1.23.0

Summary

This PR includes two important updates:

1. PVB WaitGroup Fix: Adapts upstream fix vmware-tanzu/velero#8940 (commit 2eb97fa) to prevent calling WaitGroup.Done() multiple times for the same PodVolumeBackup, which causes "negative WaitGroup counter" panic errors.

2. Go Version Update: Updates Go version from 1.22.5 to 1.23.0 across all Dockerfiles and CI workflows, aligning with upstream commit 874388bd3e34bbcfe955f624b8e3579d959132fc. This update was missed in previous CVE-related PRs.

Problem

When the event handler receives multiple update events for a PodVolumeBackup that's already in a final status (Completed or Failed), it could call WaitGroup.Done() multiple times, leading to a panic.

Implementation Differences from Upstream

Upstream Version (Velero main branch)

• Uses a pvbIndexer field (cache.Indexer) in the backupper struct
• Tracks PVB states through the indexer infrastructure
• Checks if PVB already exists in final status before calling Done()

Our Version (OADP 1.4)

• Does not have the pvbIndexer infrastructure that exists upstream
• Implements a simpler solution by checking state transitions in the UpdateFunc
• Uses the oldObj and newObj parameters to detect when a PVB transitions to a final status
• No additional fields needed in the backupper struct

Why Direct Cherry-pick Wasn't Possible

The upstream commit relies on the pvbIndexer infrastructure that was introduced in later versions of Velero but is not present in the OADP 1.4 branch. A direct cherry-pick would fail due to:

1. Missing pvbIndexer field in the backupper struct
2. Missing indexer initialization code
3. Different architectural approach to tracking PVB states

Adapted Solution

// In the event handler UpdateFunc:
UpdateFunc: func(oldObj, newObj interface{}) {
    pvb := newObj.(*velerov1api.PodVolumeBackup)
    
    // ... existing checks ...
    
    // 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)
    
    // 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()
    }
}

This achieves the same goal as upstream - preventing multiple Done() calls for the same PVB - but by checking the state transition rather than maintaining additional tracking infrastructure. This approach is simpler and more aligned with the upstream logic while being suitable for OADP 1.4's codebase.

Testing

• All existing tests pass
• Added unit test to verify WaitGroup.Done() is called only once per PVB state transition
• The fix prevents the panic while maintaining the same functional behavior
• Tested with go test ./pkg/podvolume/... -v -count=1

Files Updated for Go 1.23.0

• Dockerfile (2 occurrences)
• hack/build-image/Dockerfile
• .github/workflows/pr-ci-check.yml
• .github/workflows/push.yml
• .github/workflows/e2e-test-kind.yaml (2 occurrences)
• .github/workflows/crds-verify-kind.yaml
• Tiltfile

Related Issues

• Fixes the "negative WaitGroup counter" panic in PodVolumeBackup processing
• Based on upstream PR: Call WaitGroup.Done() once only when PVB changes to fianl status the first time to avoid panic vmware-tanzu/velero#8940
• Go update based on upstream commit: vmware-tanzu@874388b

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• DOWNSTREAM_OWNERS [kaovilai]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

@kaovilai: This pull request references OADP-6536 which is a valid jira issue.

In response to this:

Fix: Call WaitGroup.Done() once only when PVB changes to final status + Update Go to 1.23.0

Summary

This PR includes two important updates:

1. PVB WaitGroup Fix: Adapts upstream fix vmware-tanzu/velero#8940 (commit 2eb97fa) to prevent calling WaitGroup.Done() multiple times for the same PodVolumeBackup, which causes "negative WaitGroup counter" panic errors.

2. Go Version Update: Updates Go version from 1.22.5 to 1.23.0 across all Dockerfiles and CI workflows, aligning with upstream commit 874388bd3e34bbcfe955f624b8e3579d959132fc. This update was missed in previous CVE-related PRs.

Problem

When the event handler receives multiple update events for a PodVolumeBackup that's already in a final status (Completed or Failed), it could call WaitGroup.Done() multiple times, leading to a panic.

Implementation Differences from Upstream

Upstream Version (Velero main branch)

• Uses a pvbIndexer field (cache.Indexer) in the backupper struct
• Tracks PVB states through the indexer infrastructure
• Checks if PVB already exists in final status before calling Done()

Our Version (OADP 1.4)

• Does not have the pvbIndexer infrastructure that exists upstream
• Implements a simpler solution by checking state transitions in the UpdateFunc
• Uses the oldObj and newObj parameters to detect when a PVB transitions to a final status
• No additional fields needed in the backupper struct

Why Direct Cherry-pick Wasn't Possible

The upstream commit relies on the pvbIndexer infrastructure that was introduced in later versions of Velero but is not present in the OADP 1.4 branch. A direct cherry-pick would fail due to:

1. Missing pvbIndexer field in the backupper struct
2. Missing indexer initialization code
3. Different architectural approach to tracking PVB states

Adapted Solution

// In the event handler UpdateFunc:
UpdateFunc: func(oldObj, newObj interface{}) {
   pvb := newObj.(*velerov1api.PodVolumeBackup)
   
   // ... existing checks ...
   
   // 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)
   
   // 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()
   }
}

This achieves the same goal as upstream - preventing multiple Done() calls for the same PVB - but by checking the state transition rather than maintaining additional tracking infrastructure. This approach is simpler and more aligned with the upstream logic while being suitable for OADP 1.4's codebase.

Testing

• All existing tests pass
• Added unit test to verify WaitGroup.Done() is called only once per PVB state transition
• The fix prevents the panic while maintaining the same functional behavior
• Tested with go test ./pkg/podvolume/... -v -count=1

Files Updated for Go 1.23.0

• Dockerfile (2 occurrences)
• hack/build-image/Dockerfile
• .github/workflows/pr-ci-check.yml
• .github/workflows/push.yml
• .github/workflows/e2e-test-kind.yaml (2 occurrences)
• .github/workflows/crds-verify-kind.yaml
• Tiltfile

Related Issues

• Fixes the "negative WaitGroup counter" panic in PodVolumeBackup processing
• Based on upstream PR: Call WaitGroup.Done() once only when PVB changes to fianl status the first time to avoid panic vmware-tanzu/velero#8940
• Go update based on upstream commit: vmware-tanzu@874388b

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[kaovilai]

/retest

[openshift-ci]

@kaovilai: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/lint	bdb2247	link	true	/test lint

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[coderabbitai]

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)

• do-not-merge/work-in-progress

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-bot]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale