Skip to content

Commit

Permalink
Merge pull request #581 from karlkfi/karl-event-order
Browse files Browse the repository at this point in the history 
fix: Sort reconcile events to fix flakey test
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored May 3, 2022
2 parents 59160ba + 59a8300 commit 02d2092
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 64 deletions.
40 changes: 22 additions & 18 deletions pkg/apply/applier_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,24 +343,24 @@ func TestApplier(t *testing.T) {
Type: event.Started,
},
},
// Secrets before Deployments (see pkg/ordering)
// Wait events with same status sorted by Identifier (see pkg/testutil)
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
GroupName: "wait-0",
Status: event.ReconcilePending,
Identifier: testutil.ToIdentifier(t, resources["secret"]),
Identifier: testutil.ToIdentifier(t, resources["deployment"]),
},
},
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
GroupName: "wait-0",
Status: event.ReconcilePending,
Identifier: testutil.ToIdentifier(t, resources["deployment"]),
Identifier: testutil.ToIdentifier(t, resources["secret"]),
},
},
// Deployment before Secret (see statusEvents)
// Wait events with same status sorted by Identifier (see pkg/testutil)
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
Expand Down Expand Up @@ -524,24 +524,24 @@ func TestApplier(t *testing.T) {
Type: event.Started,
},
},
// Apply Secrets before Deployments (see ordering.SortableMetas)
// Wait events with same status sorted by Identifier (see pkg/testutil)
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
GroupName: "wait-0",
Status: event.ReconcilePending,
Identifier: testutil.ToIdentifier(t, resources["secret"]),
Identifier: testutil.ToIdentifier(t, resources["deployment"]),
},
},
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
GroupName: "wait-0",
Status: event.ReconcilePending,
Identifier: testutil.ToIdentifier(t, resources["deployment"]),
Identifier: testutil.ToIdentifier(t, resources["secret"]),
},
},
// Wait Deployments before Secrets (see testutil.GroupedEventsByID)
// Wait events with same status sorted by Identifier (see pkg/testutil)
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
Expand Down Expand Up @@ -730,7 +730,7 @@ func TestApplier(t *testing.T) {
Type: event.Started,
},
},
// Prune Deployments before Secrets (see ordering.SortableMetas)
// Wait events with same status sorted by Identifier (see pkg/testutil)
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
Expand All @@ -747,7 +747,7 @@ func TestApplier(t *testing.T) {
Identifier: testutil.ToIdentifier(t, resources["secret"]),
},
},
// Wait Deployments before Secrets (see testutil.GroupedEventsByID)
// Wait events with same status sorted by Identifier (see pkg/testutil)
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
Expand Down Expand Up @@ -1123,6 +1123,7 @@ func TestApplier(t *testing.T) {
Type: event.Started,
},
},
// Wait events sorted Pending > Successful (see pkg/testutil)
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
Expand Down Expand Up @@ -1214,15 +1215,15 @@ func TestApplier(t *testing.T) {
Identifiers: object.ObjMetadataSet{
object.UnstructuredToObjMetadata(
testutil.Unstructured(t, resources["deployment"], JSONPathSetter{
"$.kind", "",
"$.metadata.name", "",
}),
),
},
Error: testutil.EqualErrorString(validation.NewError(
field.Required(field.NewPath("kind"), "kind is required"),
field.Required(field.NewPath("metadata", "name"), "name is required"),
object.UnstructuredToObjMetadata(
testutil.Unstructured(t, resources["deployment"], JSONPathSetter{
"$.kind", "",
"$.metadata.name", "",
}),
),
).Error()),
Expand All @@ -1234,15 +1235,15 @@ func TestApplier(t *testing.T) {
Identifiers: object.ObjMetadataSet{
object.UnstructuredToObjMetadata(
testutil.Unstructured(t, resources["deployment"], JSONPathSetter{
"$.metadata.name", "",
"$.kind", "",
}),
),
},
Error: testutil.EqualErrorString(validation.NewError(
field.Required(field.NewPath("metadata", "name"), "name is required"),
field.Required(field.NewPath("kind"), "kind is required"),
object.UnstructuredToObjMetadata(
testutil.Unstructured(t, resources["deployment"], JSONPathSetter{
"$.metadata.name", "",
"$.kind", "",
}),
),
).Error()),
Expand Down Expand Up @@ -1302,7 +1303,7 @@ func TestApplier(t *testing.T) {
Type: event.Started,
},
},
// Secret pending
// Wait events sorted Pending > Successful (see pkg/testutil)
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
Expand All @@ -1311,7 +1312,6 @@ func TestApplier(t *testing.T) {
Identifier: testutil.ToIdentifier(t, resources["secret"]),
},
},
// Secret reconciled
{
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
Expand Down Expand Up @@ -1802,6 +1802,7 @@ func TestApplierCancel(t *testing.T) {
Type: event.Started,
},
},
// Wait events sorted Pending > Successful (see pkg/testutil)
{
// Deployment reconcile pending.
EventType: event.WaitType,
Expand Down Expand Up @@ -1920,6 +1921,9 @@ func TestApplierCancel(t *testing.T) {
}
}

// sort to allow comparison of multiple wait events
testutil.SortExpEvents(receivedEvents)

// Validate the rest of the events
testutil.AssertEqual(t, tc.expectedEvents, receivedEvents,
"Actual events (%d) do not match expected events (%d)",
Expand Down
4 changes: 4 additions & 0 deletions pkg/apply/destroyer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ func TestDestroyerCancel(t *testing.T) {
Type: event.Started,
},
},
// Wait events sorted Pending > Successful (see pkg/testutil)
{
// Deployment reconcile pending.
EventType: event.WaitType,
Expand Down Expand Up @@ -374,6 +375,9 @@ func TestDestroyerCancel(t *testing.T) {
}
}

// sort to allow comparison of multiple wait events
testutil.SortExpEvents(receivedEvents)

// Validate the rest of the events
testutil.AssertEqual(t, tc.expectedEvents, receivedEvents,
"Actual events (%d) do not match expected events (%d)",
Expand Down
62 changes: 29 additions & 33 deletions pkg/testutil/events.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,45 +447,41 @@ func (ape GroupedEventsByID) Less(i, j int) bool {
return i < j
}
switch ape[i].EventType {
case event.ValidationType:
// Validation events are predictable ordered by input object set order.
case event.ApplyType:
if ape[i].ApplyEvent.GroupName == ape[j].ApplyEvent.GroupName &&
ape[i].ApplyEvent.Status == ape[j].ApplyEvent.Status &&
ape[i].ApplyEvent.Identifier.GroupKind == ape[j].ApplyEvent.Identifier.GroupKind {
// apply events are predictably ordered by GroupKind, due to
// ordering.SortableMetas.
// So we only need to sort by namespace & name.
return ape[i].ApplyEvent.Identifier.String() < ape[j].ApplyEvent.Identifier.String()
}
// Apply events are are predictably ordered by ordering.SortableMetas.
case event.PruneType:
if ape[i].PruneEvent.GroupName == ape[j].PruneEvent.GroupName &&
ape[i].PruneEvent.Status == ape[j].PruneEvent.Status &&
ape[i].PruneEvent.Identifier.GroupKind == ape[j].PruneEvent.Identifier.GroupKind {
// prune events are predictably ordered by GroupKind, due to
// ordering.SortableMetas (reversed).
// So we only need to sort by namespace & name.
return ape[i].PruneEvent.Identifier.String() < ape[j].PruneEvent.Identifier.String()
}
// Prune events are predictably ordered in reverse apply order.
case event.DeleteType:
if ape[i].DeleteEvent.GroupName == ape[j].DeleteEvent.GroupName &&
ape[i].DeleteEvent.Status == ape[j].DeleteEvent.Status &&
ape[i].DeleteEvent.Identifier.GroupKind == ape[j].DeleteEvent.Identifier.GroupKind {
// delete events are predictably ordered by GroupKind, due to
// ordering.SortableMetas (reversed).
// So we only need to sort by namespace & name.
return ape[i].DeleteEvent.Identifier.String() < ape[j].DeleteEvent.Identifier.String()
}
// Delete events are predictably ordered in reverse apply order.
case event.WaitType:
if ape[i].WaitEvent.GroupName == ape[j].WaitEvent.GroupName &&
ape[i].WaitEvent.Status == ape[j].WaitEvent.Status &&
ape[i].WaitEvent.Status == event.ReconcileSuccessful {
// pending, skipped, and timeout operations are predictably ordered
// using the order in WaitTask.Ids.
// So we only need to sort Reconciled events, which occur in the
// order the Waitask receives StatusEvents with Current/NotFound.
// Wait events are unpredictably ordered, because the status may
// reconcile before or after the WaitTask starts, and status event
// order after starting is dependent on remote controller behavior.
// So here we sort status groups explicitly:
// Pending > Skipped > Successful > Failed > Timeout.
// Each status group is then sorted by Identifier:
// Group > Kind > Namespace > Name.
// Note that the Pending status is always optional.
if ape[i].WaitEvent.GroupName == ape[j].WaitEvent.GroupName {
if ape[i].WaitEvent.Status != ape[j].WaitEvent.Status {
return lessWaitStatus(ape[i].WaitEvent.Status, ape[j].WaitEvent.Status)
}
return ape[i].WaitEvent.Identifier.String() < ape[j].WaitEvent.Identifier.String()
}
case event.ValidationType:
return ape[i].ValidationEvent.Identifiers.Hash() < ape[j].ValidationEvent.Identifiers.Hash()
}
return i < j
}

var waitStatusWeight = map[event.WaitEventStatus]int{
event.ReconcilePending: 0,
event.ReconcileSkipped: 1,
event.ReconcileSuccessful: 2,
event.ReconcileFailed: 3,
event.ReconcileTimeout: 4,
}

func lessWaitStatus(x, y event.WaitEventStatus) bool {
return waitStatusWeight[x] < waitStatusWeight[y]
}
14 changes: 7 additions & 7 deletions test/e2e/continue_on_error_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,25 +136,25 @@ func continueOnErrorTest(ctx context.Context, c client.Client, invConfig invconf
},
},
{
// CRD reconcile Skipped.
// Pod1 reconcile Pending.
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
GroupName: "wait-0",
Status: event.ReconcileSkipped,
Identifier: object.UnstructuredToObjMetadata(invalidCrdObj),
Status: event.ReconcilePending,
Identifier: object.UnstructuredToObjMetadata(pod1Obj),
},
},
{
// Pod1 reconcile Pending.
// CRD reconcile Skipped.
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
GroupName: "wait-0",
Status: event.ReconcilePending,
Identifier: object.UnstructuredToObjMetadata(pod1Obj),
Status: event.ReconcileSkipped,
Identifier: object.UnstructuredToObjMetadata(invalidCrdObj),
},
},
{
// Pod1 confirmed Current.
// Pod1 reconcile Successful.
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
GroupName: "wait-0",
Expand Down
18 changes: 12 additions & 6 deletions test/e2e/depends_on_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,10 @@ func dependsOnTest(ctx context.Context, c client.Client, invConfig invconfig.Inv
},
},
}
Expect(testutil.EventsToExpEvents(applierEvents)).To(testutil.Equal(expEvents))
receivedEvents := testutil.EventsToExpEvents(applierEvents)
// sort to handle objects reconciling in random order
testutil.SortExpEvents(receivedEvents)
Expect(receivedEvents).To(testutil.Equal(expEvents))

By("verify namespace1 created")
e2eutil.AssertUnstructuredExists(ctx, c, namespace1Obj)
Expand Down Expand Up @@ -660,21 +663,21 @@ func dependsOnTest(ctx context.Context, c client.Client, invConfig invconfig.Inv
},
},
{
// Namespace2 reconcile Pending.
// Namespace1 reconcile Pending.
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
GroupName: "wait-3",
Status: event.ReconcilePending,
Identifier: object.UnstructuredToObjMetadata(namespace2Obj),
Identifier: object.UnstructuredToObjMetadata(namespace1Obj),
},
},
{
// Namespace1 reconcile Pending.
// Namespace2 reconcile Pending.
EventType: event.WaitType,
WaitEvent: &testutil.ExpWaitEvent{
GroupName: "wait-3",
Status: event.ReconcilePending,
Identifier: object.UnstructuredToObjMetadata(namespace1Obj),
Identifier: object.UnstructuredToObjMetadata(namespace2Obj),
},
},
{
Expand Down Expand Up @@ -723,7 +726,10 @@ func dependsOnTest(ctx context.Context, c client.Client, invConfig invconfig.Inv
},
},
}
Expect(testutil.EventsToExpEvents(destroyerEvents)).To(testutil.Equal(expEvents))
receivedEvents = testutil.EventsToExpEvents(destroyerEvents)
// sort to handle objects reconciling in random order
testutil.SortExpEvents(receivedEvents)
Expect(receivedEvents).To(testutil.Equal(expEvents))

By("verify pod1 deleted")
e2eutil.AssertUnstructuredDoesNotExist(ctx, c, pod1Obj)
Expand Down

0 comments on commit 02d2092

Please sign in to comment.