Report a measured zero incremental instead of erasing it (#10309)
Run the E2E test on kind / setup-test-matrix (push) Failing after 4s
e2e-test-kind.yaml / extract (push) Failing after 11s
Run the E2E test on kind / get-go-version (push) Failing after 12s
Run the E2E test on kind / build (push) Skipped
Run the E2E test on kind / run-e2e-test (push) Skipped
push.yml / extract (push) Failing after 7s
Main CI / get-go-version (push) Failing after 8s
Main CI / Build (push) Skipped

* Report a measured zero incremental instead of erasing it

A CBT incremental with an exactly zero delta -- nothing changed since
the parent -- was reported identically to a backup that moved the whole
device. `velero backup describe --details` printed only
"Moved data Size (bytes): 3221225472" with no incremental line, and
status.incrementalBytes was absent, for a run that transferred nothing.
The best possible CBT outcome displayed as the worst, and was
indistinguishable from a genuine full, a whole-device fallback, or a
backup predating incremental accounting.

The zero was being erased twice. Besides the API status fields,
datapath.BackupResult also carried omitempty, and that struct crosses a
JSON boundary from the data mover pod to the controller (see
micro_service_watcher.go), so the value was destroyed before the
controller could persist it. Every uploader always reports a figure
there, so 0 internally always means "transferred nothing" -- dropping
omitempty is sufficient and correct for that hop.

The API fields move to *int64 rather than just dropping omitempty. The
field shipped in v1.18.0-v1.18.2, so backups exist whose stored volume
info has no incrementalSize at all; with a plain int64 those unmarshal
to 0 and would render "Incremental data Size (bytes): 0", a false claim
of a perfect incremental on a run that never measured one. nil means not
measured, a pointer to 0 means measured zero. Both fields already carry
+optional, so the generated CRD schema is unchanged and no regeneration
is required.

Display gates relax from > 0 to != nil in all three places, including
volumesByPod.Add, whose signature takes *int64 now; the restore describer
passes nil, which is correct since restores measure no incremental.

Verified live: the same zero-delta scenario that reported <none> now
reports 0 and renders "Incremental data Size (bytes): 0", while an older
backup described with the new client still correctly prints no
incremental line at all.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
(cherry picked from commit 6c7aa9d588f6d5eab134d4ce19c92b838f45557c)
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* gofmt: fix import ordering in backup_test.go

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* Regenerate CRDs for IncrementalBytes pointer type

make update-crd was missed in the original commit. Regenerated with
the pinned controller-gen v0.16.5 to avoid unrelated version-annotation
churn across other CRDs.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* Add changelog for #10309

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* Address review: make IncrementalBytes a pointer to preserve backward compat

Per Lyndon-Li's review on #10309: dropping omitempty on the plain int64
field breaks compatibility with a data mover from release-1.17 or
earlier that predates IncrementalBytes and never writes the key -- the
new controller would unmarshal a zero value ("nothing transferred")
instead of recognizing the field is simply absent ("not measured").

Switch to *int64 with omitempty restored:
- an old mover's omitted key unmarshals to nil ("not measured")
- a current mover's genuine zero still serializes the key, unmarshaling
  to a non-nil pointer to 0 ("measured zero")
- nonzero values work exactly as before
- an old controller can still unmarshal a numeric value from a new mover

pkg/controller/data_upload_controller.go and pod_volume_backup_controller.go
assign the wire-struct field directly to their already-*int64,omitempty
CRD status field instead of re-wrapping it with ptr.To, since both are
now the same pointer type.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* Fix CI: update marshal-fail test assertions for IncrementalBytes pointer

Both backup_micro_service_test.go files hardcoded the %v-formatted
zero-value BackupResult struct in an error-message assertion. Now that
IncrementalBytes is *int64, its zero value prints as <nil> instead of 0.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

---------

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Tiger Kaovilai
2026-08-25 18:08:17 -04:00
committed by GitHub
co-authored by Claude Fable 5
parent e14ffe3e4c
commit 2c6f45508c
20 changed files with 130 additions and 37 deletions
+1
View File
@@ -0,0 +1 @@
Report a measured zero-byte incremental instead of erasing it from status
@@ -205,8 +205,12 @@ spec:
nullable: true
type: string
incrementalBytes:
description: IncrementalBytes holds the number of bytes new or changed
since the last backup
description: |-
IncrementalBytes holds the number of bytes new or changed since the last backup.
A nil value means the uploader did not report a figure; a pointer to 0 means it
reported zero, i.e. nothing changed and nothing was transferred. The two are
distinct: erasing a measured zero makes a perfect incremental indistinguishable
from a full transfer in every downstream report.
format: int64
type: integer
message:
@@ -192,8 +192,12 @@ spec:
nullable: true
type: object
incrementalBytes:
description: IncrementalBytes holds the number of bytes new or changed
since the last backup
description: |-
IncrementalBytes holds the number of bytes new or changed since the last backup.
A nil value means the uploader did not report a figure; a pointer to 0 means it
reported zero, i.e. nothing changed and nothing was transferred. The two are
distinct: erasing a measured zero makes a perfect incremental indistinguishable
from a full transfer in every downstream report.
format: int64
type: integer
message:
+8 -4
View File
@@ -175,8 +175,11 @@ type SnapshotDataMovementInfo struct {
// Moved snapshot data size.
Size int64 `json:"size"`
// Moved snapshot incremental size.
IncrementalSize int64 `json:"incrementalSize,omitempty"`
// Moved snapshot incremental size, i.e. the bytes actually transferred. Nil means
// the uploader reported no figure (including backups taken before this was
// recorded); a pointer to 0 means it transferred nothing, which is the ideal
// incremental and must stay distinguishable from "unknown".
IncrementalSize *int64 `json:"incrementalSize,omitempty"`
// The DataUpload's Status.Phase value
Phase velerov2alpha1.DataUploadPhase
@@ -225,8 +228,9 @@ type PodVolumeInfo struct {
// The snapshot corresponding volume size.
Size int64 `json:"size,omitempty"`
// The incremental snapshot size.
IncrementalSize int64 `json:"incrementalSize,omitempty"`
// The incremental snapshot size, i.e. the bytes actually transferred. Nil means
// the uploader reported no figure; a pointer to 0 means it transferred nothing.
IncrementalSize *int64 `json:"incrementalSize,omitempty"`
// The type of the uploader that uploads the data. The valid values are `kopia` and `restic`.
UploaderType string `json:"uploaderType"`
@@ -124,9 +124,13 @@ type PodVolumeBackupStatus struct {
// +optional
Progress shared.DataMoveOperationProgress `json:"progress,omitempty"`
// IncrementalBytes holds the number of bytes new or changed since the last backup
// IncrementalBytes holds the number of bytes new or changed since the last backup.
// A nil value means the uploader did not report a figure; a pointer to 0 means it
// reported zero, i.e. nothing changed and nothing was transferred. The two are
// distinct: erasing a measured zero makes a perfect incremental indistinguishable
// from a full transfer in every downstream report.
// +optional
IncrementalBytes int64 `json:"incrementalBytes,omitempty"`
IncrementalBytes *int64 `json:"incrementalBytes,omitempty"`
// AcceptedTimestamp records the time the pod volume backup is to be prepared.
// The server's time is used for AcceptedTimestamp
@@ -1055,6 +1055,11 @@ func (in *PodVolumeBackupStatus) DeepCopyInto(out *PodVolumeBackupStatus) {
*out = (*in).DeepCopy()
}
out.Progress = in.Progress
if in.IncrementalBytes != nil {
in, out := &in.IncrementalBytes, &out.IncrementalBytes
*out = new(int64)
**out = **in
}
if in.AcceptedTimestamp != nil {
in, out := &in.AcceptedTimestamp, &out.AcceptedTimestamp
*out = (*in).DeepCopy()
@@ -165,9 +165,13 @@ type DataUploadStatus struct {
// +optional
Progress shared.DataMoveOperationProgress `json:"progress,omitempty"`
// IncrementalBytes holds the number of bytes new or changed since the last backup
// IncrementalBytes holds the number of bytes new or changed since the last backup.
// A nil value means the uploader did not report a figure; a pointer to 0 means it
// reported zero, i.e. nothing changed and nothing was transferred. The two are
// distinct: erasing a measured zero makes a perfect incremental indistinguishable
// from a full transfer in every downstream report.
// +optional
IncrementalBytes int64 `json:"incrementalBytes,omitempty"`
IncrementalBytes *int64 `json:"incrementalBytes,omitempty"`
// Node is name of the node where the DataUpload is processed.
// +optional
@@ -270,6 +270,11 @@ func (in *DataUploadStatus) DeepCopyInto(out *DataUploadStatus) {
*out = (*in).DeepCopy()
}
out.Progress = in.Progress
if in.IncrementalBytes != nil {
in, out := &in.IncrementalBytes, &out.IncrementalBytes
*out = new(int64)
**out = **in
}
if in.AcceptedTimestamp != nil {
in, out := &in.AcceptedTimestamp, &out.AcceptedTimestamp
*out = (*in).DeepCopy()
+3 -2
View File
@@ -43,6 +43,7 @@ import (
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/utils/ptr"
"github.com/vmware-tanzu/velero/internal/resourcepolicies"
"github.com/vmware-tanzu/velero/internal/volume"
@@ -5681,7 +5682,7 @@ func TestUpdateVolumeInfos(t *testing.T) {
RetainedSnapshot: "vs-1",
SnapshotHandle: "snapshot-id",
Size: 1000,
IncrementalSize: 500,
IncrementalSize: ptr.To(int64(500)),
Phase: velerov2alpha1.DataUploadPhaseFailed,
},
},
@@ -5721,7 +5722,7 @@ func TestUpdateVolumeInfos(t *testing.T) {
RetainedSnapshot: "vs-1",
SnapshotHandle: "snapshot-id",
Size: 1000,
IncrementalSize: 500,
IncrementalSize: ptr.To(int64(500)),
Phase: velerov2alpha1.DataUploadPhaseCompleted,
},
},
+1 -1
View File
@@ -147,7 +147,7 @@ func (d *DataUploadBuilder) Progress(progress shared.DataMoveOperationProgress)
// IncrementalBytes sets the DataUpload's IncrementalBytes.
func (d *DataUploadBuilder) IncrementalBytes(incrementalBytes int64) *DataUploadBuilder {
d.object.Status.IncrementalBytes = incrementalBytes
d.object.Status.IncrementalBytes = &incrementalBytes
return d
}
+13 -5
View File
@@ -739,8 +739,12 @@ func describeDataMovement(d *Describer, details bool, info *volume.BackupVolumeI
d.Printf("\t\t\t\tData Mover: %s\n", dataMover)
d.Printf("\t\t\t\tUploader Type: %s\n", info.SnapshotDataMovementInfo.UploaderType)
d.Printf("\t\t\t\tMoved data Size (bytes): %d\n", info.SnapshotDataMovementInfo.Size)
if info.SnapshotDataMovementInfo.IncrementalSize > 0 {
d.Printf("\t\t\t\tIncremental data Size (bytes): %d\n", info.SnapshotDataMovementInfo.IncrementalSize)
// Print whenever the uploader measured a figure, including zero. A zero-delta
// incremental transfers nothing, which is the whole point of CBT; hiding it
// leaves only the volume size on display and makes the best possible result
// indistinguishable from a full transfer.
if info.SnapshotDataMovementInfo.IncrementalSize != nil {
d.Printf("\t\t\t\tIncremental data Size (bytes): %d\n", *info.SnapshotDataMovementInfo.IncrementalSize)
}
d.Printf("\t\t\t\tResult: %s\n", info.Result)
} else {
@@ -915,7 +919,7 @@ type volumesByPod struct {
// Add adds a pod volume with the specified pod namespace, name
// and volume to the appropriate group.
// Used for both backup and restore
func (v *volumesByPod) Add(namespace, name, volume, phase string, progress veleroapishared.DataMoveOperationProgress, incrementalBytes int64) {
func (v *volumesByPod) Add(namespace, name, volume, phase string, progress veleroapishared.DataMoveOperationProgress, incrementalBytes *int64) {
if v.volumesByPodMap == nil {
v.volumesByPodMap = make(map[string]*podVolumeGroup)
}
@@ -925,8 +929,12 @@ func (v *volumesByPod) Add(namespace, name, volume, phase string, progress veler
// append backup progress percentage if backup is in progress
if phase == "In Progress" && progress.TotalBytes != 0 {
volume = fmt.Sprintf("%s (%.2f%%)", volume, float64(progress.BytesDone)/float64(progress.TotalBytes)*100)
} else if phase == string(velerov1api.PodVolumeBackupPhaseCompleted) && incrementalBytes > 0 {
volume = fmt.Sprintf("%s (size: %v, incremental size: %v)", volume, progress.TotalBytes, incrementalBytes)
} else if phase == string(velerov1api.PodVolumeBackupPhaseCompleted) && incrementalBytes != nil {
// Report the incremental figure whenever it was measured, including zero. Zero is
// the best possible outcome - nothing changed, so nothing was transferred - and
// suppressing it leaves only the volume size on display, which reads as a full
// transfer.
volume = fmt.Sprintf("%s (size: %v, incremental size: %v)", volume, progress.TotalBytes, *incrementalBytes)
} else if (phase == string(velerov1api.PodVolumeBackupPhaseCompleted) ||
phase == string(velerov1api.PodVolumeRestorePhaseCompleted)) &&
progress.TotalBytes > 0 {
+2 -1
View File
@@ -25,6 +25,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
corev1api "k8s.io/api/core/v1"
"k8s.io/utils/ptr"
"github.com/vmware-tanzu/velero/internal/volume"
velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
@@ -629,7 +630,7 @@ func TestCSISnapshots(t *testing.T) {
SnapshotHandle: "fake-repo-id-5",
OperationID: "fake-operation-5",
Size: 100,
IncrementalSize: 50,
IncrementalSize: ptr.To(int64(50)),
Phase: velerov2alpha1.DataUploadPhaseFailed,
},
},
@@ -467,9 +467,13 @@ func describeDataMovementInSF(details bool, info *volume.BackupVolumeInfo, snaps
dataMovement["uploaderType"] = info.SnapshotDataMovementInfo.UploaderType
dataMovement["result"] = string(info.Result)
if info.SnapshotDataMovementInfo.Size > 0 || info.SnapshotDataMovementInfo.IncrementalSize > 0 {
if info.SnapshotDataMovementInfo.Size > 0 {
dataMovement["size"] = info.SnapshotDataMovementInfo.Size
dataMovement["incrementalSize"] = info.SnapshotDataMovementInfo.IncrementalSize
}
// Emit whenever measured, including zero - a zero-delta incremental transferred
// nothing, and that has to be reportable rather than absent.
if info.SnapshotDataMovementInfo.IncrementalSize != nil {
dataMovement["incrementalSize"] = *info.SnapshotDataMovementInfo.IncrementalSize
}
snapshotDetail["dataMovement"] = dataMovement
+1 -1
View File
@@ -417,7 +417,7 @@ func describePodVolumeRestores(d *Describer, restores []velerov1api.PodVolumeRes
restoresByPod := new(volumesByPod)
for _, restore := range restoresByPhase[phase] {
restoresByPod.Add(restore.Spec.Pod.Namespace, restore.Spec.Pod.Name, restore.Spec.Volume, phase, restore.Status.Progress, 0)
restoresByPod.Add(restore.Spec.Pod.Namespace, restore.Spec.Pod.Name, restore.Spec.Volume, phase, restore.Status.Progress, nil)
}
d.Printf("\t%s:\n", phase)
+1 -1
View File
@@ -152,7 +152,7 @@ func TestOnDataUploadCompleted(t *testing.T) {
{
name: "marshal fail",
marshalErr: errors.New("fake-marshal-error"),
expectedErr: "Failed to marshal backup result { false { } 0 0}: fake-marshal-error",
expectedErr: "Failed to marshal backup result { false { } 0 <nil>}: fake-marshal-error",
},
{
name: "succeed",
+2 -1
View File
@@ -22,6 +22,7 @@ import (
"github.com/cockroachdb/errors"
"github.com/sirupsen/logrus"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/client"
"github.com/vmware-tanzu/velero/internal/credentials"
@@ -220,7 +221,7 @@ func (dp *generalDataPath) StartBackup(source AccessPoint, uploaderConfig map[st
}
dp.callbacks.OnFailed(context.Background(), dp.namespace, dp.jobName, dataPathErr)
} else {
dp.callbacks.OnCompleted(context.Background(), dp.namespace, dp.jobName, Result{Backup: BackupResult{snapshotID, emptySnapshot, source, totalBytes, incrementalBytes}})
dp.callbacks.OnCompleted(context.Background(), dp.namespace, dp.jobName, Result{Backup: BackupResult{snapshotID, emptySnapshot, source, totalBytes, ptr.To(incrementalBytes)}})
}
}()
+11 -5
View File
@@ -24,6 +24,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"k8s.io/utils/ptr"
velerotest "github.com/vmware-tanzu/velero/pkg/test"
"github.com/vmware-tanzu/velero/pkg/uploader/provider"
@@ -82,10 +83,11 @@ func TestAsyncBackup(t *testing.T) {
},
result: Result{
Backup: BackupResult{
SnapshotID: "fake-snapshot",
EmptySnapshot: false,
Source: AccessPoint{ByPath: "fake-path"},
TotalBytes: 1000,
SnapshotID: "fake-snapshot",
EmptySnapshot: false,
Source: AccessPoint{ByPath: "fake-path"},
TotalBytes: 1000,
IncrementalBytes: ptr.To(int64(0)),
},
},
path: "fake-path",
@@ -96,7 +98,11 @@ func TestAsyncBackup(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
dp := newGeneralDataPath("job-1", "test", nil, "velero", Callbacks{}, velerotest.NewLogger()).(*generalDataPath)
mockProvider := providerMock.NewProvider(t)
mockProvider.On("RunBackup", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(test.result.Backup.SnapshotID, test.result.Backup.EmptySnapshot, test.result.Backup.TotalBytes, test.result.Backup.IncrementalBytes, test.err)
var incrementalBytes int64
if test.result.Backup.IncrementalBytes != nil {
incrementalBytes = *test.result.Backup.IncrementalBytes
}
mockProvider.On("RunBackup", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(test.result.Backup.SnapshotID, test.result.Backup.EmptySnapshot, test.result.Backup.TotalBytes, incrementalBytes, test.err)
mockProvider.On("Close", mock.Anything).Return(nil)
dp.uploaderProv = mockProvider
dp.initialized = true
@@ -34,6 +34,7 @@ import (
"k8s.io/client-go/kubernetes"
kubeclientfake "k8s.io/client-go/kubernetes/fake"
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"github.com/vmware-tanzu/velero/pkg/builder"
@@ -510,6 +511,42 @@ func TestGetResultFromMessage(t *testing.T) {
},
},
},
{
// An old data mover (release-1.17 and earlier) predates IncrementalBytes and
// never writes the key at all -- this pins that its absence unmarshals to nil
// ("not measured"), not a zero value.
name: "old mover message omits incrementalBytes -> nil",
taskType: TaskTypeBackup,
message: "{\"snapshotID\":\"fake-snapshot-id\",\"emptySnapshot\":false,\"source\":{\"byPath\":\"fake-path-1\",\"volumeMode\":\"Block\"}}",
expectResult: Result{
Backup: BackupResult{
SnapshotID: "fake-snapshot-id",
Source: AccessPoint{
ByPath: "fake-path-1",
VolMode: uploader.PersistentVolumeBlock,
},
IncrementalBytes: nil,
},
},
},
{
// A current mover reports a genuine zero explicitly -- this pins that the key
// being present with value 0 unmarshals to a non-nil pointer to 0 ("measured
// zero"), distinguishing it from the omitted-key case above.
name: "current mover reports measured zero incrementalBytes -> non-nil zero",
taskType: TaskTypeBackup,
message: "{\"snapshotID\":\"fake-snapshot-id\",\"emptySnapshot\":false,\"source\":{\"byPath\":\"fake-path-1\",\"volumeMode\":\"Block\"},\"incrementalBytes\":0}",
expectResult: Result{
Backup: BackupResult{
SnapshotID: "fake-snapshot-id",
Source: AccessPoint{
ByPath: "fake-path-1",
VolMode: uploader.PersistentVolumeBlock,
},
IncrementalBytes: ptr.To(int64(0)),
},
},
},
{
name: "succeed to unmarshall restore result",
taskType: TaskTypeRestore,
+9 -5
View File
@@ -30,11 +30,15 @@ type Result struct {
// BackupResult represents the result of a backup
type BackupResult struct {
SnapshotID string `json:"snapshotID"`
EmptySnapshot bool `json:"emptySnapshot"`
Source AccessPoint `json:"source,omitempty"`
TotalBytes int64 `json:"totalBytes,omitempty"`
IncrementalBytes int64 `json:"incrementalBytes,omitempty"`
SnapshotID string `json:"snapshotID"`
EmptySnapshot bool `json:"emptySnapshot"`
Source AccessPoint `json:"source,omitempty"`
TotalBytes int64 `json:"totalBytes,omitempty"`
// IncrementalBytes is a pointer so an old data mover (release-1.17 and earlier,
// which predates this field) that omits it unmarshals to nil -- "not measured" --
// while a current mover reporting a genuine zero still serializes the key and
// unmarshals to a non-nil zero, distinguishing "measured zero" from "not measured".
IncrementalBytes *int64 `json:"incrementalBytes,omitempty"`
}
// RestoreResult represents the result of a restore
+1 -1
View File
@@ -156,7 +156,7 @@ func TestOnDataPathCompleted(t *testing.T) {
{
name: "marshal fail",
marshalErr: errors.New("fake-marshal-error"),
expectedErr: "Failed to marshal backup result { false { } 0 0}: fake-marshal-error",
expectedErr: "Failed to marshal backup result { false { } 0 <nil>}: fake-marshal-error",
},
{
name: "succeed",