diff --git a/pkg/executor/executortype/newdeploy/newdeploy.go b/pkg/executor/executortype/newdeploy/newdeploy.go index 20ef631..4bf244a 100644 --- a/pkg/executor/executortype/newdeploy/newdeploy.go +++ b/pkg/executor/executortype/newdeploy/newdeploy.go @@ -65,14 +65,18 @@ func (deploy *NewDeploy) createOrGetDeployment(ctx context.Context, fn *fv1.Func return existingDepl, nil } + // existingDepl was read before the scale request, and on the scale-from-zero path + // its status can still count the pod the idle reaper is tearing down. + scaledUp := false if *existingDepl.Spec.Replicas < minScale { err = util.ScaleDeployment(ctx, deploy.kubernetesClient, deploy.logger, existingDepl.Namespace, existingDepl.Name, minScale) if err != nil { deploy.logger.Error(err, "error scaling up function deployment", "function", fn.Name) return nil, err } + scaledUp = true } - if existingDepl.Status.AvailableReplicas < minScale { + if scaledUp || existingDepl.Status.AvailableReplicas < minScale { existingDepl, err = util.WaitForDeployment(ctx, deploy.kubernetesClient, deploy.logger, existingDepl, minScale, specializationTimeout) } diff --git a/pkg/executor/executortype/newdeploy/newdeploy_test.go b/pkg/executor/executortype/newdeploy/newdeploy_test.go index 42b4b1a..8ad4ec4 100644 --- a/pkg/executor/executortype/newdeploy/newdeploy_test.go +++ b/pkg/executor/executortype/newdeploy/newdeploy_test.go @@ -5,14 +5,18 @@ package newdeploy import ( + "context" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" apiv1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" fv1 "github.com/fission/fission/pkg/apis/core/v1" ferror "github.com/fission/fission/pkg/error" @@ -639,3 +643,98 @@ func TestGetDeploymentSpecNonOCIUnchanged(t *testing.T) { } assert.Empty(t, dep.Spec.Template.Spec.ImagePullSecrets) } + +// TestNewDeployScaleFromZeroWaitsForAvailability is the newdeploy side of the +// scale-to-zero race reported in #3183. On the scale-from-zero path the +// deployment status can still describe the pod the idle reaper is tearing +// down, either because it was read before the scale request or because the +// controller has not written a status for that request yet. A caller that +// trusts such a status reports success while the Service has no endpoint and +// the router forwards into it. +func TestNewDeployScaleFromZeroWaitsForAvailability(t *testing.T) { + t.Parallel() + + // Generation of the spec after the scale request. + const generation = 2 + + tests := []struct { + name string + // status is the deployment status served by the nth read, 1-based. + status func(read int) appsv1.DeploymentStatus + }{ + { + // The copy createOrGetDeployment read before scaling up. + name: "pre-scale read still counts the terminating pod", + status: func(read int) appsv1.DeploymentStatus { + available := int32(0) + if read == 1 { + available = 1 + } + return appsv1.DeploymentStatus{ObservedGeneration: generation, AvailableReplicas: available} + }, + }, + { + // Status is written asynchronously by the deployment controller, so + // every read up to the deadline can still describe the generation + // before the scale request. + name: "status has not caught up with the scale request", + status: func(int) appsv1.DeploymentStatus { + return appsv1.DeploymentStatus{ObservedGeneration: generation - 1, AvailableReplicas: 1} + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + env := newTestNewDeployEnv() + fn := newTestNewDeployFunction() + fn.Spec.InvokeStrategy.ExecutionStrategy.MinScale = 0 + + ns := utils.DefaultNSResolver().GetFunctionNS(fn.Namespace) + zero := int32(0) + scaledIn := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "newdeploy-test-fn", + Namespace: ns, + Generation: generation, + Annotations: map[string]string{fv1.EXECUTOR_INSTANCEID_LABEL: "test-instance"}, + }, + Spec: appsv1.DeploymentSpec{Replicas: &zero}, + } + + kubeClient := fake.NewClientset(scaledIn) + var reads int + kubeClient.PrependReactor("get", "deployments", func(k8stesting.Action) (bool, runtime.Object, error) { + depl := scaledIn.DeepCopy() + reads++ + depl.Status = tc.status(reads) + return true, depl, nil + }) + // The fake clientset has no built-in scale subresource for deployments. + kubeClient.PrependReactor("update", "deployments", func(action k8stesting.Action) (bool, runtime.Object, error) { + if action.GetSubresource() != "scale" { + return false, nil, nil + } + // SAFETY: this reactor is registered for "update" and returns early unless the subresource is scale, so the action is an UpdateAction. + return true, action.(k8stesting.UpdateAction).GetObject(), nil + }) + + deploy := newTestNewDeploy(t) + deploy.kubernetesClient = kubeClient + deploy.instanceID = "test-instance" + + // The deployment never becomes available in the fake cluster, so a + // correct implementation runs the readiness wait out to this deadline. + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + + _, err := deploy.createOrGetDeployment(ctx, fn, env, "newdeploy-test-fn", + map[string]string{"app": "newdeploy-test"}, map[string]string{}, ns) + require.Error(t, err, "scale-from-zero must not report success while no replica is available") + assert.ErrorIs(t, err, context.DeadlineExceeded, + "expected the readiness wait to run to its deadline, got: %v", err) + }) + } +}