Merge pull request #11634 from wallyworld/invalid-relation-state

#11634

## Description of change

The relation state struct maintained by the uniter agent is initialised with empty (not nil) maps for Members and ApplicationMembers. YAML serialisation / deserialisation however results in these becoming nil if they were empty when serialised. There's a subsequent validation check before the next hook invocation which fails as it checks for nil. This wasn't an issue previously when state was stored to disk and not the controller as reading state off disk would ensure the maps were empty not nil.

## QA steps

I tested with a modified build which added a wrench but not the patch.

Use the wrench to set up an error in the leader-elected hook for mariadb and create a relation to mediawiki. Remove mariadb once things appear settled and observe the logs
unit-mariadb-2: 12:42:34 ERROR juju.worker.dependency "uniter" manifold worker returned unexpected error: preparing operation "run relation-broken (4; app: ) hook": inappropriate "relation-broken" for "": relation is broken and cannot be changed further

upgrade-controller --build-agent with the patch and see that the relation-broken hook is now run and there are no errors.

## Bug reference

https://bugs.launchpad.net/juju/+bug/1881021

worker/uniter/operation/runhook.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/juju/charm/v7/hooks"
 	"github.com/juju/errors"
-	"github.com/juju/juju/worker/uniter/runner/jujuc"
 
 	"github.com/juju/juju/core/model"
 	"github.com/juju/juju/core/relation"
@@ -18,6 +17,7 @@ import (
 	"github.com/juju/juju/worker/uniter/remotestate"
 	"github.com/juju/juju/worker/uniter/runner"
 	"github.com/juju/juju/worker/uniter/runner/context"
+	"github.com/juju/juju/worker/uniter/runner/jujuc"
 )
 
 type runHook struct {

worker/uniter/relation/state.go

@@ -137,23 +137,33 @@ func (s *State) YamlString() (string, error) {
 	return string(data), nil
 }
 
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (s *State) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	type StateCopy State
+	var sc StateCopy
+	err := unmarshal(&sc)
+	if err != nil {
+		return err
+	}
+	*s = State(sc)
+	if s.Members == nil {
+		s.Members = map[string]int64{}
+	}
+	if s.ApplicationMembers == nil {
+		s.ApplicationMembers = map[string]int64{}
+	}
+	return nil
+}
+
 // copy returns an independent copy of the state.
 func (s *State) copy() *State {
-	stCopy := &State{
-		RelationId:     s.RelationId,
-		ChangedPending: s.ChangedPending,
+	stCopy := NewState(s.RelationId)
+	stCopy.ChangedPending = s.ChangedPending
+	for m, v := range s.Members {
+		stCopy.Members[m] = v
 	}
-	if s.Members != nil {
-		stCopy.Members = make(map[string]int64, len(s.Members))
-		for m, v := range s.Members {
-			stCopy.Members[m] = v
-		}
-	}
-	if s.ApplicationMembers != nil {
-		stCopy.ApplicationMembers = make(map[string]int64, len(s.ApplicationMembers))
-		for m, v := range s.ApplicationMembers {
-			stCopy.ApplicationMembers[m] = v
-		}
+	for m, v := range s.ApplicationMembers {
+		stCopy.ApplicationMembers[m] = v
 	}
 	return stCopy
 }

worker/uniter/relation/statemanager_test.go

@@ -199,23 +199,23 @@ func (s *stateManagerSuite) setupMocks(c *gc.C) *gomock.Controller {
 }
 
 func (s *stateManagerSuite) setupFourStates(c *gc.C) []relation.State {
-	st1 := relation.State{RelationId: 123}
+	st1 := relation.NewState(123)
 	st1.Members = map[string]int64{
 		"foo/0": 1,
 		"foo/1": 2,
 	}
 	st1.ChangedPending = "foo/1"
-	st2 := relation.State{RelationId: 456}
+	st2 := relation.NewState(456)
 	st2.Members = map[string]int64{
 		"bar/0": 3,
 		"bar/1": 4,
 	}
-	st3 := relation.State{RelationId: 789}
-	st4 := relation.State{RelationId: 10}
+	st3 := relation.NewState(789)
+	st4 := relation.NewState(10)
 	st4.ApplicationMembers = map[string]int64{
 		"baz-app": 2,
 	}
-	states := []relation.State{st1, st2, st3, st4}
+	states := []relation.State{*st1, *st2, *st3, *st4}
 	s.expectState(c, states...)
 	return states
 }

worker/uniter/resolver.go

@@ -4,6 +4,8 @@
 package uniter
 
 import (
+	"fmt"
+
 	"github.com/juju/charm/v7/hooks"
 	"github.com/juju/errors"
 
@@ -14,6 +16,7 @@ import (
 	"github.com/juju/juju/worker/uniter/operation"
 	"github.com/juju/juju/worker/uniter/remotestate"
 	"github.com/juju/juju/worker/uniter/resolver"
+	"github.com/juju/juju/wrench"
 )
 
 // ResolverConfig defines configuration for the uniter resolver.
@@ -142,6 +145,12 @@ func (s *uniterResolver) NextOp(
 			return opFactory.NewRunHook(*localState.Hook)
 
 		case operation.Done:
+			curl := localState.CharmURL
+			if curl != nil && wrench.IsActive("hooks", fmt.Sprintf("%s-%s-error", curl.Name, localState.Hook.Kind)) {
+				s.config.Logger.Errorf("commit hook %q failed due to a wrench in the works", localState.Hook.Kind)
+				return nil, errors.Errorf("commit hook %q failed due to a wrench in the works", localState.Hook.Kind)
+			}
+
 			logger.Infof("committing %q hook", localState.Hook.Kind)
 			return opFactory.NewSkipHook(*localState.Hook)