Add synthesizer exec timeouts

Azure · Dec 28, 2023 · 17f78fd · 17f78fd
1 parent 9153252
commit 17f78fd
Show file tree

Hide file tree

Showing 7 changed files with 24 additions and 21 deletions.
diff --git a/cmd/eno-controller/main.go b/cmd/eno-controller/main.go
@@ -26,10 +26,12 @@ func main() {
 func run() error {
 	ctx := ctrl.SetupSignalHandler()
 	var (
-		rolloutCooldown time.Duration
-		synconf         = &synthesis.Config{}
+		rolloutCooldown  time.Duration
+		synthesisTimeout time.Duration
+		synconf          = &synthesis.Config{}
 	)
-	flag.DurationVar(&synconf.Timeout, "synthesis-timeout", time.Minute, "Maximum lifespan of synthesizer pods")
+	flag.DurationVar(&synconf.Timeout, "synthesis-pod-timeout", time.Minute, "Maximum lifespan of synthesizer pods")
+	flag.DurationVar(&synthesisTimeout, "synthesis-timeout", time.Second*30, "Timeout when executing synthesizer binaries")
 	flag.DurationVar(&rolloutCooldown, "rollout-cooldown", time.Second*30, "Minimum period of time between each ensuing composition update after a synthesizer is updated")
 	flag.Parse()
 
@@ -51,7 +53,7 @@ func run() error {
 		return fmt.Errorf("constructing synthesizer connection: %w", err)
 	}
 
-	err = synthesis.NewExecController(mgr, synconn)
+	err = synthesis.NewExecController(mgr, synthesisTimeout, synconn)
 	if err != nil {
 		return fmt.Errorf("constructing execution controller: %w", err)
 	}

diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go
@@ -22,7 +22,7 @@ import (
 	"github.com/go-logr/logr"
 )
 
-// TODO: Block Composition deletion until resources have been cleaned up
+// TODO: Block ResourceSlice deletion until resources have been cleaned up
 // TODO: Clean up unused resource slices older than a duration
 
 // TODO: Handle 400s better

diff --git a/internal/controllers/reconciliation/integration_test.go b/internal/controllers/reconciliation/integration_test.go
@@ -147,7 +147,7 @@ func TestCRUD(t *testing.T) {
 			require.NoError(t, synthesis.NewPodLifecycleController(mgr.Manager, &synthesis.Config{
 				Timeout: time.Second * 5,
 			}))
-			require.NoError(t, synthesis.NewExecController(mgr.Manager, &testutil.ExecConn{Hook: newSliceBuilder(t, scheme, &test)}))
+			require.NoError(t, synthesis.NewExecController(mgr.Manager, time.Second, &testutil.ExecConn{Hook: newSliceBuilder(t, scheme, &test)}))
 
 			// Test subject
 			// Only enable rediscoverWhenNotFound on k8s versions that can support it.

diff --git a/internal/controllers/synthesis/connection.go b/internal/controllers/synthesis/connection.go
@@ -83,7 +83,6 @@ func (s *SynthesizerPodConnection) Synthesize(ctx context.Context, syn *apiv1.Sy
 	})
 	if err != nil {
 		e := &exec.CodeExitError{}
-		// TODO: Unit tests
 		if errors.As(err, e) {
 			msg := truncateString(strings.TrimSpace(stderr.String()), 256)
 			return nil, fmt.Errorf("command exited with status %d - stderr: %s", e.Code, msg)

diff --git a/internal/controllers/synthesis/exec.go b/internal/controllers/synthesis/exec.go
@@ -21,17 +21,19 @@ import (
 )
 
 type execController struct {
-	client client.Client
-	conn   SynthesizerConnection
+	client  client.Client
+	timeout time.Duration
+	conn    SynthesizerConnection
 }
 
-func NewExecController(mgr ctrl.Manager, conn SynthesizerConnection) error {
+func NewExecController(mgr ctrl.Manager, timeout time.Duration, conn SynthesizerConnection) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&corev1.Pod{}).
 		WithLogConstructor(manager.NewLogConstructor(mgr, "execController")).
 		Complete(&execController{
-			client: mgr.GetClient(),
-			conn:   conn,
+			client:  mgr.GetClient(),
+			timeout: timeout,
+			conn:    conn,
 		})
 }
 
@@ -97,9 +99,11 @@ func (c *execController) synthesize(ctx context.Context, syn *apiv1.Synthesizer,
 		return nil, fmt.Errorf("building inputs: %w", err)
 	}
 
+	synctx, done := context.WithTimeout(ctx, c.timeout)
+	defer done()
+
 	start := time.Now()
-	// TODO: Timeouts?
-	stdout, err := c.conn.Synthesize(ctx, syn, pod, inputsJson)
+	stdout, err := c.conn.Synthesize(synctx, syn, pod, inputsJson)
 	if err != nil {
 		return nil, err
 	}
@@ -123,8 +127,6 @@ func (c *execController) buildInputsJson(ctx context.Context, comp *apiv1.Compos
 		input.SetAPIVersion(ref.Resource.APIVersion)
 		appendInputNameAnnotation(&ref, input)
 
-		// TODO: Cache inputs?
-
 		start := time.Now()
 		err := c.client.Get(ctx, client.ObjectKeyFromObject(input), input)
 		if err != nil {

diff --git a/internal/controllers/synthesis/exec_integration_test.go b/internal/controllers/synthesis/exec_integration_test.go
@@ -36,7 +36,7 @@ func TestExecIntegrationHappyPath(t *testing.T) {
 	require.NoError(t, NewPodLifecycleController(mgr, minimalTestConfig))
 	require.NoError(t, NewStatusController(mgr))
 	require.NoError(t, NewRolloutController(mgr, time.Millisecond*10))
-	require.NoError(t, NewExecController(mgr, conn))
+	require.NoError(t, NewExecController(mgr, time.Second, conn))
 	go mgr.Start(ctx)
 
 	syn := &apiv1.Synthesizer{}

diff --git a/internal/controllers/synthesis/integration_test.go b/internal/controllers/synthesis/integration_test.go
@@ -28,7 +28,7 @@ func TestControllerHappyPath(t *testing.T) {
 	require.NoError(t, NewPodLifecycleController(mgr.Manager, minimalTestConfig))
 	require.NoError(t, NewStatusController(mgr.Manager))
 	require.NoError(t, NewRolloutController(mgr.Manager, time.Millisecond*10))
-	require.NoError(t, NewExecController(mgr.Manager, &testutil.ExecConn{}))
+	require.NoError(t, NewExecController(mgr.Manager, time.Second, &testutil.ExecConn{}))
 	mgr.Start(t)
 
 	syn := &apiv1.Synthesizer{}
@@ -115,7 +115,7 @@ func TestControllerFastCompositionUpdates(t *testing.T) {
 	require.NoError(t, NewPodLifecycleController(mgr.Manager, minimalTestConfig))
 	require.NoError(t, NewStatusController(mgr.Manager))
 	require.NoError(t, NewRolloutController(mgr.Manager, time.Millisecond*10))
-	require.NoError(t, NewExecController(mgr.Manager, &testutil.ExecConn{Hook: func(s *apiv1.Synthesizer) []client.Object {
+	require.NoError(t, NewExecController(mgr.Manager, time.Second, &testutil.ExecConn{Hook: func(s *apiv1.Synthesizer) []client.Object {
 		// simulate real pods taking some random amount of time to generation
 		time.Sleep(time.Millisecond * time.Duration(rand.Int63n(300)))
 		return nil
@@ -164,7 +164,7 @@ func TestControllerSynthesizerRollout(t *testing.T) {
 	require.NoError(t, NewPodLifecycleController(mgr.Manager, minimalTestConfig))
 	require.NoError(t, NewStatusController(mgr.Manager))
 	require.NoError(t, NewRolloutController(mgr.Manager, time.Hour*24)) // Rollout should not continue during this test
-	require.NoError(t, NewExecController(mgr.Manager, &testutil.ExecConn{}))
+	require.NoError(t, NewExecController(mgr.Manager, time.Second, &testutil.ExecConn{}))
 	mgr.Start(t)
 
 	syn := &apiv1.Synthesizer{}
@@ -228,7 +228,7 @@ func TestControllerSwitchingSynthesizers(t *testing.T) {
 	mgr := testutil.NewManager(t)
 	cli := mgr.GetClient()
 
-	require.NoError(t, NewExecController(mgr.Manager, &testutil.ExecConn{
+	require.NoError(t, NewExecController(mgr.Manager, time.Second, &testutil.ExecConn{
 		Hook: func(s *apiv1.Synthesizer) []client.Object {
 			cm := &corev1.ConfigMap{}
 			cm.APIVersion = "v1"