Repository: knative-sandbox/net-kourier
Pull request #347 (2020-11-10 12:37:50)

Fix cluster-cache test to be correct and quicker
The cluster cache test currently does not test what we actually want to test. It instead uses a parameter that will never be set in production.

This fixes that and also makes the test considerably quicker. It used to take 2 seconds for no apparent reason.

/assign @jmprusi @davidor 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/net-kourier/pull/347#" title="Author self-approved">markusthoemmes</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fnet-kourier).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/net-kourier/blob/master/OWNERS)~~ [markusthoemmes]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/347?src=pr&el=h1) Report
> Merging [#347](https://codecov.io/gh/knative-sandbox/net-kourier/pull/347?src=pr&el=desc) (d30e953) into [master](https://codecov.io/gh/knative-sandbox/net-kourier/commit/19cdcdbc7837d9e71fb2838d23f6740093c758d4?el=desc) (19cdcdb) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/net-kourier/pull/347/graphs/tree.svg?width=650&height=150&src=pr&token=KmlRo23zX4)](https://codecov.io/gh/knative-sandbox/net-kourier/pull/347?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #347   +/-   ##
=======================================
  Coverage   78.79%   78.79%           
=======================================
  Files          14       14           
  Lines         533      533           
=======================================
  Hits          420      420           
  Misses         84       84           
  Partials       29       29           
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/net-kourier/pull/347?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/generator/cluster\_cache.go](https://codecov.io/gh/knative-sandbox/net-kourier/pull/347/diff?src=pr&el=tree#diff-cGtnL2dlbmVyYXRvci9jbHVzdGVyX2NhY2hlLmdv) | `85.71% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/347?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/347?src=pr&el=footer). Last update [19cdcdb...d30e953](https://codecov.io/gh/knative-sandbox/net-kourier/pull/347?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/net-kourier/pull/347#pullrequestreview-528129527" title="LGTM">davidor</a>*, *<a href="https://github.com/knative-sandbox/net-kourier/pull/347#" title="Author self-approved">markusthoemmes</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fnet-kourier).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/net-kourier/blob/master/OWNERS)~~ [davidor,markusthoemmes]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/generator/cluster_cache.go b/pkg/generator/cluster_cache.go
index 3b3fa96d..7c4c2aff 100644
--- a/pkg/generator/cluster_cache.go
+++ b/pkg/generator/cluster_cache.go
@@ -46,13 +46,13 @@ import (
 )
 
 const (
-	clusterExpiration      = 15 * time.Second
-	defaultExpiration      = gocache.NoExpiration
+	defaultExpiration      = 15 * time.Second
 	defaultCleanupInterval = 1 * time.Minute
 )
 
 type ClustersCache struct {
-	clusters *gocache.Cache
+	clusterExpiration time.Duration
+	clusters          *gocache.Cache
 }
 
 func newClustersCache() *ClustersCache {
@@ -60,19 +60,19 @@ func newClustersCache() *ClustersCache {
 }
 
 func newClustersCacheWithExpAndCleanupIntervals(expiration time.Duration, cleanupInterval time.Duration) *ClustersCache {
-	goCache := gocache.New(expiration, cleanupInterval)
-	return &ClustersCache{clusters: goCache}
+	goCache := gocache.New(gocache.NoExpiration, cleanupInterval)
+	return &ClustersCache{clusters: goCache, clusterExpiration: expiration}
 }
 
 func (cc *ClustersCache) set(cluster *v2.Cluster, ingressName string, ingressNamespace string) {
 	key := key(cluster.Name, ingressName, ingressNamespace)
-	cc.clusters.Set(key, cluster, defaultExpiration)
+	cc.clusters.Set(key, cluster, gocache.NoExpiration)
 }
 
 func (cc *ClustersCache) setExpiration(clusterName string, ingressName string, ingressNamespace string) {
 	key := key(clusterName, ingressName, ingressNamespace)
 	if cluster, ok := cc.clusters.Get(key); ok {
-		cc.clusters.Set(key, cluster, clusterExpiration)
+		cc.clusters.Set(key, cluster, cc.clusterExpiration)
 	}
 }
 


Test patch:
diff --git a/pkg/generator/cluster_cache_test.go b/pkg/generator/cluster_cache_test.go
index ea0a94b1..ddf018d1 100644
--- a/pkg/generator/cluster_cache_test.go
+++ b/pkg/generator/cluster_cache_test.go
@@ -24,6 +24,7 @@ import (
 	envoy_api_v2 "github.com/envoyproxy/go-control-plane/envoy/api/v2"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
+	"k8s.io/apimachinery/pkg/util/wait"
 )
 
 var testCluster1 = envoy_api_v2.Cluster{
@@ -64,11 +65,22 @@ func TestSetSeveralClusters(t *testing.T) {
 }
 
 func TestClustersExpire(t *testing.T) {
-	cleanupInterval := time.Second
-	cache := newClustersCacheWithExpAndCleanupIntervals(time.Second, cleanupInterval)
+	interval := 10 * time.Millisecond
+	cache := newClustersCacheWithExpAndCleanupIntervals(interval, interval)
+	cache.set(&testCluster1, "some_ingress_name", "some_ingress_namespace")
+	assert.Assert(t, is.Len(cache.list(), 1))
+
+	// Wait for twice the interval and assert that the cluster is still there.
+	time.Sleep(2 * interval)
+	assert.Assert(t, is.Len(cache.list(), 1))
+
+	// Mark the cluster to be expired.
 	cache.setExpiration(testCluster1.Name, "some_ingress_name", "some_ingress_namespace")
-	time.Sleep(cleanupInterval + time.Second)
 
+	// The cluster should eventually disappear.
+	wait.PollImmediate(interval, 5*time.Second, func() (bool, error) {
+		return len(cache.list()) == 0, nil
+	})
 	assert.Assert(t, is.Len(cache.list(), 0))
 }