From 4d72674499b18cfcc53b735d40846268290d3f51 Mon Sep 17 00:00:00 2001 From: Joseph Phillips Date: Wed, 11 Sep 2024 14:16:03 +0200 Subject: [PATCH] fix: process peer relations last It was found that when removing applications, hooks for some relations rely on the application data populated by peer relations. When peer relations are departed/broken first, these hooks fail, blocking teardown. It is reasonable to process existing relations peer-last to prevent this situation. Hooks for relation creation and joining are unaffected. --- worker/uniter/relation/resolver.go | 27 +++++++++++++- worker/uniter/relation/resolver_test.go | 49 +++++++++++++++++-------- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/worker/uniter/relation/resolver.go b/worker/uniter/relation/resolver.go index df8db8e9672d..6d2021deea66 100644 --- a/worker/uniter/relation/resolver.go +++ b/worker/uniter/relation/resolver.go @@ -74,8 +74,33 @@ func (r *relationsResolver) NextOp( return nil, errors.Trace(err) } - // Check whether we need to fire a hook for any of the relations. + // Collect peer relations and defer their processing until after other + // relations. This is simpler than implementing a sort based on the type. + // Processing them last ensures that upon application removal, the hooks + // for other relations can rely on the peer relations still being present. + var peerRelations []int + + // Check whether we need to fire a hook for any of the non-peer relations. for relationID, relationSnapshot := range remoteState.Relations { + if isPeer, _ := r.stateTracker.IsPeerRelation(relationID); isPeer { + peerRelations = append(peerRelations, relationID) + continue + } + + op, err := r.processRelationSnapshot(relationID, relationSnapshot, remoteState, opFactory) + if err != nil { + if errors.Is(err, resolver.ErrNoOperation) { + continue + } + return nil, errors.Trace(err) + } + return op, nil + } + + // Process the deferred peer relations. + for _, relationID := range peerRelations { + relationSnapshot := remoteState.Relations[relationID] + op, err := r.processRelationSnapshot(relationID, relationSnapshot, remoteState, opFactory) if err != nil { if errors.Is(err, resolver.ErrNoOperation) { diff --git a/worker/uniter/relation/resolver_test.go b/worker/uniter/relation/resolver_test.go index 1d23f9f2a1c7..e9fb03847ed1 100644 --- a/worker/uniter/relation/resolver_test.go +++ b/worker/uniter/relation/resolver_test.go @@ -1359,7 +1359,8 @@ func (s *mockRelationResolverSuite) TestHookRelationJoined(c *gc.C) { s.expectIsKnown(1) s.expectIsImplicitFalse(1) s.expectStateUnknown(1) - s.expectIsPeerRelationFalse(1) + + s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2) relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer) op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{}) @@ -1402,7 +1403,8 @@ func (s *mockRelationResolverSuite) TestHookRelationChangedApplication(c *gc.C) s.expectIsKnown(1) s.expectIsImplicitFalse(1) s.expectState(relationState) - s.expectIsPeerRelationFalse(1) + + s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2) relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer) op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{}) @@ -1441,6 +1443,8 @@ func (s *mockRelationResolverSuite) TestHookRelationChangedSuspended(c *gc.C) { s.expectState(relationState) s.expectLocalUnitAndApplicationLife() + s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil) + relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer) op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{}) c.Assert(err, jc.ErrorIsNil) @@ -1478,6 +1482,8 @@ func (s *mockRelationResolverSuite) TestHookRelationDeparted(c *gc.C) { s.expectState(relationState) s.expectLocalUnitAndApplicationLife() + s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil) + relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer) op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{}) c.Assert(err, jc.ErrorIsNil) @@ -1495,22 +1501,35 @@ func (s *mockRelationResolverSuite) TestHookRelationBroken(c *gc.C) { 1: { Life: life.Dying, }, + 2: { + Life: life.Dying, + }, }, } - relationState := relation.State{ + + defer s.setupMocks(c).Finish() + + s.expectSyncScopes(remoteState) + + relationState1 := relation.State{ RelationId: 1, Members: map[string]int64{}, ApplicationMembers: map[string]int64{}, ChangedPending: "", } - defer s.setupMocks(c).Finish() - s.expectSyncScopes(remoteState) s.expectIsKnown(1) s.expectIsImplicitFalse(1) - s.expectState(relationState) - s.expectIsPeerRelationFalse(1) + s.expectState(relationState1) s.expectStateFound(1) s.expectRemoteApplication(1, "") + s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2) + + // The `Relations` map in the remote state snapshot (as with all Go maps) + // has random iteration order. This will sometimes be called + // (relation ID 2 first) and sometimes not (ID 1 first). The test here is + // that in all cases, the next operation is for ID 1 (non-peer) - it is + // always enqueued ahead of ID 2, which is a peer relation. + s.mockRelStTracker.EXPECT().IsPeerRelation(2).Return(true, nil).MaxTimes(1) relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer) op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{}) @@ -1543,10 +1562,11 @@ func (s *mockRelationResolverSuite) TestHookRelationBrokenWhenSuspended(c *gc.C) s.expectIsKnown(1) s.expectIsImplicitFalse(1) s.expectState(relationState) - s.expectIsPeerRelationFalse(1) s.expectStateFound(1) s.expectRemoteApplication(1, "") + s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2) + relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer) op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{}) c.Assert(err, jc.ErrorIsNil) @@ -1577,9 +1597,10 @@ func (s *mockRelationResolverSuite) TestHookRelationBrokenOnlyOnce(c *gc.C) { s.expectIsKnown(1) s.expectIsImplicitFalse(1) s.expectState(relationState) - s.expectIsPeerRelationFalse(1) s.expectStateFoundFalse(1) + s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2) + relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer) _, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{}) c.Assert(errors.Cause(err), gc.Equals, resolver.ErrNoOperation) @@ -1606,6 +1627,8 @@ func (s *mockRelationResolverSuite) TestImplicitRelationNoHooks(c *gc.C) { s.expectIsKnown(1) s.expectIsImplicit(1) + s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil) + relationsResolver := s.newRelationResolver(s.mockRelStTracker, s.mockSupDestroyer) _, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{}) c.Assert(errors.Cause(err), gc.Equals, resolver.ErrNoOperation) @@ -1643,13 +1666,14 @@ func (s *mockRelationResolverSuite) TestPrincipalDyingDestroysSubordinates(c *gc s.expectIsKnown(1) s.expectIsImplicitFalse(1) s.expectState(relationState) - s.expectIsPeerRelationFalse(1) s.expectHasContainerScope(1) s.expectStateFound(1) s.expectRemoteApplication(1, "") destroyer := mocks.NewMockSubordinateDestroyer(ctrl) destroyer.EXPECT().DestroyAllSubordinates().Return(nil) + s.mockRelStTracker.EXPECT().IsPeerRelation(1).Return(false, nil).Times(2) + relationsResolver := s.newRelationResolver(s.mockRelStTracker, destroyer) op, err := relationsResolver.NextOp(localState, remoteState, &mockOperations{}) c.Assert(err, jc.ErrorIsNil) @@ -1698,11 +1722,6 @@ func (s *mockRelationResolverSuite) expectState(st relation.State) { exp.State(st.RelationId).Return(&st, nil) } -func (s *mockRelationResolverSuite) expectIsPeerRelationFalse(id int) { - exp := s.mockRelStTracker.EXPECT() - exp.IsPeerRelation(id).Return(false, nil) -} - func (s *mockRelationResolverSuite) expectLocalUnitAndApplicationLife() { exp := s.mockRelStTracker.EXPECT() exp.LocalUnitAndApplicationLife().Return(life.Alive, life.Alive, nil)