Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
2.1 kvm race test fixes 1664437 #6988
Conversation
jameinel
added some commits
Feb 14, 2017
| +) | ||
| + | ||
| +// we could pull this from | ||
| +// github.com/juju/juju/cloudconfig/instancecfg.DefaultBridgePrefix, but it |
axw
Feb 16, 2017
Member
AFAICT, the instancecfg.DefaultBridge* consts don't belong in that package anymore. The code in the MAAS provider referencing it appears to be redundant (we add bridge-utils for all machines via cloud-init).
| +// feels bad to import that whole package for a single constant. | ||
| +const DefaultBridgePrefix = "br-" | ||
| + | ||
| +type PrepareAPI interface { |
| + } | ||
| +} | ||
| + | ||
| +// Prepare applies changes to the host machine in that are necessary to create |
| + | ||
| +// Prepare applies changes to the host machine in that are necessary to create | ||
| +// the requested container. | ||
| +func (hp *HostPreparer) Prepare(containerTag names.MachineTag, log loggo.Logger) error { |
axw
Feb 16, 2017
Member
can loggo.Logger be made part of the struct, rather than an arg? feels like it should be part of the context rather than the args
| + | ||
| + log.Debugf("Bridging %+v devices on host %q for container %q with delay=%v, acquiring lock %q", | ||
| + devicesToBridge, hp.machineTag.String(), containerTag.String(), reconfigureDelay, hp.lockName) | ||
| + // TODO(jam): 2017-02-08 figure out how to thread catacomb.Dying() into |
jameinel
Feb 22, 2017
Owner
It is still valid, but I'll move it to the other location where we're passing AbortChan: nil
StartInstance doesn't take an Abort chan, arguably it really should, as it is one of those things that can actually take a long time.
| + | ||
| +func (s *hostPreparerSuite) TestPrepareHostChangesUnsupported(c *gc.C) { | ||
| + s.Stub.SetErrors( | ||
| + errors.NotSupportedf("container address allocation"), |
axw
Feb 16, 2017
Member
HostChangesForContainer shouldn't ever return that error should it? Doesn't it just change its behaviour based on whether addresses can be allocated for containers?
If I'm wrong, maybe leave a comment explaining the scenario this test covers? If I'm right, can we use a more realistic (or arbitrary, e.g. RPC failure) error?
jameinel
Feb 22, 2017
Owner
It was certainly an error that we were hitting in the past, and I wouldn't rule out that we would never see it.
This is mostly about 'if we get an error at this stage' we treat it as a concrete provisioning failure, instead of ignoring it and continuing. I can add some comments about it.
| + // This is what the AcquireLock should look like | ||
| + params.AcquireLockFunc = func(abort <-chan struct{}) (mutex.Releaser, error) { | ||
| + s.Stub.AddCall("AcquireLockFunc") | ||
| + spec := mutex.Spec{ |
axw
Feb 16, 2017
Member
Can we pare this back a bit please, so it's less integration and more unit test? We don't really care about mutexes specifically here. This should be sufficient to test the abort behaviour:
params.AcquireLockFunc = func(abort <-chan struct{}) (mutex.Releaser, error) {
select {
case <-abort:
return nil, errors.New("cancelled acquiring mutex")
case <-time.After(coretesting.LongWait):
return nil, errors.New("timed out waiting for mutex")
}
}
jameinel
Feb 22, 2017
Owner
| @@ -32,6 +41,7 @@ type HostPreparerParams struct { | ||
| CreateBridger func() (network.Bridger, error) | ||
| AbortChan <-chan struct{} | ||
| MachineTag names.MachineTag | ||
| + Logger loggo.Logger |
|
I'm sure I missed it, but I'll catch it before landing. :) Thanks.
…
|
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ looks like Windows got very confused, will try again. |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
jameinel commentedFeb 15, 2017
This isn't critical for 2.1.0, as it is intended to be a code refactoring at testing change, but it does add tests to an area of the code that I've been working in that did not have much (if any). It also removes some of the code paths that were replaced, but left around because of what testing there was available.
Description of change
PrepareHost wasn't tested directly, and even before my last refactoring, was only loosely tested. We can't test all of ContainerSetup because part of its function is to
apt install lxdand actually deploy containers. So this is splitting off a portion of that into a helper object that can have its functionality overridden.QA steps
go testThough if you want to make sure I haven't broken the functionality
And see that the host networking is properly updated for the container.
Documentation changes
Internal only refactoring.
Bug reference
This was wanting to get the code tested as part of fixing lp:1664437. It doesn't change the fix for it, but it does add tests that we are correctly interacting with a 'lock' object.