From d75aa2dc4073792f8aeafb6ece967abde1d7329e Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 21 May 2026 16:30:33 -0400 Subject: [PATCH 1/4] fix: prevent false PartiallyFailed on non-HCP backups Three fixes for issue #258 where daily-full backups report 17K errors: 1. AppliesTo() now returns specific HyperShift resource types instead of an empty selector that matches every resource in the cluster 2. GetHCP() returns apierrors.NewNotFound instead of plain fmt.Errorf, so the existing IsNotFound check in Execute() works as intended 3. Cache hcpNotFound flag to skip re-querying on every item when no HCP exists in the backup's included namespaces Fixes #258 Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- pkg/common/utils.go | 5 +++- pkg/common/utils_test.go | 16 +++++++++++ pkg/core/backup.go | 29 ++++++++++++++++---- pkg/core/backup_test.go | 59 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 7 deletions(-) diff --git a/pkg/common/utils.go b/pkg/common/utils.go index c93a96319..f7d4f3908 100644 --- a/pkg/common/utils.go +++ b/pkg/common/utils.go @@ -172,7 +172,10 @@ func GetHCP(ctx context.Context, nsList []string, c crclient.Client, log logrus. return &hcpList.Items[0], nil } - return nil, fmt.Errorf("no HostedControlPlane found") + return nil, apierrors.NewNotFound( + hyperv1.Resource("hostedcontrolplanes"), + "no HostedControlPlane found in provided namespaces", + ) } func GetHCPNamespace(name, namespace string) string { diff --git a/pkg/common/utils_test.go b/pkg/common/utils_test.go index eacfb8871..59786b624 100644 --- a/pkg/common/utils_test.go +++ b/pkg/common/utils_test.go @@ -8,6 +8,7 @@ import ( . "github.com/onsi/gomega" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" @@ -323,6 +324,21 @@ func TestGetHCP(t *testing.T) { } } +func TestGetHCPReturnsNotFoundError(t *testing.T) { + g := NewWithT(t) + + scheme := runtime.NewScheme() + _ = hyperv1.AddToScheme(scheme) + + c := fake.NewClientBuilder().WithScheme(scheme).Build() + log := logrus.New() + + hcp, err := GetHCP(context.TODO(), []string{"ns1", "ns2"}, c, log) + g.Expect(hcp).To(BeNil()) + g.Expect(err).To(HaveOccurred()) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "GetHCP should return a NotFound API error when no HCP exists") +} + func TestAddLabel(t *testing.T) { tests := []struct { name string diff --git a/pkg/core/backup.go b/pkg/core/backup.go index fcf9e1bd2..ad937193b 100644 --- a/pkg/core/backup.go +++ b/pkg/core/backup.go @@ -3,6 +3,7 @@ package core import ( "context" "fmt" + "slices" "strings" common "github.com/openshift/hypershift-oadp-plugin/pkg/common" @@ -27,10 +28,11 @@ type BackupPlugin struct { log logrus.FieldLogger ctx context.Context - client crclient.Client - config map[string]string - validator validation.BackupValidator - hcp *hyperv1.HostedControlPlane + client crclient.Client + config map[string]string + validator validation.BackupValidator + hcp *hyperv1.HostedControlPlane + hcpNotFound bool *plugtypes.BackupOptions // Etcd backup orchestration @@ -118,7 +120,17 @@ func (p *BackupPlugin) Name() string { } func (p *BackupPlugin) AppliesTo() (velero.ResourceSelector, error) { - return velero.ResourceSelector{}, nil + return velero.ResourceSelector{ + IncludedResources: slices.Concat( + plugtypes.BackupCommonResources, + plugtypes.BackupAWSResources, + plugtypes.BackupAzureResources, + plugtypes.BackupIBMPowerVSResources, + plugtypes.BackupOpenStackResources, + plugtypes.BackupKubevirtResources, + plugtypes.BackupAgentResources, + ), + }, nil } // Execute allows the ItemAction to perform arbitrary logic with the item being backed up, @@ -131,12 +143,17 @@ func (p *BackupPlugin) Execute(item runtime.Unstructured, backup *velerov1.Backu return item, nil, nil } + if p.hcpNotFound { + return item, nil, nil + } + if p.hcp == nil { var err error p.hcp, err = common.GetHCP(ctx, backup.Spec.IncludedNamespaces, p.client, p.log) if err != nil { if apierrors.IsNotFound(err) { - p.log.Infof("HCP not found, assuming not hypershift cluster to backup") + p.log.Infof("HCP not found in included namespaces, skipping plugin") + p.hcpNotFound = true return item, nil, nil } return nil, nil, fmt.Errorf("error getting HCP namespace: %v", err) diff --git a/pkg/core/backup_test.go b/pkg/core/backup_test.go index 8f62877ab..796775552 100644 --- a/pkg/core/backup_test.go +++ b/pkg/core/backup_test.go @@ -89,6 +89,65 @@ func newTestBackup() *velerov1.Backup { } } +func TestAppliesToReturnsSpecificResources(t *testing.T) { + g := NewWithT(t) + bp := newTestBackupPlugin() + + selector, err := bp.AppliesTo() + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(selector.IncludedResources).NotTo(BeEmpty(), "AppliesTo must not return an empty resource selector") + g.Expect(selector.IncludedResources).To(ContainElement("hostedcontrolplanes")) + g.Expect(selector.IncludedResources).To(ContainElement("hostedclusters")) + g.Expect(selector.IncludedResources).To(ContainElement("pods")) +} + +func TestExecuteSkipsWhenHCPNotFound(t *testing.T) { + g := NewWithT(t) + + scheme := common.CustomScheme + + hcpCRD := &apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "hostedcontrolplanes.hypershift.openshift.io"}, + } + + client := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(hcpCRD). + Build() + + bp := &BackupPlugin{ + log: logrus.New(), + ctx: context.Background(), + client: client, + config: map[string]string{}, + validator: &mockValidator{}, + BackupOptions: &plugtypes.BackupOptions{}, + hoNamespace: "hypershift", + etcdBackupMethod: common.EtcdBackupMethodVolume, + } + + backup := &velerov1.Backup{ + ObjectMeta: metav1.ObjectMeta{Name: "test-backup", Namespace: "openshift-adp"}, + Spec: velerov1.BackupSpec{ + IncludedNamespaces: []string{"clusters-test"}, + IncludedResources: []string{"hostedcontrolplanes"}, + }, + } + + item := newUnstructuredItem("Secret", "v1", "my-secret", "clusters-test") + + // First call should gracefully skip (no HCP in namespace) + result, _, err := bp.Execute(item, backup) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(result).NotTo(BeNil(), "should return item, not error") + g.Expect(bp.hcpNotFound).To(BeTrue(), "should cache hcpNotFound=true") + + // Second call should return immediately without re-querying + result2, _, err2 := bp.Execute(item, backup) + g.Expect(err2).NotTo(HaveOccurred()) + g.Expect(result2).NotTo(BeNil()) +} + func TestExecute(t *testing.T) { falseVal := false From 80faaf9abd6c12af4ab215fe65aa5f7d1908d56f Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 21 May 2026 16:37:25 -0400 Subject: [PATCH 2/4] fix: add comment explaining hcpNotFound guard Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- pkg/core/backup.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/core/backup.go b/pkg/core/backup.go index ad937193b..578326536 100644 --- a/pkg/core/backup.go +++ b/pkg/core/backup.go @@ -143,6 +143,7 @@ func (p *BackupPlugin) Execute(item runtime.Unstructured, backup *velerov1.Backu return item, nil, nil } + // set to true below when GetHCP returns NotFound for this backup's namespaces if p.hcpNotFound { return item, nil, nil } From 770417a12f26406f5beab10e497b129056c768ce Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 25 May 2026 14:26:49 +0000 Subject: [PATCH 3/4] fix: address PR review comments from jparrill - Fix NewNotFound second arg in GetHCP to use strings.Join(nsList, ",") instead of a message string, so error reads naturally in logs - Add AllPluginResources shared var in types.go as single source of truth for all per-platform resource lists - Replace duplicated slices.Concat(...) in backup.go and restore.go AppliesTo() with plugtypes.AllPluginResources - Fix %v -> %w in backup.go error wrap to preserve error chain - Expand TestAppliesToReturnsSpecificResources with per-provider assertions covering all platform slices (happy path) and unknown resource (sad path) Agent-Logs-Url: https://github.com/kaovilai/hypershift-oadp-plugin/sessions/50f5a7bf-b82c-4644-8f9d-9a5c59924f70 Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com> --- pkg/common/utils.go | 2 +- pkg/core/backup.go | 13 ++----------- pkg/core/backup_test.go | 28 +++++++++++++++++++++++++--- pkg/core/restore.go | 11 +---------- pkg/core/types/types.go | 14 ++++++++++++++ 5 files changed, 43 insertions(+), 25 deletions(-) diff --git a/pkg/common/utils.go b/pkg/common/utils.go index f7d4f3908..8d6ed0915 100644 --- a/pkg/common/utils.go +++ b/pkg/common/utils.go @@ -174,7 +174,7 @@ func GetHCP(ctx context.Context, nsList []string, c crclient.Client, log logrus. } return nil, apierrors.NewNotFound( hyperv1.Resource("hostedcontrolplanes"), - "no HostedControlPlane found in provided namespaces", + strings.Join(nsList, ","), ) } diff --git a/pkg/core/backup.go b/pkg/core/backup.go index 578326536..95955bccc 100644 --- a/pkg/core/backup.go +++ b/pkg/core/backup.go @@ -3,7 +3,6 @@ package core import ( "context" "fmt" - "slices" "strings" common "github.com/openshift/hypershift-oadp-plugin/pkg/common" @@ -121,15 +120,7 @@ func (p *BackupPlugin) Name() string { func (p *BackupPlugin) AppliesTo() (velero.ResourceSelector, error) { return velero.ResourceSelector{ - IncludedResources: slices.Concat( - plugtypes.BackupCommonResources, - plugtypes.BackupAWSResources, - plugtypes.BackupAzureResources, - plugtypes.BackupIBMPowerVSResources, - plugtypes.BackupOpenStackResources, - plugtypes.BackupKubevirtResources, - plugtypes.BackupAgentResources, - ), + IncludedResources: plugtypes.AllPluginResources, }, nil } @@ -157,7 +148,7 @@ func (p *BackupPlugin) Execute(item runtime.Unstructured, backup *velerov1.Backu p.hcpNotFound = true return item, nil, nil } - return nil, nil, fmt.Errorf("error getting HCP namespace: %v", err) + return nil, nil, fmt.Errorf("error getting HCP namespace: %w", err) } } diff --git a/pkg/core/backup_test.go b/pkg/core/backup_test.go index 796775552..07ff12ceb 100644 --- a/pkg/core/backup_test.go +++ b/pkg/core/backup_test.go @@ -96,9 +96,31 @@ func TestAppliesToReturnsSpecificResources(t *testing.T) { selector, err := bp.AppliesTo() g.Expect(err).NotTo(HaveOccurred()) g.Expect(selector.IncludedResources).NotTo(BeEmpty(), "AppliesTo must not return an empty resource selector") - g.Expect(selector.IncludedResources).To(ContainElement("hostedcontrolplanes")) - g.Expect(selector.IncludedResources).To(ContainElement("hostedclusters")) - g.Expect(selector.IncludedResources).To(ContainElement("pods")) + + // Per-provider happy-path assertions: at least one representative resource per platform. + // These ensure that accidentally dropping a platform slice is caught immediately. + for _, tc := range []struct { + provider string + resources []string + }{ + {"common", plugtypes.BackupCommonResources}, + {"aws", plugtypes.BackupAWSResources}, + {"azure", plugtypes.BackupAzureResources}, + {"ibmpowervs", plugtypes.BackupIBMPowerVSResources}, + {"openstack", plugtypes.BackupOpenStackResources}, + {"kubevirt", plugtypes.BackupKubevirtResources}, + {"agent", plugtypes.BackupAgentResources}, + } { + for _, r := range tc.resources { + g.Expect(selector.IncludedResources).To( + ContainElement(r), + "provider %q resource %q must appear in AppliesTo", tc.provider, r, + ) + } + } + + // Sad-path assertion: unknown resources must NOT appear. + g.Expect(selector.IncludedResources).NotTo(ContainElement("completelyunknownresource")) } func TestExecuteSkipsWhenHCPNotFound(t *testing.T) { diff --git a/pkg/core/restore.go b/pkg/core/restore.go index febdbc4fa..484bef15c 100644 --- a/pkg/core/restore.go +++ b/pkg/core/restore.go @@ -3,7 +3,6 @@ package core import ( "context" "fmt" - "slices" "strings" hive "github.com/openshift/hive/apis/hive/v1" @@ -110,15 +109,7 @@ func (p *RestorePlugin) Name() string { func (p *RestorePlugin) AppliesTo() (velero.ResourceSelector, error) { return velero.ResourceSelector{ - IncludedResources: slices.Concat( - plugtypes.BackupCommonResources, - plugtypes.BackupAWSResources, - plugtypes.BackupAzureResources, - plugtypes.BackupIBMPowerVSResources, - plugtypes.BackupOpenStackResources, - plugtypes.BackupKubevirtResources, - plugtypes.BackupAgentResources, - ), + IncludedResources: plugtypes.AllPluginResources, }, nil } diff --git a/pkg/core/types/types.go b/pkg/core/types/types.go index 5cd0d1944..f3b69029f 100644 --- a/pkg/core/types/types.go +++ b/pkg/core/types/types.go @@ -1,5 +1,7 @@ package types +import "slices" + var ( BackupCommonResources = []string{ "hostedclusters", "hostedcluster", "hostedcontrolplanes", "hostedcontrolplane", "nodepools", "nodepool", @@ -16,6 +18,18 @@ var ( BackupOpenStackResources = []string{"openstackmachines", "openstackmachinetemplates", "openstackclusters", "openstackclustertemplates"} BackupKubevirtResources = []string{"kubevirtcluster", "kubevirtmachinetemplate", "datavolume"} BackupAgentResources = []string{"agents", "agentmachines", "agentmachinetemplates", "agentmachinepools", "agentclusters", "nmstateconfigs", "nmstateconfig", "infraenvs", "infraenv"} + + // AllPluginResources is the combined set of all per-platform resource lists. + // Use this single source of truth in AppliesTo() for both backup and restore plugins. + AllPluginResources = slices.Concat( + BackupCommonResources, + BackupAWSResources, + BackupAzureResources, + BackupIBMPowerVSResources, + BackupOpenStackResources, + BackupKubevirtResources, + BackupAgentResources, + ) ) type BackupOptions struct { From a6ce3716267bfcd83c92d1d25a709f3016dd4799 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 26 May 2026 10:18:28 -0400 Subject: [PATCH 4/4] fix: add ClusterDeployment to BackupAgentResources Both backup and restore Execute() handle ClusterDeploymentKind but the resource was missing from BackupAgentResources, preventing Velero from routing ClusterDeployment items to the plugin. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- pkg/core/types/types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/core/types/types.go b/pkg/core/types/types.go index f3b69029f..71061fe3c 100644 --- a/pkg/core/types/types.go +++ b/pkg/core/types/types.go @@ -17,7 +17,7 @@ var ( BackupIBMPowerVSResources = []string{"ibmpowervsmachines", "ibmpowervsmachinetemplates", "ibmpowervsclusters", "ibmpowervsclustertemplates"} BackupOpenStackResources = []string{"openstackmachines", "openstackmachinetemplates", "openstackclusters", "openstackclustertemplates"} BackupKubevirtResources = []string{"kubevirtcluster", "kubevirtmachinetemplate", "datavolume"} - BackupAgentResources = []string{"agents", "agentmachines", "agentmachinetemplates", "agentmachinepools", "agentclusters", "nmstateconfigs", "nmstateconfig", "infraenvs", "infraenv"} + BackupAgentResources = []string{"agents", "agentmachines", "agentmachinetemplates", "agentmachinepools", "agentclusters", "clusterdeployments", "clusterdeployment", "nmstateconfigs", "nmstateconfig", "infraenvs", "infraenv"} // AllPluginResources is the combined set of all per-platform resource lists. // Use this single source of truth in AppliesTo() for both backup and restore plugins.