Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix instance mutator test #10605

Merged
merged 4 commits into from Sep 5, 2019
Merged

Fix instance mutator test #10605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b342baf
Make the test more robust.
howbazaar Sep 5, 2019
1e0f5e1
Don't mock the logger.
howbazaar Sep 5, 2019
c5e89a2
Clean up other test.
howbazaar Sep 5, 2019
f27b712
Naming tweaks.
howbazaar Sep 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 12 additions & 6 deletions worker/instancemutater/mutater.go
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ package instancemutater

import (
"fmt"
"sync"

"github.com/juju/collections/set"
"github.com/juju/errors"
Expand Down Expand Up @@ -55,6 +56,7 @@ type MutaterContext interface {
type mutater struct {
context MutaterContext
logger Logger
wg *sync.WaitGroup
machines map[names.MachineTag]chan struct{}
machineDead chan instancemutater.MutaterMachine
}
Expand All @@ -67,7 +69,7 @@ func (m *mutater) startMachines(tags []names.MachineTag) error {
default:
}
m.logger.Tracef("received tag %q", tag.String())
if c := m.machines[tag]; c == nil {
if ch := m.machines[tag]; ch == nil {
// First time we receive the tag, setup watchers.
api, err := m.context.getMachine(tag)
if err != nil {
Expand All @@ -85,8 +87,8 @@ func (m *mutater) startMachines(tags []names.MachineTag) error {
continue
}

c = make(chan struct{})
m.machines[tag] = c
ch = make(chan struct{})
m.machines[tag] = ch

machine := MutaterMachine{
context: m.context.newMachineContext(),
Expand All @@ -95,24 +97,28 @@ func (m *mutater) startMachines(tags []names.MachineTag) error {
id: id,
}

go runMachine(machine, c, m.machineDead)
m.wg.Add(1)
go runMachine(machine, ch, m.machineDead, func() { m.wg.Done() })
} else {
// We've received this tag before, therefore
// the machine has been removed from the model
// cache and no longer needed
c <- struct{}{}
ch <- struct{}{}
}
}
return nil
}

func runMachine(machine MutaterMachine, removed <-chan struct{}, died chan<- instancemutater.MutaterMachine) {
func runMachine(machine MutaterMachine, removed <-chan struct{}, died chan<- instancemutater.MutaterMachine, cleanup func()) {
defer cleanup()
defer func() {
// We can't just send on the dead channel because the
// central loop might be trying to write to us on the
// removed channel.
for {
select {
case <-machine.context.dying():
return
case died <- machine.machineApi:
return
case <-removed:
Expand Down
11 changes: 5 additions & 6 deletions worker/instancemutater/mutater_test.go
View file
Open in desktop
Expand Up @@ -6,6 +6,8 @@ package instancemutater_test
import (
"github.com/golang/mock/gomock"
"github.com/juju/errors"
"github.com/juju/loggo"
"github.com/juju/testing"
jc "github.com/juju/testing/checkers"
gc "gopkg.in/check.v1"
"gopkg.in/juju/names.v3"
Expand All @@ -20,7 +22,7 @@ import (
)

type mutaterSuite struct {
loggerSuite
testing.IsolationSuite

tag names.MachineTag
instId string
Expand All @@ -46,7 +48,6 @@ func (s *mutaterSuite) TestProcessMachineProfileChanges(c *gc.C) {
startingProfiles := []string{"default", "juju-testme"}
finishingProfiles := append(startingProfiles, "juju-testme-lxd-profile-1")

s.ignoreLogging(c)
s.expectRefreshLifeAliveStatusIdle()
s.expectLXDProfileNames(startingProfiles, nil)
s.expectAssignLXDProfiles(finishingProfiles, nil)
Expand All @@ -63,7 +64,6 @@ func (s *mutaterSuite) TestProcessMachineProfileChangesMachineDead(c *gc.C) {

startingProfiles := []string{"default", "juju-testme"}

s.ignoreLogging(c)
s.expectRefreshLifeDead()

info := s.info(startingProfiles, 1, false)
Expand All @@ -77,7 +77,6 @@ func (s *mutaterSuite) TestProcessMachineProfileChangesError(c *gc.C) {
startingProfiles := []string{"default", "juju-testme"}
finishingProfiles := append(startingProfiles, "juju-testme-lxd-profile-1")

s.ignoreLogging(c)
s.expectRefreshLifeAliveStatusIdle()
s.expectLXDProfileNames(startingProfiles, nil)
s.expectAssignLXDProfiles(finishingProfiles, errors.New("fail me"))
Expand Down Expand Up @@ -210,15 +209,15 @@ func (s *mutaterSuite) TestVerifyCurrentProfilesError(c *gc.C) {
func (s *mutaterSuite) setUpMocks(c *gc.C) *gomock.Controller {
ctrl := gomock.NewController(c)

s.logger = mocks.NewMockLogger(ctrl)
logger := loggo.GetLogger("mutaterSuite")

s.machine = mocks.NewMockMutaterMachine(ctrl)
s.machine.EXPECT().Tag().Return(s.tag).AnyTimes()

s.broker = mocks.NewMockLXDProfiler(ctrl)
s.facade = mocks.NewMockInstanceMutaterAPI(ctrl)

s.mutaterMachine = instancemutater.NewMachineContext(s.logger, s.broker, s.machine, s.getRequiredLXDProfiles, s.tag.Id())
s.mutaterMachine = instancemutater.NewMachineContext(logger, s.broker, s.machine, s.getRequiredLXDProfiles, s.tag.Id())
return ctrl
}

Expand Down
5 changes: 5 additions & 0 deletions worker/instancemutater/worker.go
View file
Open in desktop
Expand Up @@ -4,6 +4,8 @@
package instancemutater

import (
"sync"

"github.com/juju/errors"
"gopkg.in/juju/names.v3"
"gopkg.in/juju/worker.v1"
Expand Down Expand Up @@ -174,9 +176,12 @@ type mutaterWorker struct {
}

func (w *mutaterWorker) loop() error {
var wg sync.WaitGroup
defer wg.Wait()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only problem with wg.Wait is that it will hang if we are supposed to be dying if our code has bugs in it. But i think you've wired through dying already, so its probably ok.

m := &mutater{
context: w.getRequiredContextFunc(w),
logger: w.logger,
wg: &wg,
machines: make(map[names.MachineTag]chan struct{}),
machineDead: make(chan instancemutater.MutaterMachine),
}
Expand Down