fix: remove resolvedOfferings, defaults, and scheduling for containers not defined in app (#2468)

g-linville · web-flow · commit 3964bbc97fd9 · 2024-01-31T17:16:17.000-05:00
Signed-off-by: Grant Linville &lt;grant@acorn.io&gt;
diff --git a/pkg/apis/internal.acorn.io/v1/appinstance.go b/pkg/apis/internal.acorn.io/v1/appinstance.go
@@ -5,6 +5,8 @@ package v1
 import (
 	"strings"
 
+	"golang.org/x/exp/maps"
+	"golang.org/x/exp/slices"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -122,6 +124,19 @@ func (in *AppInstance) GetStopped() bool {
 	return in.Spec.Stop != nil && *in.Spec.Stop && in.DeletionTimestamp.IsZero()
 }
 
+// GetAllContainerNames returns a string slice containing the name of every container, job, and sidecar defined in Status.AppSpec.
+func (in *AppInstance) GetAllContainerNames() []string {
+	allContainers := append(maps.Keys(in.Status.AppSpec.Containers), maps.Keys(in.Status.AppSpec.Jobs)...)
+	for _, container := range in.Status.AppSpec.Containers {
+		allContainers = append(allContainers, maps.Keys(container.Sidecars)...)
+	}
+	for _, job := range in.Status.AppSpec.Jobs {
+		allContainers = append(allContainers, maps.Keys(job.Sidecars)...)
+	}
+	slices.Sort[[]string](allContainers)
+	return allContainers
+}
+
 func (in *AppInstanceSpec) GetAutoUpgrade() bool {
 	return in.AutoUpgrade != nil && *in.AutoUpgrade
 }
diff --git a/pkg/controller/defaults/memory.go b/pkg/controller/defaults/memory.go
@@ -7,6 +7,7 @@ import (
 	adminv1 "github.com/acorn-io/runtime/pkg/apis/internal.admin.acorn.io/v1"
 	"github.com/acorn-io/runtime/pkg/computeclasses"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/utils/strings/slices"
 )
 
 // defaultMemory calculates the default that should be used and considers the defaults from the Config, ComputeClass, and
@@ -54,6 +55,17 @@ func addDefaultMemory(req router.Request, cfg *apiv1.Config, appInstance *v1.App
 		return err
 	}
 
+	// Remove any memory defaults for containers that are no longer defined in the app.
+	allContainers := appInstance.GetAllContainerNames()
+	for containerName := range appInstance.Status.Defaults.Memory {
+		if containerName == "" {
+			continue
+		}
+		if !slices.Contains(allContainers, containerName) {
+			delete(appInstance.Status.Defaults.Memory, containerName)
+		}
+	}
+
 	return nil
 }
 
diff --git a/pkg/controller/defaults/memory_test.go b/pkg/controller/defaults/memory_test.go
@@ -72,3 +72,7 @@ func TestTwoPCCDefaultsShouldError(t *testing.T) {
 
 	assert.True(t, resp.NoPrune, "NoPrune should be true when error occurs")
 }
+
+func TestRemovedContainer(t *testing.T) {
+	tester.DefaultTest(t, scheme.Scheme, "testdata/memory/removed-container", Calculate)
+}
diff --git a/pkg/controller/defaults/testdata/memory/removed-container/existing.yaml b/pkg/controller/defaults/testdata/memory/removed-container/existing.yaml
@@ -0,0 +1,9 @@
+kind: ProjectInstance
+apiVersion: internal.acorn.io/v1
+metadata:
+  name: app-namespace
+spec: {}
+status:
+  defaultRegion: local
+  supportedRegions:
+    - local
diff --git a/pkg/controller/defaults/testdata/memory/removed-container/expected.golden b/pkg/controller/defaults/testdata/memory/removed-container/expected.golden
@@ -0,0 +1,62 @@
+`apiVersion: internal.acorn.io/v1
+kind: AppInstance
+metadata:
+  creationTimestamp: null
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  image: test
+  memory:
+    oneimage: 1048576
+status:
+  appImage:
+    buildContext: {}
+    id: test
+    imageData: {}
+    vcs: {}
+  appSpec:
+    containers:
+      oneimage:
+        build:
+          context: .
+          dockerfile: Dockerfile
+        image: image-name
+        metrics: {}
+        ports:
+        - port: 80
+          protocol: http
+          targetPort: 81
+        probes: null
+        sidecars:
+          left:
+            image: foo
+            metrics: {}
+            ports:
+            - port: 90
+              protocol: tcp
+              targetPort: 91
+            probes: null
+  appStatus: {}
+  columns: {}
+  conditions:
+    reason: Success
+    status: "True"
+    success: true
+    type: defaults
+  defaults:
+    memory:
+      "": 0
+      left: 0
+      oneimage: 0
+    region: local
+  namespace: app-created-namespace
+  observedGeneration: 1
+  resolvedOfferings: {}
+  staged:
+    appImage:
+      buildContext: {}
+      imageData: {}
+      vcs: {}
+  summary: {}
+`
diff --git a/pkg/controller/defaults/testdata/memory/removed-container/input.yaml b/pkg/controller/defaults/testdata/memory/removed-container/input.yaml
@@ -0,0 +1,36 @@
+kind: AppInstance
+apiVersion: internal.acorn.io/v1
+metadata:
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  image: test
+  memory:
+    oneimage: 1048576 # 1Mi
+status:
+  observedGeneration: 1
+  namespace: app-created-namespace
+  appImage:
+    id: test
+  appSpec:
+    containers:
+      oneimage:
+        sidecars:
+          left:
+            image: "foo"
+            ports:
+              - port: 90
+                targetPort: 91
+                protocol: tcp
+        ports:
+        - port: 80
+          targetPort: 81
+          protocol: http
+        image: "image-name"
+        build:
+          dockerfile: "Dockerfile"
+          context: "."
+  defaults:
+    memory:
+      twoimage: 0
diff --git a/pkg/controller/resolvedofferings/computeclass.go b/pkg/controller/resolvedofferings/computeclass.go
@@ -7,6 +7,7 @@ import (
 	adminv1 "github.com/acorn-io/runtime/pkg/apis/internal.admin.acorn.io/v1"
 	"github.com/acorn-io/runtime/pkg/computeclasses"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/utils/strings/slices"
 )
 
 // resolveComputeClasses resolves the compute class information for each container in the AppInstance
@@ -69,6 +70,17 @@ func resolveComputeClasses(req router.Request, cfg *apiv1.Config, appInstance *v
 		return err
 	}
 
+	// Remove any resolved offerings for containers that are no longer defined in the app.
+	allContainers := appInstance.GetAllContainerNames()
+	for containerName := range appInstance.Status.ResolvedOfferings.Containers {
+		if containerName == "" {
+			continue
+		}
+		if !slices.Contains(allContainers, containerName) {
+			delete(appInstance.Status.ResolvedOfferings.Containers, containerName)
+		}
+	}
+
 	return nil
 }
 
diff --git a/pkg/controller/resolvedofferings/computeclass_test.go b/pkg/controller/resolvedofferings/computeclass_test.go
@@ -88,3 +88,7 @@ func TestUserOverrideComputeClass(t *testing.T) {
 func TestWithAcornfileMemoryAndSpecOverride(t *testing.T) {
 	tester.DefaultTest(t, scheme.Scheme, "testdata/computeclass/with-acornfile-memory-and-spec-override", Calculate)
 }
+
+func TestRemovedContainer(t *testing.T) {
+	tester.DefaultTest(t, scheme.Scheme, "testdata/computeclass/removed-container", Calculate)
+}
diff --git a/pkg/controller/resolvedofferings/testdata/computeclass/removed-container/existing.yaml b/pkg/controller/resolvedofferings/testdata/computeclass/removed-container/existing.yaml
@@ -0,0 +1,9 @@
+kind: ProjectInstance
+apiVersion: internal.acorn.io/v1
+metadata:
+  name: app-namespace
+spec: {}
+status:
+  defaultRegion: local
+  supportedRegions:
+    - local
diff --git a/pkg/controller/resolvedofferings/testdata/computeclass/removed-container/expected.golden b/pkg/controller/resolvedofferings/testdata/computeclass/removed-container/expected.golden
@@ -0,0 +1,65 @@
+`apiVersion: internal.acorn.io/v1
+kind: AppInstance
+metadata:
+  creationTimestamp: null
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  image: test
+  memory:
+    oneimage: 1048576
+status:
+  appImage:
+    buildContext: {}
+    id: test
+    imageData: {}
+    vcs: {}
+  appSpec:
+    containers:
+      oneimage:
+        build:
+          context: .
+          dockerfile: Dockerfile
+        image: image-name
+        metrics: {}
+        ports:
+        - port: 80
+          protocol: http
+          targetPort: 81
+        probes: null
+        sidecars:
+          left:
+            image: foo
+            metrics: {}
+            ports:
+            - port: 90
+              protocol: tcp
+              targetPort: 91
+            probes: null
+  appStatus: {}
+  columns: {}
+  conditions:
+    reason: Success
+    status: "True"
+    success: true
+    type: resolved-offerings
+  defaults: {}
+  namespace: app-created-namespace
+  observedGeneration: 1
+  resolvedOfferings:
+    containers:
+      "":
+        memory: 0
+      left:
+        memory: 0
+      oneimage:
+        memory: 1048576
+    region: local
+  staged:
+    appImage:
+      buildContext: {}
+      imageData: {}
+      vcs: {}
+  summary: {}
+`
diff --git a/pkg/controller/resolvedofferings/testdata/computeclass/removed-container/input.yaml b/pkg/controller/resolvedofferings/testdata/computeclass/removed-container/input.yaml
@@ -0,0 +1,37 @@
+kind: AppInstance
+apiVersion: internal.acorn.io/v1
+metadata:
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  image: test
+  memory:
+    oneimage: 1048576 # 1Mi
+status:
+  observedGeneration: 1
+  namespace: app-created-namespace
+  appImage:
+    id: test
+  appSpec:
+    containers:
+      oneimage:
+        sidecars:
+          left:
+            image: "foo"
+            ports:
+              - port: 90
+                targetPort: 91
+                protocol: tcp
+        ports:
+        - port: 80
+          targetPort: 81
+          protocol: http
+        image: "image-name"
+        build:
+          dockerfile: "Dockerfile"
+          context: "."
+  resolvedOfferings:
+    containers:
+      twoimage:
+        memory: 0
diff --git a/pkg/controller/scheduling/computeclass_test.go b/pkg/controller/scheduling/computeclass_test.go
@@ -114,3 +114,7 @@ func TestInvalidPriorityClassShouldError(t *testing.T) {
 
 	assert.True(t, resp.NoPrune, "NoPrune should be true when error occurs")
 }
+
+func TestRemovedContainerComputeClass(t *testing.T) {
+	tester.DefaultTest(t, scheme.Scheme, "testdata/computeclass/removed-container", Calculate)
+}
diff --git a/pkg/controller/scheduling/memory_test.go b/pkg/controller/scheduling/memory_test.go
@@ -42,3 +42,7 @@ func TestAllSetOverwrite(t *testing.T) {
 func TestSameGenerationMemory(t *testing.T) {
 	tester.DefaultTest(t, scheme.Scheme, "testdata/memory/same-generation", Calculate)
 }
+
+func TestRemovedContainerMemory(t *testing.T) {
+	tester.DefaultTest(t, scheme.Scheme, "testdata/memory/removed-container", Calculate)
+}
diff --git a/pkg/controller/scheduling/scheduling.go b/pkg/controller/scheduling/scheduling.go
@@ -13,6 +13,7 @@ import (
 	nodev1 "k8s.io/api/node/v1"
 	schedulingv1 "k8s.io/api/scheduling/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/utils/strings/slices"
 )
 
 // Calculate is a handler that sets the scheduling rules for an AppInstance to its
@@ -49,6 +50,15 @@ func calculate(req router.Request, appInstance *v1.AppInstance) error {
 	if err := addScheduling(req, appInstance, appInstance.Status.AppSpec.Jobs); err != nil {
 		return err
 	}
+
+	// Remove any scheduling information for containers that are no longer defined in the app.
+	allContainers := appInstance.GetAllContainerNames()
+	for containerName := range appInstance.Status.Scheduling {
+		if !slices.Contains(allContainers, containerName) {
+			delete(appInstance.Status.Scheduling, containerName)
+		}
+	}
+
 	return nil
 }
 
diff --git a/pkg/controller/scheduling/testdata/computeclass/removed-container/existing.yaml b/pkg/controller/scheduling/testdata/computeclass/removed-container/existing.yaml
@@ -0,0 +1,20 @@
+---
+kind: ClusterComputeClassInstance
+apiVersion: internal.admin.acorn.io/v1
+metadata:
+  name: sample-compute-class
+description: Simple description for a simple ComputeClass
+cpuScaler: 0.25
+memory:
+  min: 1Mi # 1Mi
+  max: 2Mi # 2Mi
+  default: 1Mi # 1Mi
+affinity:
+  nodeAffinity:
+    requiredDuringSchedulingIgnoredDuringExecution:
+      nodeSelectorTerms:
+      - matchExpressions:
+        - key: foo
+          operator: In
+          values:
+          - bar
diff --git a/pkg/controller/scheduling/testdata/computeclass/removed-container/expected.golden b/pkg/controller/scheduling/testdata/computeclass/removed-container/expected.golden
diff --git a/pkg/controller/scheduling/testdata/computeclass/removed-container/input.yaml b/pkg/controller/scheduling/testdata/computeclass/removed-container/input.yaml
diff --git a/pkg/controller/scheduling/testdata/memory/removed-container/expected.golden b/pkg/controller/scheduling/testdata/memory/removed-container/expected.golden
diff --git a/pkg/controller/scheduling/testdata/memory/removed-container/input.yaml b/pkg/controller/scheduling/testdata/memory/removed-container/input.yaml
diff --git a/pkg/controller/scheduling/testdata/tolerations/removed-container/expected.golden b/pkg/controller/scheduling/testdata/tolerations/removed-container/expected.golden
diff --git a/pkg/controller/scheduling/testdata/tolerations/removed-container/input.yaml b/pkg/controller/scheduling/testdata/tolerations/removed-container/input.yaml
diff --git a/pkg/controller/scheduling/toleration_test.go b/pkg/controller/scheduling/toleration_test.go

Original file line number	Diff line number	Diff line change
`@@ -72,3 +72,7 @@ func TestTwoPCCDefaultsShouldError(t *testing.T) {`
`72`	`72`
`73`	`73`	`assert.True(t, resp.NoPrune, "NoPrune should be true when error occurs")`
`74`	`74`	`}`
	`75`	`+`
	`76`	`+func TestRemovedContainer(t *testing.T) {`
	`77`	`+ tester.DefaultTest(t, scheme.Scheme, "testdata/memory/removed-container", Calculate)`
	`78`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -88,3 +88,7 @@ func TestUserOverrideComputeClass(t *testing.T) {`
`88`	`88`	`func TestWithAcornfileMemoryAndSpecOverride(t *testing.T) {`
`89`	`89`	`tester.DefaultTest(t, scheme.Scheme, "testdata/computeclass/with-acornfile-memory-and-spec-override", Calculate)`
`90`	`90`	`}`
	`91`	`+`
	`92`	`+func TestRemovedContainer(t *testing.T) {`
	`93`	`+ tester.DefaultTest(t, scheme.Scheme, "testdata/computeclass/removed-container", Calculate)`
	`94`	`+}`