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.

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

BROOKLYN-396: test and fix #444

Merged
merged 1 commit into from Nov 18, 2016
Merged

BROOKLYN-396: test and fix #444

merged 1 commit into from Nov 18, 2016

Conversation

aledsage
Copy link
Contributor

When rebinding a location, if a config flag corresponds to a field
with @SetFromFlag then just set the field, and don’t leave the
key-value inside config().getBag().

Some classes use fields to store internal state, which should not be
passed to child locations (e.g. LocalhostMachineProvisioningLocation.origConfigs).

When rebinding a location, if a config flag corresponds to a field
with “@SetFromFlag” then just set the field, and don’t leave the 
key-value inside “config().getBag()”.

Some classes use fields to store internal state, which should not be
passed to child locations
(e.g. `LocalhostMachineProvisioningLocation.origConfigs`).
@aledsage
Copy link
Contributor Author

It's concerning that the implementation of BasicLocationRebindSupport.addConfig() is very different from BasicEntityRebindSupport and BasicPolicyRebindSupport.

But I don't want to try changing that stuff now. It's one to mull over, and change in the future (possibly next time we're doing major work on rebind).

@sjcorbett
Copy link
Contributor

@aledsage the fix looks good. The blueprint I gave in 396 resizes correctly after rebind.

In the comments on the issue you wrote that "I presume the solution will be to improve what we store in the origConfigs (e.g. so that the sub-machines don't inherit it)." Is origConfigs doing the right thing? Or have we side-stepped it?

@aledsage
Copy link
Contributor Author

aledsage commented Nov 18, 2016

@sjcorbett this change ensures the origConfigs has the right thing.

origConfigs is a field on the provisioning location. Its purpose is to remember the state (i.e. config) of the child locations from before we handed them out, so that (for a BYON) if the provisioner gets back the machine, it can reset the config to what it was originally. This means that origConfigs is a map from MachineLocation instance to its config map.

After rebind(), the origConfigs was put into the provisoiner's config().getBag() (as well as being a field). This meant that if the provisioner created another machine on-the-fly then it would pass the origConfigs to the child location. That led to the circular reference, and the horrible map.toString() StackOverflowError!

This only happens for localhost because that is the only such provisioner that can create new machines on-the-fly. For BYON, the pool of machines is fixed.

@aledsage
Copy link
Contributor Author

Merging now.

@asfgit asfgit merged commit 3563d47 into apache:master Nov 18, 2016
asfgit pushed a commit that referenced this pull request Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants