feat: classify known Azure CSI snapshot errors as retriable (cloudnative-pg#6906)

Fixes: cloudnative-pg#6901 

Signed-off-by: Jeff Mealo <jmealo@protonmail.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>

Author: 3 people

.wordlist-en-custom.txt

@@ -1147,6 +1147,7 @@ rehydrated
 rehydration
 relabelings
 relatime
+retryable
 replicationSecretVersion
 replicationSlots
 replicationTLSSecret

pkg/reconciler/backup/volumesnapshot/errors.go

@@ -0,0 +1,89 @@
+/*
+Copyright The CloudNativePG Contributors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package volumesnapshot
+
+import (
+	"regexp"
+	"strconv"
+	"strings"
+)
+
+var (
+	retryableStatusCodes = []int{408, 429, 500, 502, 503, 504}
+	httpStatusCodeRegex  = regexp.MustCompile(`HTTPStatusCode:\s(\d{3})`)
+)
+
+// isRetriableErrorMessage detects if a certain error message belongs
+// to a retriable error or not. This is obviously an heuristic but
+// unfortunately we don't have that information exposed in the
+// Kubernetes VolumeSnapshot API and the CSI driver haven't that too.
+func isRetriableErrorMessage(msg string) bool {
+	isRetryableFuncs := []func(string) bool{
+		isExplicitlyRetriableError,
+		isRetryableHTTPError,
+		isConflictError,
+	}
+
+	for _, isRetryableFunc := range isRetryableFuncs {
+		if isRetryableFunc(msg) {
+			return true
+		}
+	}
+
+	return false
+}
+
+// isConflictError detects optimistic locking errors
+func isConflictError(msg string) bool {
+	// Obviously this is a heuristic, but unfortunately we don't have
+	// the information we need.
+	// We're trying to handle the cases where the external-snapshotter
+	// controller failed on a conflict with the following error:
+	//
+	// > the object has been modified; please apply your changes to the
+	// > latest version and try again
+
+	return strings.Contains(msg, "the object has been modified")
+}
+
+// isExplicitlyRetriableError detects explicitly retriable errors as raised
+// by the Azure CSI driver. These errors contain the "Retriable: true"
+// string.
+func isExplicitlyRetriableError(msg string) bool {
+	return strings.Contains(msg, "Retriable: true")
+}
+
+// isRetryableHTTPError, will return a retry on the following status codes:
+// - 408: Request Timeout
+// - 429: Too Many Requests
+// - 500: Internal Server Error
+// - 502: Bad Gateway
+// - 503: Service Unavailable
+// - 504: Gateway Timeout
+func isRetryableHTTPError(msg string) bool {
+	if matches := httpStatusCodeRegex.FindStringSubmatch(msg); len(matches) == 2 {
+		if code, err := strconv.Atoi(matches[1]); err == nil {
+			for _, retryableCode := range retryableStatusCodes {
+				if code == retryableCode {
+					return true
+				}
+			}
+		}
+	}
+
+	return false
+}

pkg/reconciler/backup/volumesnapshot/errors_test.go

@@ -0,0 +1,37 @@
+/*
+Copyright The CloudNativePG Contributors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package volumesnapshot
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Retriable error messages", func() {
+	DescribeTable(
+		"Retriable error messages",
+		func(msg string, isRetriable bool) {
+			Expect(isRetriableErrorMessage(msg)).To(Equal(isRetriable))
+		},
+		Entry("conflict", "Hey, the object has been modified!", true),
+		Entry("non-retriable error", "VolumeSnapshotClass not found", false),
+		Entry("explicitly retriable error", "Retriable: true, the storage is gone away forever", true),
+		Entry("explicitly non-retriable error", "Retriable: false because my pod is working", false),
+		Entry("error code 502 - retriable", "RetryAfter: 0s, HTTPStatusCode: 502, RawError: Internal Server Error", true),
+		Entry("error code 404 - non retriable", "RetryAfter: 0s, HTTPStatusCode: 404, RawError: Not found", false),
+	)
+})

pkg/reconciler/backup/volumesnapshot/resources.go

@@ -19,7 +19,6 @@ package volumesnapshot
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	storagesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -69,26 +68,16 @@ func (err volumeSnapshotError) Error() string {
 // IsRetryable returns true if the external snapshotter controller
 // will retry taking the snapshot
 func (err volumeSnapshotError) isRetryable() bool {
-	if err.InternalError.Message == nil {
-		return false
-	}
-
-	// Obviously this is a heuristic, but unfortunately we don't have
-	// the information we need.
-	// We're trying to handle the cases where the external-snapshotter
-	// controller failed on a conflict with the following error:
-	//
-	// > the object has been modified; please apply your changes to the
-	// > latest version and try again
-
 	// TODO: instead of blindingly retry on matching errors, we
 	// should enhance our CRD with a configurable deadline. After
 	// the deadline have been met on err.InternalError.CreatedAt
 	// the backup can be marked as failed
 
-	return strings.Contains(
-		*err.InternalError.Message,
-		"the object has been modified")
+	if err.InternalError.Message == nil {
+		return false
+	}
+
+	return isRetriableErrorMessage(*err.InternalError.Message)
 }
 
 // slice represents a slice of []storagesnapshotv1.VolumeSnapshot