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

Already on GitHub? Sign in to your account

Correct merging of cluster and facet objects, with specs (fixes #158) #175

Merged
merged 3 commits into from Oct 1, 2012

Conversation

Projects
None yet
3 participants
Contributor

nickmarden commented Sep 29, 2012

This patch fixes the incorrect merging of the clouds collection, which was at the root of #158.

The basic idea is that, Gorillib::Resolution extensions to allow for deep merging didn't actually dig far enough into the Gorillib internals to ensure that cluster.clouds and facet.clouds would merge correctly. Furthermore, a failure to deep_copy the underlay attributes at resolve-time was causing a corruption in the underlay (read: cluster) security group definitions which would, under certain circumstances, result in "correct behavior" insofar as the merge algorithm expectations were concerned.

This patch also moves much of the resolution and expansion of server information into the Ironfan.cluster call itself, to ensure that no downstream callers are responsible for calling things like cluster.resolve or cluster.expand_servers! in order to get a consistent view of a cluster.

Lastly, specs are added which assert some sane things about how multi-facet clusters should work, and proving that they now do.

Owner

mrflip commented Sep 30, 2012

HOORAY SPECS. I'll do my best to get on that train.

I merged this branch with the other PRs I put in -- see https://github.com/infochimps-labs/ironfan/tree/cloud_merge_borkage

I expect @temujin9 will have to pore over this but I'll try to keep my next patches clean against infochimps-labs/cloud_merge_borkage and infochimps-labs/keypair_not_key_pair

PS In this case, with the gemspec / rakefile / etc changes it was probably appropriate, but I usually see open-source projects recommend against committing version bumps, as it can cause merge conflicts. I haven't been updating the changelog; instead I try to form changelog-ready commit titles and leave it to the maintainer to manage. Anyway if you'll forgive the digression -- @temujin9 what policy would you like us to follow wrt updating the changelog / gem version / gemspec on commits?

Owner

mrflip commented Oct 1, 2012

I'm having a weird way-out-of-scale memory consumption thing happening; I don't know if it's in this code or the changes I made to collection types. I may not be able to take a hard look until Tuesday, but let me know if you notice this too.

Contributor

nickmarden commented Oct 1, 2012

Thanks for catching that @mrflip. The memory footprint issue that you're referring to is definitely real. Some poking around shows that it's caused by

nickmarden/ironfan@63af087#L0R139

which is - at present - a required line because it prevents corruption of copied underlay attributes. (Simply revert that single line and you'll see that the security group specs fail.)

I patched the code this way because of the following discovery: when cluster.clouds gets merged into facet.clouds, and then the facet.cloud(:ec2).security_groups are resolved, using a non-deep copy means that one particular facet's security_groups get overwritten onto the entire cloud's security groups. This is the root cause of #158.

I'm going to continue to look into to it to see if there is a simplifying assumption that can be introduced to achieve clean attributes while reducing memory bloat. In the meantime, I think that the code in my patch is more correct that the original code, albeit at the cost of a goofy memory footprint.

Contributor

nickmarden commented Oct 1, 2012

b01fc4c seems to reduce the memory footprint back to what it once was, by simply removing a whole bunch of unnecessary #resolve calls in favor of a single top-down cluster.resolve call at the end of the cluster loading process.

Contributor

nickmarden commented on lib/ironfan.rb in b01fc4c Oct 1, 2012

This was unrelated to the memory bloat issue, but something that had been bugging me for a long time. In Ironfan v3, Ironfan.clusters returned the hash of known clusters. In Ironfan v4 it always returns an empty hash, which is about as useful as the proverbial poopy-flavored lollipop.

@temujin9 temujin9 merged commit 3bd0f80 into infochimps-labs:master Oct 1, 2012

Contributor

temujin9 commented Oct 1, 2012

Rock. On.

Merged with extreme prejudice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment