[release-0.18] Backport #1052 and #1054 (#1059)

* Fix for test flake when sync waiting and an intermediate error occurs (#1052)

* fix: Cancel error timer on sync wait when a non-error event arrives

* chore: Added

* Add WithLabel list filter to serving client lib (#1054)

* add WithLabel list filter

Signed-off-by: Zbynek Roubalik <[email protected]>

* adding tests

Signed-off-by: Zbynek Roubalik <[email protected]>

* update changelog

Signed-off-by: Zbynek Roubalik <[email protected]>

* Update CHANGELOG

Co-authored-by: Roland Huß <[email protected]>
Co-authored-by: Zbynek Roubalik <[email protected]>

CHANGELOG.adoc

@@ -12,6 +12,22 @@
 | https://github.com/knative/client/pull/[#]
 ////
 
+## v0.18.1 (2020-10-12)
+
+[cols="1,10,3", options="header", width="100%"]
+|===
+| | Description | PR
+
+| 🎁
+| Add WithLabel list filter to serving client lib
+| https://github.com/knative/client/pull/1054[#1054]
+
+| 🐛
+| Fix for test flake when sync waiting and an intermediate error occurs
+| https://github.com/knative/client/pull/1052[#1052]
+
+|===
+
 ## v0.18.0 (2020-10-07)
 
 [cols="1,10,3", options="header", width="100%"]

lib/test/result_collector.go

@@ -98,10 +98,15 @@ func (c *KnRunResultCollector) AddDump(kind string, name string, namespace strin
 // DumpIfFailed logs if collector failed
 func (c *KnRunResultCollector) DumpIfFailed() {
 	if c.t.Failed() {
-		c.t.Log(c.errorDetails())
+		c.Dump()
 	}
 }
 
+// Dump prints out the collected output and logs
+func (c *KnRunResultCollector) Dump() {
+	c.t.Log(c.errorDetails())
+}
+
 // Private
 
 func (c *KnRunResultCollector) errorDetails() string {

pkg/serving/v1/client.go

@@ -141,6 +141,13 @@ func WithService(service string) ListConfig {
 	}
 }
 
+// WithLabel filters on the provided label
+func WithLabel(labelKey, labelValue string) ListConfig {
+	return func(lo *listConfigCollector) {
+		lo.Labels[labelKey] = labelValue
+	}
+}
+
 type knServingClient struct {
 	client    clientv1.ServingV1Interface
 	namespace string

pkg/serving/v1/client_mock_test.go

@@ -34,6 +34,7 @@ func TestMockKnClient(t *testing.T) {
 	// Record all services
 	recorder.GetService("hello", nil, nil)
 	recorder.ListServices(mock.Any(), nil, nil)
+	recorder.ListServices(mock.Any(), nil, nil)
 	recorder.CreateService(&servingv1.Service{}, nil)
 	recorder.UpdateService(&servingv1.Service{}, nil)
 	recorder.DeleteService("hello", time.Duration(10)*time.Second, nil)
@@ -48,6 +49,7 @@ func TestMockKnClient(t *testing.T) {
 	// Call all services
 	client.GetService("hello")
 	client.ListServices(WithName("blub"))
+	client.ListServices(WithLabel("foo", "bar"))
 	client.CreateService(&servingv1.Service{})
 	client.UpdateService(&servingv1.Service{})
 	client.DeleteService("hello", time.Duration(10)*time.Second)

pkg/serving/v1/client_test.go

@@ -85,24 +85,45 @@ func TestListService(t *testing.T) {
 	serving, client := setup()
 
 	t.Run("list service returns a list of services", func(t *testing.T) {
+		labelKey := "labelKey"
+		labelValue := "labelValue"
+		labels := map[string]string{labelKey: labelValue}
+		incorrectLabels := map[string]string{"foo": "bar"}
+
 		service1 := newService("service-1")
 		service2 := newService("service-2")
+		service3 := newService("service-3-with-label")
+		service3.Labels = labels
+		service4 := newService("service-4-with-label")
+		service4.Labels = labels
+		service5 := newService("service-5-with-incorrect-label")
+		service5.Labels = incorrectLabels
 
 		serving.AddReactor("list", "services",
 			func(a clienttesting.Action) (bool, runtime.Object, error) {
 				assert.Equal(t, testNamespace, a.GetNamespace())
-				return true, &servingv1.ServiceList{Items: []servingv1.Service{*service1, *service2}}, nil
+				return true, &servingv1.ServiceList{Items: []servingv1.Service{*service1, *service2, *service3, *service4, *service5}}, nil
 			})
 
 		listServices, err := client.ListServices()
 		assert.NilError(t, err)
-		assert.Assert(t, len(listServices.Items) == 2)
+		assert.Assert(t, len(listServices.Items) == 5)
 		assert.Equal(t, listServices.Items[0].Name, "service-1")
 		assert.Equal(t, listServices.Items[1].Name, "service-2")
 		validateGroupVersionKind(t, listServices)
 		validateGroupVersionKind(t, &listServices.Items[0])
 		validateGroupVersionKind(t, &listServices.Items[1])
+
+		listFilteredServices, err := client.ListServices(WithLabel(labelKey, labelValue))
+		assert.NilError(t, err)
+		assert.Assert(t, len(listFilteredServices.Items) == 2)
+		assert.Equal(t, listFilteredServices.Items[0].Name, "service-3-with-label")
+		assert.Equal(t, listFilteredServices.Items[1].Name, "service-4-with-label")
+		validateGroupVersionKind(t, listFilteredServices)
+		validateGroupVersionKind(t, &listFilteredServices.Items[0])
+		validateGroupVersionKind(t, &listFilteredServices.Items[1])
 	})
+
 }
 
 func TestCreateService(t *testing.T) {

pkg/wait/wait_for_ready.go

@@ -135,6 +135,12 @@ func (w *waitForReadyConfig) Wait(name string, options Options, msgCallback Mess
 	}
 }
 
+// waitForReadyCondition waits until the status condition "Ready" is set to true (good path) or return an error
+// when the "Ready" condition is set to false. An error is also returned when the given timeout is reached (plus the
+// return value of timeoutReached is set to true in this case).
+// An errorWindow can be specified which takes into account of intermediate "false" ready conditions. So before returning
+// an error, this methods waits for the errorWindow duration and if an "True" or "Unknown" event arrives in the meantime
+// for the "Ready" condition, then the method continues to wait.
 func (w *waitForReadyConfig) waitForReadyCondition(start time.Time, name string, timeout time.Duration, errorWindow time.Duration, msgCallback MessageCallback) (retry bool, timeoutReached bool, err error) {
 
 	watcher, err := w.watchMaker(name, timeout)
@@ -143,7 +149,7 @@ func (w *waitForReadyConfig) waitForReadyCondition(start time.Time, name string,
 	}
 	defer watcher.Stop()
 
-	// channel used to transport the error
+	// channel used to transport the error that has been received
 	errChan := make(chan error)
 
 	var errorTimer *time.Timer
@@ -152,14 +158,18 @@ func (w *waitForReadyConfig) waitForReadyCondition(start time.Time, name string,
 	defer (func() {
 		if errorTimer != nil {
 			errorTimer.Stop()
+			errorTimer = nil
 		}
 	})()
 
 	for {
 		select {
 		case <-time.After(timeout):
+			// We reached a timeout without receiving a "Ready" == "True" event
 			return false, true, nil
 		case err = <-errChan:
+			// The error timer fired and we have not received a recovery event ("True" / "Unknown") in the
+			// meantime. So the error status is considered to be final.
 			return false, false, err
 		case event, ok := <-watcher.ResultChan():
 			if !ok || event.Object == nil {
@@ -197,6 +207,7 @@ func (w *waitForReadyConfig) waitForReadyCondition(start time.Time, name string,
 				if cond.Type == apis.ConditionReady {
 					switch cond.Status {
 					case corev1.ConditionTrue:
+						// Any error timer running will be cancelled by the defer method that has been set above
 						return false, false, nil
 					case corev1.ConditionFalse:
 						// Fire up a timer waiting for the error window duration to still allow to reconcile
@@ -209,6 +220,14 @@ func (w *waitForReadyConfig) waitForReadyCondition(start time.Time, name string,
 								errChan <- err
 							})
 						}
+					case corev1.ConditionUnknown:
+						// If an errorTimer is triggered because of a previous "False" event, but now
+						// we received an "Unknown" event during the error window, cancel the error timer
+						// to avoid to receive an error signal.
+						if errorTimer != nil {
+							errorTimer.Stop()
+							errorTimer = nil
+						}
 					}
 					if cond.Message != "" {
 						msgCallback(time.Since(start), cond.Message)