Repository: kubernetes-sigs/gateway-api-inference-extension

Issue:
code is using time.Sleep when doing periodic task
in some places in the code, there is a periodic task needs to be executed and the code is using time.sleep which is a bad practice. 
for example, time.sleep may cause delays when receiving SIGTERM - if the go routine started the sleep section we will need to wait until sleep competes and only then the go routine will exist (due to context close).

the Go way for doing periodic task endlessly (until a cancel/close is received) is by using ticker. 

Discussion:

Solution patch:
diff --git a/pkg/epp/backend/metrics/logger.go b/pkg/epp/backend/metrics/logger.go
index d71dc3fa..d9a93027 100644
--- a/pkg/epp/backend/metrics/logger.go
+++ b/pkg/epp/backend/metrics/logger.go
@@ -32,6 +32,7 @@ const (
 	// Note currently the EPP treats stale metrics same as fresh.
 	// TODO: https://github.com/kubernetes-sigs/gateway-api-inference-extension/issues/336
 	metricsValidityPeriod = 5 * time.Second
+	debugPrintInterval    = 5 * time.Second
 )
 
 type Datastore interface {
@@ -46,16 +47,15 @@ type Datastore interface {
 // enabled; 2) flushes Prometheus metrics about the backend servers.
 func StartMetricsLogger(ctx context.Context, datastore Datastore, refreshPrometheusMetricsInterval time.Duration) {
 	logger := log.FromContext(ctx)
-
-	// Periodically flush prometheus metrics for inference pool
+	ticker := time.NewTicker(refreshPrometheusMetricsInterval)
 	go func() {
+		defer ticker.Stop()
 		for {
 			select {
 			case <-ctx.Done():
 				logger.V(logutil.DEFAULT).Info("Shutting down prometheus metrics thread")
 				return
-			default:
-				time.Sleep(refreshPrometheusMetricsInterval)
+			case <-ticker.C: // Periodically flush prometheus metrics for inference pool
 				flushPrometheusMetricsOnce(logger, datastore)
 			}
 		}
@@ -64,13 +64,14 @@ func StartMetricsLogger(ctx context.Context, datastore Datastore, refreshPrometh
 	// Periodically print out the pods and metrics for DEBUGGING.
 	if logger := logger.V(logutil.DEBUG); logger.Enabled() {
 		go func() {
+			ticker := time.NewTicker(debugPrintInterval)
+			defer ticker.Stop()
 			for {
 				select {
 				case <-ctx.Done():
 					logger.V(logutil.DEFAULT).Info("Shutting down metrics logger thread")
 					return
-				default:
-					time.Sleep(5 * time.Second)
+				case <-ticker.C:
 					podsWithFreshMetrics := datastore.PodList(func(pm PodMetrics) bool {
 						return time.Since(pm.GetMetrics().UpdateTime) <= metricsValidityPeriod
 					})
diff --git a/pkg/epp/backend/metrics/pod_metrics.go b/pkg/epp/backend/metrics/pod_metrics.go
index cfb6b138..c85d4d79 100644
--- a/pkg/epp/backend/metrics/pod_metrics.go
+++ b/pkg/epp/backend/metrics/pod_metrics.go
@@ -84,21 +84,19 @@ func (pm *podMetrics) startRefreshLoop() {
 	pm.once.Do(func() {
 		go func() {
 			pm.logger.V(logutil.DEFAULT).Info("Starting refresher", "pod", pm.GetPod())
+			ticker := time.NewTicker(pm.interval)
+			defer ticker.Stop()
 			for {
 				select {
 				case <-pm.done:
 					return
 				case <-pm.parentCtx.Done():
 					return
-				default:
+				case <-ticker.C: // refresh metrics periodically
+					if err := pm.refreshMetrics(); err != nil {
+						pm.logger.V(logutil.TRACE).Error(err, "Failed to refresh metrics", "pod", pm.GetPod())
+					}
 				}
-
-				err := pm.refreshMetrics()
-				if err != nil {
-					pm.logger.V(logutil.TRACE).Error(err, "Failed to refresh metrics", "pod", pm.GetPod())
-				}
-
-				time.Sleep(pm.interval)
 			}
 		}()
 	})
diff --git a/pkg/epp/server/runserver.go b/pkg/epp/server/runserver.go
index a6c9f1d3..7ed183be 100644
--- a/pkg/epp/server/runserver.go
+++ b/pkg/epp/server/runserver.go
@@ -76,6 +76,7 @@ func NewDefaultExtProcServerRunner() *ExtProcServerRunner {
 		PoolName:                                 DefaultPoolName,
 		PoolNamespace:                            DefaultPoolNamespace,
 		SecureServing:                            DefaultSecureServing,
+		RefreshPrometheusMetricsInterval:         DefaultRefreshPrometheusMetricsInterval,
 		// Datastore can be assigned later.
 	}
 }



Test patch:
diff --git a/test/integration/epp/hermetic_test.go b/test/integration/epp/hermetic_test.go
index cf00a049..1c5eca18 100644
--- a/test/integration/epp/hermetic_test.go
+++ b/test/integration/epp/hermetic_test.go
@@ -1548,6 +1548,8 @@ func setUpHermeticServer(t *testing.T, podAndMetrics map[backendmetrics.Pod]*bac
 		}
 	}()
 
+	time.Sleep(serverRunner.RefreshPrometheusMetricsInterval) // wait for metrics to get available before running tests that rely on these metrics
+
 	// check if all pods are synced to datastore
 	assert.EventuallyWithT(t, func(t *assert.CollectT) {
 		assert.Len(t, serverRunner.Datastore.PodGetAll(), len(podAndMetrics), "Datastore not synced")