diff --git a/worker/uniter/relation/resolver.go b/worker/uniter/relation/resolver.go index 41d37c59ebf..2685d804657 100644 --- a/worker/uniter/relation/resolver.go +++ b/worker/uniter/relation/resolver.go @@ -51,11 +51,13 @@ type relationsResolver struct { } // NextOp implements resolver.Resolver. -func (r *relationsResolver) NextOp(localState resolver.LocalState, remoteState remotestate.Snapshot, opFactory operation.Factory) (_ operation.Operation, err error) { +func (r *relationsResolver) NextOp( + localState resolver.LocalState, remoteState remotestate.Snapshot, opFactory operation.Factory, +) (_ operation.Operation, err error) { if r.logger.IsTraceEnabled() { r.logger.Tracef("relation resolver next op for new remote relations %# v", pretty.Formatter(remoteState.Relations)) defer func() { - if err == resolver.ErrNoOperation { + if errors.Is(err, resolver.ErrNoOperation) { r.logger.Tracef("no relation operation to run") } }() @@ -72,42 +74,85 @@ func (r *relationsResolver) NextOp(localState resolver.LocalState, remoteState r return nil, errors.Trace(err) } - // Check whether we need to fire a hook for any of the relations - for relationId, relationSnapshot := range remoteState.Relations { - if !r.stateTracker.IsKnown(relationId) { - r.logger.Tracef("unknown relation %d resolving next op", relationId) - continue - } else if isImplicit, _ := r.stateTracker.IsImplicit(relationId); isImplicit { + // 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 } - // If either the unit or the relation are Dying, or the - // relation becomes suspended, then the relation should be - // broken. - var remoteBroken bool - if remoteState.Life == life.Dying || relationSnapshot.Life == life.Dying || relationSnapshot.Suspended { - relationSnapshot = remotestate.RelationSnapshot{} - remoteBroken = true - // TODO(axw) if relation is implicit, leave scope & remove. + 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] - // Examine local/remote states and figure out if a hook needs - // to be fired for this relation. - relState, err := r.stateTracker.State(relationId) + op, err := r.processRelationSnapshot(relationID, relationSnapshot, remoteState, opFactory) if err != nil { - // - relState = NewState(relationId) - } - hInfo, err := r.nextHookForRelation(relState, relationSnapshot, remoteBroken) - if err == resolver.ErrNoOperation { - continue + if errors.Is(err, resolver.ErrNoOperation) { + continue + } + return nil, errors.Trace(err) } - return opFactory.NewRunHook(hInfo) + return op, nil } return nil, resolver.ErrNoOperation } +// processRelationSnapshot reconciles the local and remote states for a +// single relation and determines what hoof (if any) should be fired. +func (r *relationsResolver) processRelationSnapshot( + relationID int, + relationSnapshot remotestate.RelationSnapshot, + remoteState remotestate.Snapshot, + opFactory operation.Factory, +) (operation.Operation, error) { + if !r.stateTracker.IsKnown(relationID) { + r.logger.Infof("unknown relation %d resolving next op", relationID) + return nil, resolver.ErrNoOperation + } else if isImplicit, _ := r.stateTracker.IsImplicit(relationID); isImplicit { + return nil, resolver.ErrNoOperation + } + + // If either the unit or the relation are Dying, or the relation + // becomes suspended, then the relation should be broken. + var remoteBroken bool + if remoteState.Life == life.Dying || relationSnapshot.Life == life.Dying || relationSnapshot.Suspended { + relationSnapshot = remotestate.RelationSnapshot{} + remoteBroken = true + } + + // Examine local/remote states and figure out if a hook needs + // to be fired for this relation. + relState, err := r.stateTracker.State(relationID) + if err != nil { + if !errors.Is(err, errors.NotFound) { + return nil, errors.Trace(err) + } + relState = NewState(relationID) + } + hInfo, err := r.nextHookForRelation(relState, relationSnapshot, remoteBroken) + if err != nil { + return nil, errors.Trace(err) + } + return opFactory.NewRunHook(hInfo) +} + // maybeDestroySubordinates checks whether the remote state indicates that the // unit is dying and ensures that any related subordinates are properly // destroyed. @@ -343,7 +388,7 @@ func (r *createdRelationsResolver) NextOp( if r.logger.IsTraceEnabled() { r.logger.Tracef("create relation resolver next op for new remote relations %# v", pretty.Formatter(remoteState.Relations)) defer func() { - if err == resolver.ErrNoOperation { + if errors.Is(err, resolver.ErrNoOperation) { r.logger.Tracef("no create relation operation to run") } }() @@ -369,7 +414,7 @@ func (r *createdRelationsResolver) NextOp( hook, err := r.nextHookForRelation(relationId) if err != nil { - if err == resolver.ErrNoOperation { + if errors.Is(err, resolver.ErrNoOperation) { continue } diff --git a/worker/uniter/relation/resolver_test.go b/worker/uniter/relation/resolver_test.go index 5c01f592cda..6bc4628bbdc 100644 --- a/worker/uniter/relation/resolver_test.go +++ b/worker/uniter/relation/resolver_test.go @@ -136,7 +136,7 @@ func assertNumCalls(c *gc.C, numCalls *int32, expected int32) { c.Assert(v, gc.Equals, expected) } -func (s *relationResolverSuite) newRelationStateTracer(c *gc.C, apiCaller base.APICaller, unitTag names.UnitTag) relation.RelationStateTracker { +func (s *relationResolverSuite) newRelationStateTracker(c *gc.C, apiCaller base.APICaller, unitTag names.UnitTag) relation.RelationStateTracker { abort := make(chan struct{}) st := uniter.NewState(apiCaller, unitTag) u, err := st.Unit(unitTag) @@ -166,7 +166,7 @@ func (s *relationResolverSuite) setupRelations(c *gc.C) relation.RelationStateTr uniterAPICall("State", unitEntity, unitStateResults, nil), uniterAPICall("RelationsStatus", unitEntity, params.RelationUnitStatusResults{Results: []params.RelationUnitStatusResult{{RelationResults: []params.RelationUnitStatus{}}}}, nil), ) - r := s.newRelationStateTracer(c, apiCaller, unitTag) + r := s.newRelationStateTracker(c, apiCaller, unitTag) assertNumCalls(c, &numCalls, 4) return r } @@ -231,7 +231,7 @@ func (s *relationResolverSuite) assertNewRelationsWithExistingRelations(c *gc.C, ) } apiCaller := mockAPICaller(c, &numCalls, apiCalls...) - r := s.newRelationStateTracer(c, apiCaller, unitTag) + r := s.newRelationStateTracker(c, apiCaller, unitTag) assertNumCalls(c, &numCalls, int32(len(apiCalls))) info := r.GetInfo() @@ -268,7 +268,7 @@ func (s *relationResolverSuite) TestNextOpNothing(c *gc.C) { uniterAPICall("State", unitEntity, unitStateResults, nil), uniterAPICall("RelationsStatus", unitEntity, params.RelationUnitStatusResults{Results: []params.RelationUnitStatusResult{{RelationResults: []params.RelationUnitStatus{}}}}, nil), ) - r := s.newRelationStateTracer(c, apiCaller, unitTag) + r := s.newRelationStateTracker(c, apiCaller, unitTag) assertNumCalls(c, &numCalls, 4) localState := resolver.LocalState{ @@ -393,7 +393,7 @@ func (s *relationResolverSuite) assertHookRelationJoined(c *gc.C, numCalls *int3 unitTag := names.NewUnitTag("wordpress/0") apiCaller := mockAPICaller(c, numCalls, apiCalls...) - r := s.newRelationStateTracer(c, apiCaller, unitTag) + r := s.newRelationStateTracker(c, apiCaller, unitTag) assertNumCalls(c, numCalls, 4) localState := resolver.LocalState{ @@ -839,7 +839,7 @@ func (s *relationResolverSuite) TestImplicitRelationNoHooks(c *gc.C) { var numCalls int32 apiCaller := mockAPICaller(c, &numCalls, apiCalls...) - r := s.newRelationStateTracer(c, apiCaller, unitTag) + r := s.newRelationStateTracker(c, apiCaller, unitTag) localState := resolver.LocalState{ State: operation.State{ @@ -979,7 +979,7 @@ func (s *relationResolverSuite) TestSubSubPrincipalRelationDyingDestroysUnit(c * //apiCalls = append(apiCalls, uniterAPICall("State", nrpeUnitEntity, unitStateResults, nil)) apiCaller := mockAPICaller(c, &numCalls, apiCalls...) - r := s.newRelationStateTracer(c, apiCaller, nrpeUnitTag) + r := s.newRelationStateTracker(c, apiCaller, nrpeUnitTag) assertNumCalls(c, &numCalls, callsBeforeDestroy) // So now we have a relations object with two relations, one to @@ -1035,7 +1035,7 @@ func (s *relationResolverSuite) TestSubSubOtherRelationDyingNotDestroyed(c *gc.C apiCaller := mockAPICaller(c, &numCalls, apiCalls...) - r := s.newRelationStateTracer(c, apiCaller, nrpeUnitTag) + r := s.newRelationStateTracker(c, apiCaller, nrpeUnitTag) // TODO: Fix this test... // This test intermittently makes either 16 or 17 @@ -1154,7 +1154,7 @@ func (s *relationResolverSuite) TestPrincipalDyingDestroysSubordinates(c *gc.C) //apiCalls = append(apiCalls, uniterAPICall("State", nrpeUnitEntity, unitStateResults, nil)) apiCaller := mockAPICaller(c, &numCalls, apiCalls...) - r := s.newRelationStateTracer(c, apiCaller, nrpeUnitTag) + r := s.newRelationStateTracker(c, apiCaller, nrpeUnitTag) assertNumCalls(c, &numCalls, callsBeforeDestroy) // So now we have a relation between a principal (wordpress) and a @@ -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) @@ -1690,7 +1714,7 @@ func (s *mockRelationResolverSuite) expectIsImplicitFalse(id int) { func (s *mockRelationResolverSuite) expectStateUnknown(id int) { exp := s.mockRelStTracker.EXPECT() - exp.State(id).Return(nil, errors.Errorf("unknown relation: %d", id)) + exp.State(id).Return(nil, errors.NotFoundf("relation: %d", id)) } func (s *mockRelationResolverSuite) expectState(st relation.State) { @@ -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)