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 3 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
10 changes: 8 additions & 2 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 Down Expand Up @@ -95,7 +97,8 @@ func (m *mutater) startMachines(tags []names.MachineTag) error {
id: id,
}

go runMachine(machine, c, m.machineDead)
m.wg.Add(1)
go runMachine(machine, c, m.machineDead, m.context.dying(), func() { m.wg.Done() })
} else {
// We've received this tag before, therefore
// the machine has been removed from the model
Expand All @@ -106,13 +109,16 @@ func (m *mutater) startMachines(tags []names.MachineTag) error {
return nil
}

func runMachine(machine MutaterMachine, removed <-chan struct{}, died chan<- instancemutater.MutaterMachine) {
func runMachine(machine MutaterMachine, removed <-chan struct{}, died chan<- instancemutater.MutaterMachine, dying <-chan struct{}, done func()) {
Copy link
Member

Choose a reason for hiding this comment

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

given 'died' 'dying' and 'done', maybe better names might be:
'processDying', 'machineDied', 'cleanup' ?
or maybe 'stop', 'apiDied', 'cleanup'?
I'm not sure on the names, it is just a little confusing as to what is signalling this code that it should stop, vs where is this code signaling others

Copy link
Member

Choose a reason for hiding this comment

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

'removed' is modelCache.removeMachine has been called for the watched lxd machine or container. 'died' is the machine hosting the containers is going away. With my comment at 120, dying isn't necessary.

Perhaps:
died = hostMachineDied
removed = machineDying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dying is necessary, otherwise the goroutine doesn't end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn't see comment below, will use that.

defer done()
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 <-dying:
Copy link
Member

Choose a reason for hiding this comment

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

suggest:
case <- machine.context.dying()

The machines and the worker share a context, no need to pass it in. It's buried down in mutaterWorker newMachineContext(). Ran the race stress 500+ successfully.

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
110 changes: 16 additions & 94 deletions worker/instancemutater/worker_test.go
View file
Open in desktop
Expand Up @@ -61,22 +61,22 @@ func (s *workerConfigSuite) TestInvalidConfigValidate(c *gc.C) {
{
description: "Test no api",
config: instancemutater.Config{
Logger: mocks.NewMockLogger(ctrl),
Logger: loggo.GetLogger("test"),
},
err: "nil Facade not valid",
},
{
description: "Test no environ",
config: instancemutater.Config{
Logger: mocks.NewMockLogger(ctrl),
Logger: loggo.GetLogger("test"),
Facade: mocks.NewMockInstanceMutaterAPI(ctrl),
},
err: "nil Broker not valid",
},
{
description: "Test no agent",
config: instancemutater.Config{
Logger: mocks.NewMockLogger(ctrl),
Logger: loggo.GetLogger("test"),
Facade: mocks.NewMockInstanceMutaterAPI(ctrl),
Broker: mocks.NewMockLXDProfiler(ctrl),
},
Expand All @@ -85,7 +85,7 @@ func (s *workerConfigSuite) TestInvalidConfigValidate(c *gc.C) {
{
description: "Test no tag",
config: instancemutater.Config{
Logger: mocks.NewMockLogger(ctrl),
Logger: loggo.GetLogger("test"),
Facade: mocks.NewMockInstanceMutaterAPI(ctrl),
Broker: mocks.NewMockLXDProfiler(ctrl),
AgentConfig: mocks.NewMockConfig(ctrl),
Expand All @@ -95,7 +95,7 @@ func (s *workerConfigSuite) TestInvalidConfigValidate(c *gc.C) {
{
description: "Test no GetMachineWatcher",
config: instancemutater.Config{
Logger: mocks.NewMockLogger(ctrl),
Logger: loggo.GetLogger("test"),
Facade: mocks.NewMockInstanceMutaterAPI(ctrl),
Broker: mocks.NewMockLXDProfiler(ctrl),
AgentConfig: mocks.NewMockConfig(ctrl),
Expand All @@ -106,7 +106,7 @@ func (s *workerConfigSuite) TestInvalidConfigValidate(c *gc.C) {
{
description: "Test no GetRequiredLXDProfiles",
config: instancemutater.Config{
Logger: mocks.NewMockLogger(ctrl),
Logger: loggo.GetLogger("test"),
Facade: mocks.NewMockInstanceMutaterAPI(ctrl),
Broker: mocks.NewMockLXDProfiler(ctrl),
AgentConfig: mocks.NewMockConfig(ctrl),
Expand All @@ -133,7 +133,7 @@ func (s *workerConfigSuite) TestValidConfigValidate(c *gc.C) {

config := instancemutater.Config{
Facade: mocks.NewMockInstanceMutaterAPI(ctrl),
Logger: mocks.NewMockLogger(ctrl),
Logger: loggo.GetLogger("test"),
Broker: mocks.NewMockLXDProfiler(ctrl),
AgentConfig: mocks.NewMockConfig(ctrl),
Tag: names.MachineTag{},
Expand All @@ -148,8 +148,9 @@ func (s *workerConfigSuite) TestValidConfigValidate(c *gc.C) {
}

type workerSuite struct {
loggerSuite
testing.IsolationSuite

logger loggo.Logger
facade *mocks.MockInstanceMutaterAPI
broker *mocks.MockLXDProfiler
agentConfig *mocks.MockConfig
Expand All @@ -172,6 +173,9 @@ var _ = gc.Suite(&workerSuite{})
func (s *workerSuite) SetUpTest(c *gc.C) {
s.IsolationSuite.SetUpTest(c)

s.logger = loggo.GetLogger("workerSuite")
s.logger.SetLogLevel(loggo.TRACE)

s.newWorkerFunc = instancemutater.NewEnvironTestWorker
s.machineTag = names.NewMachineTag("0")
s.getRequiredLXDProfiles = func(modelName string) []string {
Expand All @@ -191,7 +195,6 @@ var _ = gc.Suite(&workerEnvironSuite{})
func (s *workerEnvironSuite) TestFullWorkflow(c *gc.C) {
defer s.setup(c, 1).Finish()

s.ignoreLogging(c)
s.notifyMachines([][]string{{"0"}})
s.expectFacadeMachineTag(0)
s.notifyMachineAppLXDProfile(0, 1)
Expand All @@ -208,7 +211,6 @@ func (s *workerEnvironSuite) TestFullWorkflow(c *gc.C) {
func (s *workerEnvironSuite) TestVerifyCurrentProfilesTrue(c *gc.C) {
defer s.setup(c, 1).Finish()

s.ignoreLogging(c)
s.notifyMachines([][]string{{"0"}})
s.expectFacadeMachineTag(0)
s.notifyMachineAppLXDProfile(0, 1)
Expand All @@ -223,7 +225,6 @@ func (s *workerEnvironSuite) TestVerifyCurrentProfilesTrue(c *gc.C) {
func (s *workerEnvironSuite) TestRemoveAllCharmProfiles(c *gc.C) {
defer s.setup(c, 1).Finish()

s.ignoreLogging(c)
s.notifyMachines([][]string{{"0"}})
s.expectFacadeMachineTag(0)
s.notifyMachineAppLXDProfile(0, 1)
Expand All @@ -243,7 +244,6 @@ func (s *workerEnvironSuite) TestMachineNotifyTwice(c *gc.C) {
// machine notifications are sent. The 2nd group must
// be after machine 0 gets Life() == Alive.
var group sync.WaitGroup
s.ignoreLogging(c)
s.notifyMachinesWaitGroup([][]string{{"0", "1"}, {"0"}}, &group)
s.expectFacadeMachineTag(0)
s.expectFacadeMachineTag(1)
Expand All @@ -262,7 +262,6 @@ func (s *workerEnvironSuite) TestMachineNotifyTwice(c *gc.C) {
func (s *workerEnvironSuite) TestNoChangeFoundOne(c *gc.C) {
defer s.setup(c, 1).Finish()

s.ignoreLogging(c)
s.notifyMachines([][]string{{"0"}})
s.expectFacadeMachineTag(0)
s.notifyMachineAppLXDProfile(0, 1)
Expand All @@ -274,53 +273,31 @@ func (s *workerEnvironSuite) TestNoChangeFoundOne(c *gc.C) {
func (s *workerEnvironSuite) TestNoMachineFound(c *gc.C) {
defer s.setup(c, 1).Finish()

s.ignoreLogging(c)
s.notifyMachines([][]string{{"0"}})
s.expectFacadeReturnsNoMachine()

w, err := s.workerErrorForScenario(c)

// Since we don't use cleanKill() nor errorKill()
// here, but do waitDone() before checking errors.
s.waitDone(c)

// This test had intermittent failures, one of the
// two following would occur. The 2nd is what we're
// looking for. Please improve this test if you're
// able.
if err != nil {
c.Assert(err, gc.ErrorMatches, "catacomb .* is dying")
} else {
err = workertest.CheckKill(c, w)
c.Assert(err, jc.Satisfies, errors.IsNotFound)
}
err := s.errorKill(c, s.workerForScenario(c))
c.Assert(err, jc.Satisfies, errors.IsNotFound)
}

func (s *workerEnvironSuite) TestCharmProfilingInfoNotProvisioned(c *gc.C) {
defer s.setup(c, 1).Finish()

s.ignoreLogging(c)
s.notifyMachines([][]string{{"0"}})
s.expectFacadeMachineTag(0)
s.notifyMachineAppLXDProfile(0, 1)
s.expectCharmProfileInfoNotProvisioned(0)

err := s.errorKill(c, s.workerForScenario(c))
c.Assert(err, gc.IsNil)
s.cleanKill(c, s.workerForScenario(c))
}

func (s *workerEnvironSuite) TestCharmProfilingInfoError(c *gc.C) {
defer s.setup(c, 1).Finish()

s.ignoreLogging(c)
s.notifyMachines([][]string{{"0"}})
s.expectFacadeMachineTag(0)
s.notifyMachineAppLXDProfile(0, 1)
s.expectCharmProfileInfoError(0)

// This is required so that the waitgroups don't collapse before the
// context KillWithError is called with the right method. Otherwise
// context.Kill(nil) is called first. This prevents the logical race.
s.expectContextKillError()

err := s.errorKill(c, s.workerForScenarioWithContext(c))
Expand All @@ -330,7 +307,6 @@ func (s *workerEnvironSuite) TestCharmProfilingInfoError(c *gc.C) {
func (s *workerSuite) setup(c *gc.C, machineCount int) *gomock.Controller {
ctrl := gomock.NewController(c)

s.logger = mocks.NewMockLogger(ctrl)
s.facade = mocks.NewMockInstanceMutaterAPI(ctrl)
s.broker = mocks.NewMockLXDProfiler(ctrl)
s.agentConfig = mocks.NewMockConfig(ctrl)
Expand Down Expand Up @@ -389,23 +365,6 @@ func (s *workerSuite) workerForScenarioWithContext(c *gc.C) worker.Worker {
return w
}

// workerErrorForScenario creates worker config based on the suite's mocks.
// Any supplied behaviour functions are executed, then a new worker is
// started and returned with any error in creation.
func (s *workerSuite) workerErrorForScenario(c *gc.C) (worker.Worker, error) {
config := instancemutater.Config{
Facade: s.facade,
Logger: s.logger,
Broker: s.broker,
AgentConfig: s.agentConfig,
Tag: s.machineTag,
}

return s.newWorkerFunc(config, func(ctx instancemutater.MutaterContext) instancemutater.MutaterContext {
return ctx
})
}

func (s *workerSuite) expectFacadeMachineTag(machine int) {
tag := names.NewMachineTag(strconv.Itoa(machine))
s.facade.EXPECT().Machine(tag).Return(s.machine[machine], nil).AnyTimes()
Expand Down Expand Up @@ -649,7 +608,7 @@ func (s *workerSuite) waitDone(c *gc.C) {
ch := make(chan struct{})
go func() {
s.doneWG.Wait()
ch <- struct{}{}
close(ch)
}()

select {
Expand All @@ -659,42 +618,6 @@ func (s *workerSuite) waitDone(c *gc.C) {
}
}

type loggerSuite struct {
testing.IsolationSuite

logger *mocks.MockLogger
}

var _ = gc.Suite(&loggerSuite{})

// ignoreLogging turns the suite's mock Logger into a sink, with no validation.
// Logs are still emitted via the test Logger.
func (s *loggerSuite) ignoreLogging(c *gc.C) {
warnIt := func(message string, args ...interface{}) { logIt(c, loggo.WARNING, message, args) }
debugIt := func(message string, args ...interface{}) { logIt(c, loggo.DEBUG, message, args) }
errorIt := func(message string, args ...interface{}) { logIt(c, loggo.ERROR, message, args) }
traceIt := func(message string, args ...interface{}) { logIt(c, loggo.TRACE, message, args) }

e := s.logger.EXPECT()
e.Warningf(gomock.Any(), gomock.Any()).AnyTimes().Do(warnIt)
e.Debugf(gomock.Any(), gomock.Any()).AnyTimes().Do(debugIt)
e.Errorf(gomock.Any(), gomock.Any()).AnyTimes().Do(errorIt)
e.Tracef(gomock.Any(), gomock.Any()).AnyTimes().Do(traceIt)

}

func logIt(c *gc.C, level loggo.Level, message string, args interface{}) {
var nArgs []interface{}
var ok bool
if nArgs, ok = args.([]interface{}); ok {
nArgs = append([]interface{}{level}, nArgs...)
} else {
nArgs = append([]interface{}{level}, args)
}

c.Logf("%s "+message, nArgs...)
}

type workerContainerSuite struct {
workerSuite

Expand Down Expand Up @@ -723,7 +646,6 @@ func (s *workerContainerSuite) SetUpTest(c *gc.C) {
func (s *workerContainerSuite) TestFullWorkflow(c *gc.C) {
defer s.setup(c).Finish()

s.ignoreLogging(c)
s.notifyContainers(0, [][]string{{"0/lxd/0", "0/kvm/0"}})
s.expectFacadeMachineTag(0)
s.expectFacadeContainerTags()
Expand Down