fix, podopslifecycle webhook waits for more events

pkg/controllers/utils/pod_utils.go

@@ -18,6 +18,7 @@ package utils
 
 import (
 	"encoding/json"
+	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -106,6 +107,20 @@ func IsPodServiceAvailable(pod *corev1.Pod) bool {
 	return exist
 }
 
+func GetProtectionFinalizers(pod *corev1.Pod) []string {
+	if pod == nil || pod.ObjectMeta.Finalizers == nil || len(pod.ObjectMeta.Finalizers) == 0 {
+		return nil
+	}
+
+	var finalizers []string
+	for _, f := range pod.ObjectMeta.Finalizers {
+		if strings.HasPrefix(f, v1alpha1.PodOperationProtectionFinalizerPrefix) {
+			finalizers = append(finalizers, f)
+		}
+	}
+	return finalizers
+}
+
 func IsExpectedFinalizerSatisfied(pod *corev1.Pod) (bool, map[string]string, error) {
 	satisfied := true
 	notSatisfiedFinalizers := make(map[string]string) // expected finalizers that are not satisfied

pkg/controllers/utils/pod_utils_test.go

@@ -18,6 +18,7 @@ package utils
 
 import (
 	"encoding/json"
+	"fmt"
 	"testing"
 
 	corev1 "k8s.io/api/core/v1"
@@ -235,6 +236,30 @@ func TestIsPodServiceAvailable(t *testing.T) {
 	}
 }
 
+func TestGetProtectionFinalizers(t *testing.T) {
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test",
+		},
+		Spec: corev1.PodSpec{},
+	}
+
+	if finalizers := GetProtectionFinalizers(pod); finalizers != nil {
+		t.Fatalf("expected failure")
+	}
+
+	protectionFinalizer := fmt.Sprintf("%s/%s", appsv1alpha1.PodOperationProtectionFinalizerPrefix, "finalizer1")
+	pod.ObjectMeta.Finalizers = []string{
+		protectionFinalizer,
+		"finalizer2",
+	}
+
+	finalizers := GetProtectionFinalizers(pod)
+	if len(finalizers) != 1 || finalizers[0] != protectionFinalizer {
+		t.Fatalf("expected failure")
+	}
+}
+
 func TestIsPodExpectedFinalizerSatisfied(t *testing.T) {
 	pod := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{

pkg/webhook/server/generic/pod/opslifecycle/mutating.go

@@ -36,7 +36,7 @@ func (lc *OpsLifecycle) Mutating(ctx context.Context, c client.Client, oldPod, n
 		return nil
 	}
 
-	// add readiness gate when pod is created
+	// Add readiness gate when pod is created
 	if operation == admissionv1.Create {
 		addReadinessGates(newPod, v1alpha1.ReadinessGatePodServiceReady)
 	}
@@ -50,14 +50,14 @@ func (lc *OpsLifecycle) Mutating(ctx context.Context, c client.Client, oldPod, n
 	var operatingCount, operateCount, operatedCount, completeCount int
 	var undoTypeToNumsMap = map[string]int{}
 	for id, labels := range newIDToLabelsMap {
-		if undoOperationType, ok := labels[v1alpha1.PodUndoOperationTypeLabelPrefix]; ok { // operation is canceled
+		if undoOperationType, ok := labels[v1alpha1.PodUndoOperationTypeLabelPrefix]; ok { // Operation is canceled
 			if _, ok := undoTypeToNumsMap[undoOperationType]; !ok {
 				undoTypeToNumsMap[undoOperationType] = 1
 			} else {
 				undoTypeToNumsMap[undoOperationType] = undoTypeToNumsMap[undoOperationType] + 1
 			}
 
-			// clean up these labels with id
+			// Clean up these labels with the ID
 			for _, v := range []string{v1alpha1.PodOperatingLabelPrefix,
 				v1alpha1.PodOperationTypeLabelPrefix,
 				v1alpha1.PodPreCheckLabelPrefix,
@@ -76,19 +76,18 @@ func (lc *OpsLifecycle) Mutating(ctx context.Context, c client.Client, oldPod, n
 			operatingCount++
 
 			if _, ok := labels[v1alpha1.PodPreCheckedLabelPrefix]; ok { // pre-checked
-				_, hasPrepare := labels[v1alpha1.PodPreparingLabelPrefix]
-				_, hasOperate := labels[v1alpha1.PodOperateLabelPrefix]
-
-				if !hasPrepare && !hasOperate {
+				_, hasPreparing := labels[v1alpha1.PodPreparingLabelPrefix]
+				if !hasPreparing {
 					delete(newPod.Labels, v1alpha1.PodServiceAvailableLabel)
 
-					lc.addLabelWithTime(newPod, fmt.Sprintf("%s/%s", v1alpha1.PodPreparingLabelPrefix, id)) // prepare
-				} else if !hasOperate {
-					if ready, _ := lc.readyToUpgrade(newPod); ready {
-						delete(newPod.Labels, fmt.Sprintf("%s/%s", v1alpha1.PodPreparingLabelPrefix, id))
+					lc.addLabelWithTime(newPod, fmt.Sprintf("%s/%s", v1alpha1.PodPreparingLabelPrefix, id)) // preparing
+				}
+
+				_, hasOperate := labels[v1alpha1.PodOperateLabelPrefix]
+				if !hasOperate && lc.readyToOperate(newPod) {
+					delete(newPod.Labels, fmt.Sprintf("%s/%s", v1alpha1.PodPreparingLabelPrefix, id))
 
-						lc.addLabelWithTime(newPod, fmt.Sprintf("%s/%s", v1alpha1.PodOperateLabelPrefix, id)) // operate
-					}
+					lc.addLabelWithTime(newPod, fmt.Sprintf("%s/%s", v1alpha1.PodOperateLabelPrefix, id)) // operate
 				}
 			} else {
 				if _, ok := labels[v1alpha1.PodPreCheckLabelPrefix]; !ok {
@@ -116,17 +115,17 @@ func (lc *OpsLifecycle) Mutating(ctx context.Context, c client.Client, oldPod, n
 	klog.Infof("pod: %s/%s, numOfIDs: %d, operatingCount: %d, operateCount: %d, operatedCount: %d, completeCount: %d", newPod.Namespace, newPod.Name, numOfIDs, operatingCount, operateCount, operatedCount, completeCount)
 
 	for t, num := range undoTypeToNumsMap {
-		if num == typeToNumsMap[t] { // reset the permission with type t if all operating with type t are canceled
+		if num == typeToNumsMap[t] { // Reset the permission with type t if all operating with type t are canceled
 			delete(newPod.Labels, fmt.Sprintf("%s/%s", v1alpha1.PodOperationPermissionLabelPrefix, t))
 		}
 	}
 
-	if operatingCount != 0 { // when operation is done, controller will remove operating label and operation type label
+	if operatingCount != 0 { // When operation is done, controller will remove operating label and operation type label
 		return nil
 	}
 
-	if completeCount == numOfIDs { // all operations are completed
-		satisfied, notSatisfiedFinalizers, err := controllerutils.IsExpectedFinalizerSatisfied(newPod) // whether all expected finalizers are satisfied
+	if completeCount == numOfIDs { // All operations are completed
+		satisfied, notSatisfiedFinalizers, err := controllerutils.IsExpectedFinalizerSatisfied(newPod) // Whether all expected finalizers are satisfied
 		if err != nil || !satisfied {
 			klog.Infof("pod: %s/%s, satisfied: %v, expectedFinalizer: %v, err: %v", newPod.Namespace, newPod.Name, satisfied, notSatisfiedFinalizers, err)
 			return err
@@ -146,7 +145,7 @@ func (lc *OpsLifecycle) Mutating(ctx context.Context, c client.Client, oldPod, n
 		return nil
 	}
 
-	if operateCount == numOfIDs { // all operations are going to be done
+	if operateCount == numOfIDs { // All operations are going to be done
 		oldIdToLabelsMap, _, err := podopslifecycle.PodIDAndTypesMap(oldPod)
 		if err != nil {
 			return err
@@ -172,7 +171,7 @@ func (lc *OpsLifecycle) Mutating(ctx context.Context, c client.Client, oldPod, n
 		}
 	}
 
-	if operatedCount == numOfIDs { // all operations are done
+	if operatedCount == numOfIDs { // All operations are done
 		for id, labels := range newIDToLabelsMap {
 			if _, ok := labels[v1alpha1.PodPostCheckLabelPrefix]; !ok {
 				lc.addLabelWithTime(newPod, fmt.Sprintf("%s/%s", v1alpha1.PodPostCheckLabelPrefix, id)) // post-check

pkg/webhook/server/generic/pod/opslifecycle/validating.go

@@ -41,7 +41,7 @@ func (lc *OpsLifecycle) Validating(ctx context.Context, c client.Client, oldPod,
 	expectedLabels := make(map[string]struct{})
 	foundLabels := make(map[string]struct{})
 	for label := range newPod.Labels {
-		for _, v := range pairLabelPrefixesMap { // labels must exist together and have the same id
+		for _, v := range pairLabelPrefixesMap { // Labels must exist together and have the same ID
 			if !strings.HasPrefix(label, v) {
 				continue
 			}
@@ -62,7 +62,7 @@ func (lc *OpsLifecycle) Validating(ctx context.Context, c client.Client, oldPod,
 		}
 
 		found := false
-		for v := range expectedLabels { // try to find the expected another label prefixes
+		for v := range expectedLabels { // Try to find the expected another label prefixes
 			if strings.HasPrefix(label, v) {
 				foundLabels[v] = struct{}{}
 				found = true
@@ -73,7 +73,7 @@ func (lc *OpsLifecycle) Validating(ctx context.Context, c client.Client, oldPod,
 			continue
 		}
 
-		for _, v := range coexistingLabelPrefixesMap { // labels must exist together
+		for _, v := range coexistingLabelPrefixesMap { // Labels must exist together
 			if !strings.HasPrefix(label, v) {
 				continue
 			}

pkg/webhook/server/generic/pod/opslifecycle/webhook.go

@@ -17,9 +17,7 @@ limitations under the License.
 package opslifecycle
 
 import (
-	"encoding/json"
 	"strconv"
-	"strings"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -33,7 +31,7 @@ const (
 )
 
 var (
-	// some labels must exist together and have the same id, and they are a pair
+	// Some labels must exist together and have the same ID
 	pairLabelPrefixesMap = map[string]string{
 		v1alpha1.PodOperatingLabelPrefix:     v1alpha1.PodOperationTypeLabelPrefix,
 		v1alpha1.PodOperationTypeLabelPrefix: v1alpha1.PodOperatingLabelPrefix,
@@ -42,27 +40,24 @@ var (
 		v1alpha1.PodDoneOperationTypeLabelPrefix: v1alpha1.PodOperatedLabelPrefix,
 	}
 
-	// some labels must exist together
+	// Some labels must exist together
 	coexistingLabelPrefixesMap = map[string]string{
 		v1alpha1.PodPreCheckedLabelPrefix:          v1alpha1.PodOperationPermissionLabelPrefix,
 		v1alpha1.PodOperationPermissionLabelPrefix: v1alpha1.PodPreCheckedLabelPrefix,
 	}
 )
 
-type ReadyToUpgrade func(pod *corev1.Pod) (bool, []string)
+type ReadyToOperate func(pod *corev1.Pod) bool
 type TimeLabelValue func() string
-type IsPodReady func(pod *corev1.Pod) bool
 
 type OpsLifecycle struct {
-	readyToUpgrade ReadyToUpgrade // for testing
-	isPodReady     IsPodReady
+	readyToOperate ReadyToOperate
 	timeLabelValue TimeLabelValue
 }
 
 func New() *OpsLifecycle {
 	return &OpsLifecycle{
-		readyToUpgrade: hasNoBlockingFinalizer,
-		isPodReady:     controllerutils.IsPodReady,
+		readyToOperate: readyToOperate,
 		timeLabelValue: func() string {
 			return strconv.FormatInt(time.Now().UnixNano(), 10)
 		},
@@ -91,56 +86,23 @@ func addReadinessGates(pod *corev1.Pod, conditionType corev1.PodConditionType) {
 	})
 }
 
-func hasNoBlockingFinalizer(pod *corev1.Pod) (bool, []string) {
+func readyToOperate(pod *corev1.Pod) bool {
 	if pod == nil {
-		return true, nil
+		return false
 	}
 
-	hasReadinessGate := false
-	if pod.Spec.ReadinessGates != nil {
-		for _, readinessGate := range pod.Spec.ReadinessGates {
-			if readinessGate.ConditionType == v1alpha1.ReadinessGatePodServiceReady {
-				hasReadinessGate = true
-				break
-			}
-		}
-	}
-	if !hasReadinessGate {
-		// if has no service-ready ReadinessGate, treat it as normal pod.
-		return true, nil
-	}
-
-	if pod.ObjectMeta.Finalizers == nil || len(pod.ObjectMeta.Finalizers) == 0 {
-		return true, nil
-	}
-
-	var finalizers []string
-	for _, f := range pod.ObjectMeta.Finalizers {
-		if strings.HasPrefix(f, v1alpha1.PodOperationProtectionFinalizerPrefix) {
-			finalizers = append(finalizers, f)
-		}
+	index, condition := controllerutils.GetPodCondition(&pod.Status, v1alpha1.ReadinessGatePodServiceReady)
+	if index == -1 {
+		return true
 	}
 
+	finalizers := controllerutils.GetProtectionFinalizers(pod)
 	if len(finalizers) > 0 {
-		return false, finalizers
-	}
-
-	return true, nil
-}
-
-func podAvailableConditions(pod *corev1.Pod) (*v1alpha1.PodAvailableConditions, error) {
-	if pod.Annotations == nil {
-		return nil, nil
-	}
-
-	anno, ok := pod.Annotations[v1alpha1.PodAvailableConditionsAnnotation]
-	if !ok {
-		return nil, nil
+		return false
 	}
 
-	availableConditions := &v1alpha1.PodAvailableConditions{}
-	if err := json.Unmarshal([]byte(anno), availableConditions); err != nil {
-		return nil, err
+	if condition.Status == corev1.ConditionFalse {
+		return true
 	}
-	return availableConditions, nil
+	return false
 }