PWX-34411 Allow overriding the min available value of px-storage PDB

Signed-off-by: Piyush Nimbalkar <pnimbalkar@purestorage.com>
libopenstorage · Dec 5, 2023 · bbf86b4 · bbf86b4
1 parent 700e3bf
commit bbf86b4
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 29 deletions.
diff --git a/drivers/storage/portworx/component/disruption_budget.go b/drivers/storage/portworx/component/disruption_budget.go
@@ -100,31 +100,42 @@ func (c *disruptionBudget) createPortworxPodDisruptionBudget(
 	cluster *corev1.StorageCluster,
 	ownerRef *metav1.OwnerReference,
 ) error {
-	var err error
-	c.sdkConn, err = pxutil.GetPortworxConn(c.sdkConn, c.k8sClient, cluster.Namespace)
+	userProvidedMinValue, err := pxutil.MinAvailableForStoragePDB(cluster)
 	if err != nil {
-		return err
+		logrus.Warnf("Invalid value for annotation %s: %v", pxutil.AnnotationStoragePodDisruptionBudget, err)
+		userProvidedMinValue = -1
 	}
 
-	storageNodesCount, err := pxutil.CountStorageNodes(cluster, c.sdkConn, c.k8sClient)
-	if err != nil {
-		c.closeSdkConn()
-		return err
+	var minAvailable int
+	if err != nil || userProvidedMinValue < 0 {
+		c.sdkConn, err = pxutil.GetPortworxConn(c.sdkConn, c.k8sClient, cluster.Namespace)
+		if err != nil {
+			return err
+		}
+
+		storageNodesCount, err := pxutil.CountStorageNodes(cluster, c.sdkConn, c.k8sClient)
+		if err != nil {
+			c.closeSdkConn()
+			return err
+		}
+		minAvailable = storageNodesCount - 1
+	} else {
+		minAvailable = userProvidedMinValue
 	}
 
-	// Create PDB only if there are at least 3 nodes. With 2 nodes are less, if 1
+	// Create PDB only if there are at least 3 nodes. With 2 nodes or less, if 1
 	// node goes down Portworx will lose quorum anyway. Such clusters would be
 	// non-prod clusters and there is no point in blocking the evictions.
-	if storageNodesCount > 2 {
-		minAvailable := intstr.FromInt(storageNodesCount - 1)
+	if minAvailable > 1 {
+		minAvailableIntStr := intstr.FromInt(minAvailable)
 		pdb := &policyv1.PodDisruptionBudget{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:            StoragePodDisruptionBudgetName,
 				Namespace:       cluster.Namespace,
 				OwnerReferences: []metav1.OwnerReference{*ownerRef},
 			},
 			Spec: policyv1.PodDisruptionBudgetSpec{
-				MinAvailable: &minAvailable,
+				MinAvailable: &minAvailableIntStr,
 				Selector: &metav1.LabelSelector{
 					MatchLabels: map[string]string{
 						constants.LabelKeyClusterName: cluster.Name,

diff --git a/drivers/storage/portworx/components_test.go b/drivers/storage/portworx/components_test.go
@@ -13,29 +13,17 @@ import (
 	"testing"
 	"time"
 
-	ocpconfig "github.com/openshift/api/config/v1"
-
 	"github.com/golang-jwt/jwt/v4"
 	"github.com/golang/mock/gomock"
-	osdapi "github.com/libopenstorage/openstorage/api"
-	"github.com/libopenstorage/operator/drivers/storage/portworx/component"
-	"github.com/libopenstorage/operator/drivers/storage/portworx/manifest"
-	pxutil "github.com/libopenstorage/operator/drivers/storage/portworx/util"
-	corev1 "github.com/libopenstorage/operator/pkg/apis/core/v1"
-	"github.com/libopenstorage/operator/pkg/constants"
-	"github.com/libopenstorage/operator/pkg/mock"
-	"github.com/libopenstorage/operator/pkg/util"
-	k8sutil "github.com/libopenstorage/operator/pkg/util/k8s"
-	testutil "github.com/libopenstorage/operator/pkg/util/test"
+	ocpconfig "github.com/openshift/api/config/v1"
 	ocp_secv1 "github.com/openshift/api/security/v1"
-	"github.com/sirupsen/logrus"
-	"github.com/stretchr/testify/assert"
-	"golang.org/x/sys/unix"
-
 	apiextensionsops "github.com/portworx/sched-ops/k8s/apiextensions"
 	coreops "github.com/portworx/sched-ops/k8s/core"
 	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/unix"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	policyv1 "k8s.io/api/policy/v1"
@@ -56,6 +44,17 @@ import (
 	"k8s.io/client-go/tools/record"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	osdapi "github.com/libopenstorage/openstorage/api"
+	"github.com/libopenstorage/operator/drivers/storage/portworx/component"
+	"github.com/libopenstorage/operator/drivers/storage/portworx/manifest"
+	pxutil "github.com/libopenstorage/operator/drivers/storage/portworx/util"
+	corev1 "github.com/libopenstorage/operator/pkg/apis/core/v1"
+	"github.com/libopenstorage/operator/pkg/constants"
+	"github.com/libopenstorage/operator/pkg/mock"
+	"github.com/libopenstorage/operator/pkg/util"
+	k8sutil "github.com/libopenstorage/operator/pkg/util/k8s"
+	testutil "github.com/libopenstorage/operator/pkg/util/test"
 )
 
 const (
@@ -11471,7 +11470,21 @@ func TestPodDisruptionBudgetEnabled(t *testing.T) {
 	err = testutil.Get(k8sClient, storagePDB, component.StoragePodDisruptionBudgetName, cluster.Namespace)
 	require.True(t, errors.IsNotFound(err))
 
-	// TestCase: Create storage PDB if total nodes with storage is at least 3
+	// TestCase: Do not create storage PDB if storage pod annotation value is less than 2
+	cluster.Annotations = map[string]string{
+		pxutil.AnnotationStoragePodDisruptionBudget: "1",
+	}
+
+	err = driver.PreInstall(cluster)
+	require.NoError(t, err)
+
+	storagePDB = &policyv1.PodDisruptionBudget{}
+	err = testutil.Get(k8sClient, storagePDB, component.StoragePodDisruptionBudgetName, cluster.Namespace)
+	require.True(t, errors.IsNotFound(err))
+
+	// TestCase: Create storage PDB if total nodes with storage is at least 3.
+	// Also, ignore the annotation if the value is an invalid integer
+	cluster.Annotations[pxutil.AnnotationStoragePodDisruptionBudget] = "invalid"
 	expectedNodeEnumerateResp.Nodes = []*osdapi.StorageNode{
 		{Pools: []*osdapi.StoragePool{{ID: 1}}, SchedulerNodeName: "node1"},
 		{Pools: []*osdapi.StoragePool{{ID: 2}}, SchedulerNodeName: "node2"},
@@ -11492,7 +11505,9 @@ func TestPodDisruptionBudgetEnabled(t *testing.T) {
 	require.Equal(t, cluster.Name, storagePDB.Spec.Selector.MatchLabels[constants.LabelKeyClusterName])
 	require.Equal(t, constants.LabelValueTrue, storagePDB.Spec.Selector.MatchLabels[constants.LabelKeyStoragePod])
 
-	// TestCase: Update storage PDB if count of nodes with storage changes
+	// TestCase: Update storage PDB if count of nodes with storage changes.
+	// Also, ignore the annotation if the value is an invalid integer
+	cluster.Annotations[pxutil.AnnotationStoragePodDisruptionBudget] = "still_invalid"
 	expectedNodeEnumerateResp.Nodes = []*osdapi.StorageNode{
 		{Pools: []*osdapi.StoragePool{{ID: 1}}, SchedulerNodeName: "node1"},
 		{Pools: []*osdapi.StoragePool{{ID: 2}}, SchedulerNodeName: "node2"},

diff --git a/drivers/storage/portworx/util/util.go b/drivers/storage/portworx/util/util.go
@@ -123,6 +123,9 @@ const (
 	AnnotationRunOnMaster = pxAnnotationPrefix + "/run-on-master"
 	// AnnotationPodDisruptionBudget annotation indicating whether to create pod disruption budgets
 	AnnotationPodDisruptionBudget = pxAnnotationPrefix + "/pod-disruption-budget"
+	// AnnotationStoragePodDisruptionBudget annotation to specify the min available value of the px-storage
+	// pod disruption budget
+	AnnotationStoragePodDisruptionBudget = pxAnnotationPrefix + "/storage-pdb-min-available"
 	// AnnotationPodSecurityPolicy annotation indicating whether to enable creation
 	// of pod security policies
 	AnnotationPodSecurityPolicy = pxAnnotationPrefix + "/pod-security-policy"
@@ -1062,6 +1065,13 @@ func GetClusterID(cluster *corev1.StorageCluster) string {
 	return cluster.Name
 }
 
+func MinAvailableForStoragePDB(cluster *corev1.StorageCluster) (int, error) {
+	if cluster.Annotations[AnnotationStoragePodDisruptionBudget] != "" {
+		return strconv.Atoi(cluster.Annotations[AnnotationStoragePodDisruptionBudget])
+	}
+	return -1, nil
+}
+
 // CountStorageNodes counts how many px storage node are there on given k8s cluster,
 // use this to count number of storage pods as well
 func CountStorageNodes(