Skip to content

Commit f90c7ab

Browse files
dgrove-ossdgrove-oss
authored
bug fix: ensure ComponentStatus is initialized in ExpectedPodCount (#170)
1 parent 8a6756f commit f90c7ab

File tree

6 files changed

+63
-72
lines changed

6 files changed

+63
-72
lines changed

‎internal/controller/appwrapper/appwrapper_controller.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ import (
3939
"sigs.k8s.io/controller-runtime/pkg/reconcile"
4040

4141
workloadv1beta2 "github.com/project-codeflare/appwrapper/api/v1beta2"
42-
"github.com/project-codeflare/appwrapper/internal/controller/awstatus"
4342
"github.com/project-codeflare/appwrapper/pkg/config"
4443
"github.com/project-codeflare/appwrapper/pkg/utils"
4544
)
@@ -155,7 +154,7 @@ func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request)
155154
}
156155
}
157156

158-
if err := awstatus.EnsureComponentStatusInitialized(ctx, aw); err != nil {
157+
if err := utils.EnsureComponentStatusInitialized(aw); err != nil {
159158
return ctrl.Result{}, err
160159
}
161160

@@ -491,7 +490,11 @@ func (r *AppWrapperReconciler) getPodStatus(ctx context.Context, aw *workloadv1b
491490
client.MatchingLabels{AppWrapperLabel: aw.Name}); err != nil {
492491
return nil, err
493492
}
494-
summary := &podStatusSummary{expected: utils.ExpectedPodCount(aw)}
493+
pc, err := utils.ExpectedPodCount(aw)
494+
if err != nil {
495+
return nil, err
496+
}
497+
summary := &podStatusSummary{expected: pc}
495498

496499
for _, pod := range pods.Items {
497500
switch pod.Status.Phase {

‎internal/controller/appwrapper/appwrapper_controller_test.go‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ var _ = Describe("AppWrapper Controller", func() {
106106
Expect((*workload.AppWrapper)(aw).IsSuspended()).Should(BeFalse())
107107
podStatus, err := awReconciler.getPodStatus(ctx, aw)
108108
Expect(err).NotTo(HaveOccurred())
109-
Expect(podStatus.pending).Should(Equal(utils.ExpectedPodCount(aw)))
109+
Expect(utils.ExpectedPodCount(aw)).Should(Equal(podStatus.pending))
110110

111111
By("Simulating first Pod Running")
112112
Expect(setPodStatus(aw, v1.PodRunning, 1)).To(Succeed())
@@ -123,15 +123,18 @@ var _ = Describe("AppWrapper Controller", func() {
123123
Expect((*workload.AppWrapper)(aw).IsSuspended()).Should(BeFalse())
124124
podStatus, err = awReconciler.getPodStatus(ctx, aw)
125125
Expect(err).NotTo(HaveOccurred())
126-
Expect(podStatus.pending).Should(Equal(utils.ExpectedPodCount(aw) - 1))
126+
Expect(podStatus.running).Should(Equal(int32(1)))
127+
Expect(utils.ExpectedPodCount(aw)).Should(Equal(podStatus.pending + podStatus.running))
127128
}
128129

129130
fullyRunning := func() {
130131
aw := getAppWrapper(awName)
131132
By("Simulating all Pods Running")
132-
Expect(setPodStatus(aw, v1.PodRunning, utils.ExpectedPodCount(aw))).To(Succeed())
133+
pc, err := utils.ExpectedPodCount(aw)
134+
Expect(err).NotTo(HaveOccurred())
135+
Expect(setPodStatus(aw, v1.PodRunning, pc)).To(Succeed())
133136
By("Reconciling: Running -> Running")
134-
_, err := awReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: awName})
137+
_, err = awReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: awName})
135138
Expect(err).NotTo(HaveOccurred())
136139

137140
aw = getAppWrapper(awName)
@@ -144,7 +147,7 @@ var _ = Describe("AppWrapper Controller", func() {
144147
Expect((*workload.AppWrapper)(aw).PodsReady()).Should(BeTrue())
145148
podStatus, err := awReconciler.getPodStatus(ctx, aw)
146149
Expect(err).NotTo(HaveOccurred())
147-
Expect(podStatus.running).Should(Equal(utils.ExpectedPodCount(aw)))
150+
Expect(podStatus.running).Should(Equal(pc))
148151
_, finished := (*workload.AppWrapper)(aw).Finished()
149152
Expect(finished).Should(BeFalse())
150153
}
@@ -184,13 +187,16 @@ var _ = Describe("AppWrapper Controller", func() {
184187
Expect(meta.IsStatusConditionTrue(aw.Status.Conditions, string(workloadv1beta2.QuotaReserved))).Should(BeTrue())
185188
Expect((*workload.AppWrapper)(aw).IsActive()).Should(BeTrue())
186189
Expect((*workload.AppWrapper)(aw).IsSuspended()).Should(BeFalse())
190+
pc, err := utils.ExpectedPodCount(aw)
191+
Expect(err).NotTo(HaveOccurred())
192+
Expect(pc).Should(Equal(int32(2)))
187193
podStatus, err := awReconciler.getPodStatus(ctx, aw)
188194
Expect(err).NotTo(HaveOccurred())
189-
Expect(podStatus.running).Should(Equal(utils.ExpectedPodCount(aw) - 1))
195+
Expect(podStatus.running).Should(Equal(int32(1)))
190196
Expect(podStatus.succeeded).Should(Equal(int32(1)))
191197

192198
By("Simulating all Pods Completing")
193-
Expect(setPodStatus(aw, v1.PodSucceeded, utils.ExpectedPodCount(aw))).To(Succeed())
199+
Expect(setPodStatus(aw, v1.PodSucceeded, 2)).To(Succeed())
194200
By("Reconciling: Running -> Succeeded")
195201
_, err = awReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: awName})
196202
Expect(err).NotTo(HaveOccurred())

‎internal/controller/awstatus/status_utils.go‎

Lines changed: 0 additions & 54 deletions
This file was deleted.

‎internal/controller/workload/workload_controller.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ limitations under the License.
1717
package workload
1818

1919
import (
20-
"context"
2120
"fmt"
2221

2322
"k8s.io/apimachinery/pkg/api/meta"
@@ -33,7 +32,6 @@ import (
3332
"sigs.k8s.io/kueue/pkg/podset"
3433

3534
workloadv1beta2 "github.com/project-codeflare/appwrapper/api/v1beta2"
36-
"github.com/project-codeflare/appwrapper/internal/controller/awstatus"
3735
"github.com/project-codeflare/appwrapper/pkg/utils"
3836
)
3937

@@ -79,7 +77,7 @@ func (aw *AppWrapper) GVK() schema.GroupVersionKind {
7977

8078
func (aw *AppWrapper) PodSets() []kueue.PodSet {
8179
podSets := []kueue.PodSet{}
82-
if err := awstatus.EnsureComponentStatusInitialized(context.Background(), (*workloadv1beta2.AppWrapper)(aw)); err != nil {
80+
if err := utils.EnsureComponentStatusInitialized((*workloadv1beta2.AppWrapper)(aw)); err != nil {
8381
// Kueue will raise an error on zero length PodSet. Unfortunately, the Kueue API prevents propagating the actual error
8482
return podSets
8583
}

‎pkg/utils/utils.go‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -285,14 +285,46 @@ func Replicas(ps workloadv1beta2.AppWrapperPodSet) int32 {
285285
}
286286
}
287287

288-
func ExpectedPodCount(aw *workloadv1beta2.AppWrapper) int32 {
288+
func ExpectedPodCount(aw *workloadv1beta2.AppWrapper) (int32, error) {
289+
if err := EnsureComponentStatusInitialized(aw); err != nil {
290+
return 0, err
291+
}
289292
var expected int32
290293
for _, c := range aw.Status.ComponentStatus {
291294
for _, s := range c.PodSets {
292295
expected += Replicas(s)
293296
}
294297
}
295-
return expected
298+
return expected, nil
299+
}
300+
301+
// EnsureComponentStatusInitialized initializes aw.Status.ComponenetStatus, including performing PodSet inference for known GVKs
302+
func EnsureComponentStatusInitialized(aw *workloadv1beta2.AppWrapper) error {
303+
if len(aw.Status.ComponentStatus) == len(aw.Spec.Components) {
304+
return nil
305+
}
306+
307+
// Construct definitive PodSets from the Spec + InferPodSets and cache in the Status (to avoid clashing with user updates to the Spec via apply)
308+
compStatus := make([]workloadv1beta2.AppWrapperComponentStatus, len(aw.Spec.Components))
309+
for idx := range aw.Spec.Components {
310+
if len(aw.Spec.Components[idx].DeclaredPodSets) > 0 {
311+
compStatus[idx].PodSets = aw.Spec.Components[idx].DeclaredPodSets
312+
} else {
313+
obj := &unstructured.Unstructured{}
314+
if _, _, err := unstructured.UnstructuredJSONScheme.Decode(aw.Spec.Components[idx].Template.Raw, nil, obj); err != nil {
315+
// Transient error; Template.Raw was validated by our AdmissionController
316+
return err
317+
}
318+
podSets, err := InferPodSets(obj)
319+
if err != nil {
320+
// Transient error; InferPodSets was validated by our AdmissionController
321+
return err
322+
}
323+
compStatus[idx].PodSets = podSets
324+
}
325+
}
326+
aw.Status.ComponentStatus = compStatus
327+
return nil
296328
}
297329

298330
// inferReplicas parses the value at the given path within obj as an int or return 1 or error

‎test/e2e/util_test.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -253,15 +253,21 @@ func waitAWPodsDeleted(ctx context.Context, awNamespace string, awName string) e
253253
}
254254

255255
func waitAWPodsReady(ctx context.Context, aw *workloadv1beta2.AppWrapper) error {
256-
numExpected := utils.ExpectedPodCount(aw)
256+
numExpected, err := utils.ExpectedPodCount(aw)
257+
if err != nil {
258+
return err
259+
}
257260
phases := []v1.PodPhase{v1.PodRunning, v1.PodSucceeded}
258261
return wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 120*time.Second, true, podsInPhase(aw.Namespace, aw.Name, phases, numExpected))
259262
}
260263

261264
func checkAllAWPodsReady(ctx context.Context, aw *workloadv1beta2.AppWrapper) bool {
262-
numExpected := utils.ExpectedPodCount(aw)
265+
numExpected, err := utils.ExpectedPodCount(aw)
266+
if err != nil {
267+
return false
268+
}
263269
phases := []v1.PodPhase{v1.PodRunning, v1.PodSucceeded}
264-
err := wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 100*time.Millisecond, true, podsInPhase(aw.Namespace, aw.Name, phases, numExpected))
270+
err = wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 100*time.Millisecond, true, podsInPhase(aw.Namespace, aw.Name, phases, numExpected))
265271
return err == nil
266272
}
267273

0 commit comments

Comments
 (0)