[release-0.47] Timeout if Pending takes too long (#1027)

* Timeout if Pending takes too long Add LastUnavailableNodeCount timestamp to avoid getting stuck at Pending forever. Signed-off-by: Radim Hrazdil <[email protected]> * handler, container: Use centos stream 8 Signed-off-by: Radim Hrazdil <[email protected]>
nmstate · Mar 28, 2022 · 3a832ad · 3a832ad
1 parent c2b607d
commit 3a832ad
Show file tree

Hide file tree

Showing 7 changed files with 64 additions and 33 deletions.
diff --git a/api/shared/nodenetworkconfigurationpolicy_types.go b/api/shared/nodenetworkconfigurationpolicy_types.go
@@ -1,6 +1,9 @@
 package shared
 
-import "k8s.io/apimachinery/pkg/util/intstr"
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+)
 
 // NodeNetworkConfigurationPolicySpec defines the desired state of NodeNetworkConfigurationPolicy
 type NodeNetworkConfigurationPolicySpec struct {
@@ -28,6 +31,9 @@ type NodeNetworkConfigurationPolicyStatus struct {
 	// processing a NodeNetworkConfigurationPolicy
 	// +optional
 	UnavailableNodeCount int `json:"unavailableNodeCount,omitempty" optional:"true"`
+	// LastUnavailableNodeCountUpdate is time of the last UnavailableNodeCount update
+	// +optional
+	LastUnavailableNodeCountUpdate *metav1.Time `json:"lastUnavailableNodeCountUpdate,omitempty" optional:"true"`
 }
 
 const (

diff --git a/build/Dockerfile b/build/Dockerfile
@@ -1,4 +1,4 @@
-FROM centos:8
+FROM quay.io/centos/centos:stream8
 ARG NMSTATE_COPR_REPO
 ARG NM_COPR_REPO
 RUN dnf install -b -y dnf-plugins-core && \

diff --git a/controllers/nodenetworkconfigurationpolicy_controller.go b/controllers/nodenetworkconfigurationpolicy_controller.go
@@ -38,6 +38,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
@@ -52,6 +53,7 @@ import (
 	nmstate "github.com/nmstate/kubernetes-nmstate/pkg/helper"
 	"github.com/nmstate/kubernetes-nmstate/pkg/node"
 	"github.com/nmstate/kubernetes-nmstate/pkg/policyconditions"
+	"github.com/nmstate/kubernetes-nmstate/pkg/probe"
 	"github.com/nmstate/kubernetes-nmstate/pkg/selectors"
 )
 
@@ -178,7 +180,7 @@ func (r *NodeNetworkConfigurationPolicyReconciler) Reconcile(ctx context.Context
 		return ctrl.Result{}, nil
 	}
 
-	if r.shouldIncrementUnavailableNodeCount(previousConditions) {
+	if r.shouldIncrementUnavailableNodeCount(instance, previousConditions) {
 		err = r.incrementUnavailableNodeCount(instance)
 		if err != nil {
 			if apierrors.IsConflict(err) {
@@ -313,8 +315,9 @@ func (r *NodeNetworkConfigurationPolicyReconciler) waitEnactmentCreated(enactmen
 	return pollErr
 }
 
-func (r *NodeNetworkConfigurationPolicyReconciler) shouldIncrementUnavailableNodeCount(conditions *nmstateapi.ConditionList) bool {
-	return !enactmentstatus.IsProgressing(conditions)
+func (r *NodeNetworkConfigurationPolicyReconciler) shouldIncrementUnavailableNodeCount(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy, conditions *nmstateapi.ConditionList) bool {
+	return !enactmentstatus.IsProgressing(conditions) && (policy.Status.LastUnavailableNodeCountUpdate == nil ||
+		time.Since(policy.Status.LastUnavailableNodeCountUpdate.Time) < (nmstate.DesiredStateConfigurationTimeout+probe.ProbesTotalTimeout))
 }
 
 func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) error {
@@ -330,6 +333,7 @@ func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount
 	if policy.Status.UnavailableNodeCount >= maxUnavailable {
 		return apierrors.NewConflict(schema.GroupResource{Resource: "nodenetworkconfigurationpolicies"}, policy.Name, fmt.Errorf("maximal number of %d nodes are already processing policy configuration", policy.Status.UnavailableNodeCount))
 	}
+	policy.Status.LastUnavailableNodeCountUpdate = &metav1.Time{Time: time.Now()}
 	policy.Status.UnavailableNodeCount += 1
 	err = r.Client.Status().Update(context.TODO(), policy)
 	if err != nil {
@@ -349,6 +353,7 @@ func (r *NodeNetworkConfigurationPolicyReconciler) decrementUnavailableNodeCount
 		if instance.Status.UnavailableNodeCount <= 0 {
 			return fmt.Errorf("no unavailable nodes")
 		}
+		instance.Status.LastUnavailableNodeCountUpdate = &metav1.Time{Time: time.Now()}
 		instance.Status.UnavailableNodeCount -= 1
 		return r.Client.Status().Update(context.TODO(), instance)
 	})

diff --git a/controllers/nodenetworkconfigurationpolicy_controller_test.go b/controllers/nodenetworkconfigurationpolicy_controller_test.go
@@ -73,10 +73,11 @@ var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func()
 	)
 
 	type incrementUnavailableNodeCountCase struct {
-		currentUnavailableNodeCount  int
-		expectedUnavailableNodeCount int
-		expectedReconcileResult      ctrl.Result
-		previousEnactmentConditions  func(*shared.ConditionList, string)
+		currentUnavailableNodeCount      int
+		expectedUnavailableNodeCount     int
+		expectedReconcileResult          ctrl.Result
+		previousEnactmentConditions      func(*shared.ConditionList, string)
+		shouldUpdateUnavailableNodeCount bool
 	}
 	DescribeTable("when claimNodeRunningUpdate is called and",
 		func(c incrementUnavailableNodeCountCase) {
@@ -132,34 +133,41 @@ var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func()
 			obtainedNNCP := nmstatev1beta1.NodeNetworkConfigurationPolicy{}
 			cl.Get(context.TODO(), types.NamespacedName{Name: nncp.Name}, &obtainedNNCP)
 			Expect(obtainedNNCP.Status.UnavailableNodeCount).To(Equal(c.expectedUnavailableNodeCount))
+			if c.shouldUpdateUnavailableNodeCount {
+				Expect(obtainedNNCP.Status.LastUnavailableNodeCountUpdate).ToNot(BeNil())
+			}
 		},
 		Entry("No node applying policy with empty enactment, should succeed incrementing UnavailableNodeCount",
 			incrementUnavailableNodeCountCase{
-				currentUnavailableNodeCount:  0,
-				expectedUnavailableNodeCount: 0,
-				previousEnactmentConditions:  func(*shared.ConditionList, string) {},
-				expectedReconcileResult:      ctrl.Result{},
+				currentUnavailableNodeCount:      0,
+				expectedUnavailableNodeCount:     0,
+				previousEnactmentConditions:      func(*shared.ConditionList, string) {},
+				expectedReconcileResult:          ctrl.Result{},
+				shouldUpdateUnavailableNodeCount: true,
 			}),
 		Entry("No node applying policy with progressing enactment, should succeed incrementing UnavailableNodeCount",
 			incrementUnavailableNodeCountCase{
-				currentUnavailableNodeCount:  0,
-				expectedUnavailableNodeCount: 0,
-				previousEnactmentConditions:  conditions.SetProgressing,
-				expectedReconcileResult:      ctrl.Result{},
+				currentUnavailableNodeCount:      0,
+				expectedUnavailableNodeCount:     0,
+				previousEnactmentConditions:      conditions.SetProgressing,
+				expectedReconcileResult:          ctrl.Result{},
+				shouldUpdateUnavailableNodeCount: false,
 			}),
 		Entry("One node applying policy with empty enactment, should conflict incrementing UnavailableNodeCount",
 			incrementUnavailableNodeCountCase{
-				currentUnavailableNodeCount:  1,
-				expectedUnavailableNodeCount: 1,
-				previousEnactmentConditions:  func(*shared.ConditionList, string) {},
-				expectedReconcileResult:      ctrl.Result{RequeueAfter: nodeRunningUpdateRetryTime},
+				currentUnavailableNodeCount:      1,
+				expectedUnavailableNodeCount:     1,
+				previousEnactmentConditions:      func(*shared.ConditionList, string) {},
+				expectedReconcileResult:          ctrl.Result{RequeueAfter: nodeRunningUpdateRetryTime},
+				shouldUpdateUnavailableNodeCount: false,
 			}),
 		Entry("One node applying policy with Progressing enactment, should succeed incrementing UnavailableNodeCount",
 			incrementUnavailableNodeCountCase{
-				currentUnavailableNodeCount:  1,
-				expectedUnavailableNodeCount: 0,
-				previousEnactmentConditions:  conditions.SetProgressing,
-				expectedReconcileResult:      ctrl.Result{},
+				currentUnavailableNodeCount:      1,
+				expectedUnavailableNodeCount:     0,
+				previousEnactmentConditions:      conditions.SetProgressing,
+				expectedReconcileResult:          ctrl.Result{},
+				shouldUpdateUnavailableNodeCount: false,
 			}),
 	)
 })
diff --git a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml
@@ -90,6 +90,11 @@ spec:
                   - type
                   type: object
                 type: array
+              lastUnavailableNodeCountUpdate:
+                description: LastUnavailableNodeCountUpdate is time of the last UnavailableNodeCount
+                  update
+                format: date-time
+                type: string
               unavailableNodeCount:
                 description: UnavailableNodeCount represents the total number of potentially
                   unavailable nodes that are processing a NodeNetworkConfigurationPolicy
@@ -172,6 +177,11 @@ spec:
                   - type
                   type: object
                 type: array
+              lastUnavailableNodeCountUpdate:
+                description: LastUnavailableNodeCountUpdate is time of the last UnavailableNodeCount
+                  update
+                format: date-time
+                type: string
               unavailableNodeCount:
                 description: UnavailableNodeCount represents the total number of potentially
                   unavailable nodes that are processing a NodeNetworkConfigurationPolicy

diff --git a/pkg/helper/client.go b/pkg/helper/client.go
@@ -22,9 +22,14 @@ var (
 	log = logf.Log.WithName("client")
 )
 
-const defaultGwRetrieveTimeout = 120 * time.Second
-const defaultGwProbeTimeout = 120 * time.Second
-const apiServerProbeTimeout = 120 * time.Second
+const (
+	defaultGwProbeTimeout = 120 * time.Second
+	apiServerProbeTimeout = 120 * time.Second
+	// DesiredStateConfigurationTimeout doubles the default gw ping probe and API server
+	// connectivity check timeout to ensure the Checkpoint is alive before rolling it back
+	// https://nmstate.github.io/cli_guide#manual-transaction-control
+	DesiredStateConfigurationTimeout = (defaultGwProbeTimeout + apiServerProbeTimeout) * 2
+)
 
 func InitializeNodeNetworkState(client client.Client, node *corev1.Node) (*nmstatev1beta1.NodeNetworkState, error) {
 	ownerRefList := []metav1.OwnerReference{{Name: node.ObjectMeta.Name, Kind: "Node", APIVersion: "v1", UID: node.UID}}
@@ -112,11 +117,7 @@ func ApplyDesiredState(client client.Client, desiredState shared.State) (string,
 	// working fine after apply
 	probes := probe.Select(client)
 
-	// commit timeout doubles the default gw ping probe and check API server
-	// connectivity timeout, to
-	// ensure the Checkpoint is alive before rolling it back
-	// https://nmstate.github.io/cli_guide#manual-transaction-control
-	setOutput, err := nmstatectl.Set(desiredState, (defaultGwProbeTimeout+apiServerProbeTimeout)*2)
+	setOutput, err := nmstatectl.Set(desiredState, DesiredStateConfigurationTimeout)
 	if err != nil {
 		return setOutput, err
 	}

diff --git a/pkg/probe/probes.go b/pkg/probe/probes.go
@@ -40,6 +40,7 @@ const (
 	defaultDnsProbeTimeout    = 120 * time.Second
 	apiServerProbeTimeout     = 120 * time.Second
 	nodeReadinessProbeTimeout = 120 * time.Second
+	ProbesTotalTimeout        = defaultGwRetrieveTimeout + defaultDnsProbeTimeout + defaultDnsProbeTimeout + apiServerProbeTimeout + nodeReadinessProbeTimeout
 )
 
 func currentStateAsGJson() (gjson.Result, error) {