Redo Service Placement

Redo Service Placement

This changes a number of aspects of how service placement is handled. There
may still be some edge cases and QA will need to be extensive as this 
has moved through phases where various parts worked and didn't. I believe 
this version to be good however.

Position annotations remain on service models now rather than being deleted
when applied. We favor this to having vars such as hasBeenPositioned,
positionedFromGhost and service model x/y (which mixed into BoundingBoxes). 
These various complications are mostly gone and the handling of updating 
position annotations falls to a single method on the topology. (Though to be
fair that is still called from more than one place). 

Further simplification might be possible by unifying the node creation and
node update paths wrt annotations. This didn't however fit in the time
provided.

This also includes a basic fix for always pulling positions from the client
during an export.

https://code.launchpad.net/~bcsaller/juju-gui/exportXY/+merge/191119

(do not edit description out of merge proposal)

[bcsaller, 2013-10-15 08:26:10]

Please take a look.

[gary.poster, 2013-10-15 12:37:24]

On 2013/10/15 08:26:10, bcsaller wrote:
> Please take a look.

Doing qa first.  A lot looks good.  There's one class of serious problems I
still see (and I am pretty sure I did not see these during qa last night).  Here
are symptom descriptions.  They seem very related.

- In a sandbox, go to
http://localhost:8888/sidebar/search/:flags:/charmworldv3/?text=benji and drag
the bundle out to deploy it.  It will appear and then seem to disappear.  It has
moved to the bottom right.  You can drag the bundle up to the center, but within
a few seconds it will return to the bottom right.

- In a real environment, when you first go to the GUI and see only the GUI
service, it shows up in the middle of the canvas and then moves down to the
bottom right.  Again, you can drag it to the right place, but it will return
within a minute or so to the bottom right.

The thing that unifies these scenarios is that the services do not begin with
x/y annotations.

I'll look again when you've had a chance to address, or if I you need to hand it
off to us.

[benjamin.saller, 2013-10-15 14:44:32]

I believe you when you say that it didn't happen in QA at the midway point. My
feeling was that it was working better as well before but trying to get it
passing the existing tests forced some changes and while I thought I got it back
into shape it seems this must be resolved. 

One difference I know is that I don't publish annotations for packed elements as
this was interfering with imports. They'd get build and the delta comes back,
they get placed, and the annotation set and then the imports set annotations
call fires and another delta comes in, racing with the cli placement. That
change and the case where I decide when to pan (which should only happen after
packing more than one new service) both need looking at. I'll try to resolve
these. If that doesn't work we can back up the patch a few revisions and try to
merge in test changes.

I'll look into this now.

[gary.poster, 2013-10-15 15:34:16]

Hey Ben.  I just saw your reply to my qa, which touches on some of my review
comments below.  I'm not sure I said thank you the first time: thank you!  this
will be a great change, and a nice last (major?) commit (for now?) :-)

Gary

https://codereview.appspot.com/14695043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/14695043/diff/1/app/app.js#newcode626
app/app.js:626: var result = this.db.exportDeployer(topology);
why is this necessary if we don't remove x/y annotations anymore?

https://codereview.appspot.com/14695043/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/14695043/diff/1/app/models/models.js#newcode318
app/models/models.js:318: life: {
indentation needs an extra space.

https://codereview.appspot.com/14695043/diff/1/app/models/models.js#newcode1143
app/models/models.js:1143: exportDeployer: function(topology) {
Same comment as before: if we are handling x/y annotations properly now, why is
the topology necessary?

https://codereview.appspot.com/14695043/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/14695043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:64: annotations['gui-y'] =
ghostAttributes.coordinates[1];
Note to self: double check that these annotations are actually sent to server
after creation.

https://codereview.appspot.com/14695043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:65: ghostService.set('annotations', annotations);
Why is this necessary?  Do we need to trigger an event?  If so, please add a
comment to that effect.

https://codereview.appspot.com/14695043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:301: topo.annotateBoxPosition(boxModel);
Is this the rough equivalent of "options.env.update_annotations(	
serviceName, 'service', ghostService.get('annotations')"?

https://codereview.appspot.com/14695043/diff/1/app/views/topology/relation.js
File app/views/topology/relation.js (left):

https://codereview.appspot.com/14695043/diff/1/app/views/topology/relation.js...
app/views/topology/relation.js:187: self.updateLinkEndpoints({ service: svc });
Why is this no longer necessary?

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:106: // current drag supercedes any previous
annotations).
Until we have timestamps associated with our x/y annotations, we will
perpetually have race conditions ISTM. :-(

That's not for this branch, just thinking.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:115: topo.fire('panToCenter');
Ah, I bet this is what I'm encountering in qa.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:927: } else {
delete that extra space to get the linting right, yeah?

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:1004: // new_service_boxes are those w/o current
x/y pos and no
I'd move this comment to right above where the variable is defined.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:1071: // don't share pack positions with other
clients.
This worries me a lot.  Could you elaborate on why you advocate this?

My concern is that we are supposed to be building a shared layout.  Here's a
scenario in which not exporting positions falls over.

- I make a GUI deploy.
- Someone else makes a CLI deploy.
- I make a GUI deploy.
- Someone else opens GUI.

For me, I have the CLI-deployed service positioned in relation to the first
service.  For "someone else," the CLI-deployed service is positioned in relation
to the second service.

This does not seem like what we want.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/topology.js
File app/views/topology/topology.js (right):

https://codereview.appspot.com/14695043/diff/1/app/views/topology/topology.js...
app/views/topology/topology.js:217: box.inDrag = views.DRAG_ENDING;
I think I see the point here, but please add an explanatory comment.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/topology.js...
app/views/topology/topology.js:220: // or applying them locally.
So this is your workaround for the race conditions I mentioned, right?

https://codereview.appspot.com/14695043/diff/1/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/14695043/diff/1/test/test_environment_view.js#...
test/test_environment_view.js:540: console.log('transform',
serviceNode.getAttribute('transform'));
delete, yeah?

https://codereview.appspot.com/14695043/diff/1/test/test_environment_view.js#...
test/test_environment_view.js:626: /*view.topo.servicePointOutside = function()
{
Why are we removing this?  Seems like a reasonable assertion.

https://codereview.appspot.com/14695043/diff/1/test/test_environment_view.js#...
test/test_environment_view.js:645: *view.topo.servicePointOutside = function() {
same comment as above.

https://codereview.appspot.com/14695043/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/14695043/diff/1/test/test_model.js#newcode841
test/test_model.js:841: var result = db.exportDeployer(topology).envExport;
Back to first comment: I'd hope this is no longer necessary.

[benjamin.saller, 2013-10-15 19:40:11]

Thanks, new version pushing now

https://codereview.appspot.com/14695043/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/14695043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:64: annotations['gui-y'] =
ghostAttributes.coordinates[1];
On 2013/10/15 15:34:16, gary.poster wrote:
> Note to self: double check that these annotations are actually sent to server
> after creation.

Line ~301 in the callback handler. We know its on the backend at that point,
there is an explicit call.

https://codereview.appspot.com/14695043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:65: ghostService.set('annotations', annotations);
On 2013/10/15 15:34:16, gary.poster wrote:
> Why is this necessary?  Do we need to trigger an event?  If so, please add a
> comment to that effect.

I was thinking of handling position changes on annotation change events, but no,
this was a guard rail vs some strange behavior I was seeing, it shouldn't
technically be needed.

https://codereview.appspot.com/14695043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:301: topo.annotateBoxPosition(boxModel);
On 2013/10/15 15:34:16, gary.poster wrote:
> Is this the rough equivalent of "options.env.update_annotations(	
> serviceName, 'service', ghostService.get('annotations')"?

It is, it adds support for putting the box in the DRAG_ENDING state with a timed
window in which it shouldn't resend position annotations.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/relation.js
File app/views/topology/relation.js (left):

https://codereview.appspot.com/14695043/diff/1/app/views/topology/relation.js...
app/views/topology/relation.js:187: self.updateLinkEndpoints({ service: svc });
On 2013/10/15 15:34:16, gary.poster wrote:
> Why is this no longer necessary?

It shouldn't have been needed for some time. The only ways to position objects
all explicitly request updates to endpoint/relations connected to the moved
object.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:106: // current drag supercedes any previous
annotations).
On 2013/10/15 15:34:16, gary.poster wrote:
> Until we have timestamps associated with our x/y annotations, we will
> perpetually have race conditions ISTM. :-(
> 
> That's not for this branch, just thinking.

This is also the last write wins case where two clients are moving the same
service at the same time, the first dragend will send a delta event, but the
other client still in the drag will ignore it rather than have it jump mid drag
to some new position.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:106: // current drag supercedes any previous
annotations).
On 2013/10/15 15:34:16, gary.poster wrote:
> Until we have timestamps associated with our x/y annotations, we will
> perpetually have race conditions ISTM. :-(
> 
> That's not for this branch, just thinking.

We also try to protect vs more than one client having the same service in a drag
state and it not being interrupted mid drag by the other client firing a
position annotation. In those cases last write wins so we have some control over
how we race here.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:115: topo.fire('panToCenter');
On 2013/10/15 15:34:16, gary.poster wrote:
> Ah, I bet this is what I'm encountering in qa.

I reworked this code a little, I'm not really sure if its better in all cases.
It's less aggressive now though.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:927: } else {
On 2013/10/15 15:34:16, gary.poster wrote:
> delete that extra space to get the linting right, yeah?

Done.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/topology.js
File app/views/topology/topology.js (right):

https://codereview.appspot.com/14695043/diff/1/app/views/topology/topology.js...
app/views/topology/topology.js:217: box.inDrag = views.DRAG_ENDING;
On 2013/10/15 15:34:16, gary.poster wrote:
> I think I see the point here, but please add an explanatory comment.

Done.

https://codereview.appspot.com/14695043/diff/1/app/views/topology/topology.js...
app/views/topology/topology.js:220: // or applying them locally.
On 2013/10/15 15:34:16, gary.poster wrote:
> So this is your workaround for the race conditions I mentioned, right?

Yeah, its not great, but it seems to work in practice.

https://codereview.appspot.com/14695043/diff/1/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/14695043/diff/1/test/test_environment_view.js#...
test/test_environment_view.js:626: /*view.topo.servicePointOutside = function()
{
On 2013/10/15 15:34:16, gary.poster wrote:
> Why are we removing this?  Seems like a reasonable assertion.

Yeah, and it was, some of the ordering of annotation application has changed so
its possible this can be called, then overwritten.

https://codereview.appspot.com/14695043/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/14695043/diff/1/test/test_model.js#newcode841
test/test_model.js:841: var result = db.exportDeployer(topology).envExport;
On 2013/10/15 15:34:16, gary.poster wrote:
> Back to first comment: I'd hope this is no longer necessary.

It took some reworking to really get there, but it no longer is.

[bcsaller, 2013-10-15 20:40:58]

Please take a look.

[gary.poster, 2013-10-15 22:16:55]

Awesome!  LGTM and qa good, with trivial.  Thank you!

https://codereview.appspot.com/14695043/diff/9001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/14695043/diff/9001/app/models/models.js#newcod...
app/models/models.js:1139: * @param {Object} [topology] used to get position
when available.
Remove this line

https://codereview.appspot.com/14695043/diff/9001/app/models/models.js#newcod...
app/models/models.js:1142: exportDeployer: function(topology) {
Remove topology from arguments

https://codereview.appspot.com/14695043/diff/9001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/14695043/diff/9001/app/views/topology/service....
app/views/topology/service.js:1123: @method findCentroid
panToCentroid seems more apt.  <shrug>

https://codereview.appspot.com/14695043/diff/9001/app/views/topology/topology.js
File app/views/topology/topology.js (right):

https://codereview.appspot.com/14695043/diff/9001/app/views/topology/topology...
app/views/topology/topology.js:212: @param {ms} window.
timeout? wait?  window is overloaded. <shrug>

https://codereview.appspot.com/14695043/diff/9001/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/14695043/diff/9001/test/test_environment_view....
test/test_environment_view.js:625: /*view.topo.servicePointOutside = function()
{
so...should we uncomment this or delete this?  I didn't quite follow from your
reply.

https://codereview.appspot.com/14695043/diff/9001/test/test_environment_view....
test/test_environment_view.js:644: *view.topo.servicePointOutside = function() {
Should we uncomment this or delete this?  I didn't quite follow from your reply.

[matthew.scott, 2013-10-15 22:29:56]

LGTMly code, QAing quickly.

https://codereview.appspot.com/14695043/diff/9001/app/views/topology/topology.js
File app/views/topology/topology.js (right):

https://codereview.appspot.com/14695043/diff/9001/app/views/topology/topology...
app/views/topology/topology.js:212: @param {ms} window.
On 2013/10/15 22:16:56, gary.poster wrote:
> timeout? wait?  window is overloaded. <shrug>

+1 on timeout

https://codereview.appspot.com/14695043/diff/9001/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/14695043/diff/9001/test/test_environment_view....
test/test_environment_view.js:625: /*view.topo.servicePointOutside = function()
{
On 2013/10/15 22:16:56, gary.poster wrote:
> so...should we uncomment this or delete this?  I didn't quite follow from your
> reply.

From my branch, these tests will pass/still test what they if you reset the db;
I think it's worth testing, but that means that tests aren't written properly. 
Rewrite, or maybe move to their own test with it.skip?

https://codereview.appspot.com/14695043/diff/9001/test/test_environment_view....
test/test_environment_view.js:644: *view.topo.servicePointOutside = function() {
On 2013/10/15 22:16:56, gary.poster wrote:
> Should we uncomment this or delete this?  I didn't quite follow from your
reply.

as above

[benjamin.saller, 2013-10-15 22:41:06]

Thanks again, sorry for so many rounds.

https://codereview.appspot.com/14695043/diff/9001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/14695043/diff/9001/app/models/models.js#newcod...
app/models/models.js:1139: * @param {Object} [topology] used to get position
when available.
On 2013/10/15 22:16:56, gary.poster wrote:
> Remove this line

Doh! Thanks

https://codereview.appspot.com/14695043/diff/9001/app/views/topology/topology.js
File app/views/topology/topology.js (right):

https://codereview.appspot.com/14695043/diff/9001/app/views/topology/topology...
app/views/topology/topology.js:212: @param {ms} window.
On 2013/10/15 22:16:56, gary.poster wrote:
> timeout? wait?  window is overloaded. <shrug>

Done.

https://codereview.appspot.com/14695043/diff/9001/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/14695043/diff/9001/test/test_environment_view....
test/test_environment_view.js:644: *view.topo.servicePointOutside = function() {
On 2013/10/15 22:16:56, gary.poster wrote:
> Should we uncomment this or delete this?  I didn't quite follow from your
reply.

Done.

[benjamin.saller, 2013-10-15 22:46:12]

Thanks to you both.

https://codereview.appspot.com/14695043/diff/9001/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/14695043/diff/9001/test/test_environment_view....
test/test_environment_view.js:625: /*view.topo.servicePointOutside = function()
{
On 2013/10/15 22:29:57, matthew.scott wrote:
> On 2013/10/15 22:16:56, gary.poster wrote:
> > so...should we uncomment this or delete this?  I didn't quite follow from
your
> > reply.
> 
> From my branch, these tests will pass/still test what they if you reset the
db;
> I think it's worth testing, but that means that tests aren't written properly.

> Rewrite, or maybe move to their own test with it.skip?

I deleted these for now, but if someone wants to check the QA issue gary found
with reloading positioned objects from a real env, adding in improvements here
might fit.

[bcsaller, 2013-10-15 22:59:04]

*** Submitted:

Redo Service Placement

This changes a number of aspects of how service placement is handled. There
may still be some edge cases and QA will need to be extensive as this 
has moved through phases where various parts worked and didn't. I believe 
this version to be good however.

Position annotations remain on service models now rather than being deleted
when applied. We favor this to having vars such as hasBeenPositioned,
positionedFromGhost and service model x/y (which mixed into BoundingBoxes). 
These various complications are mostly gone and the handling of updating 
position annotations falls to a single method on the topology. (Though to be
fair that is still called from more than one place). 

Further simplification might be possible by unifying the node creation and
node update paths wrt annotations. This didn't however fit in the time
provided.

This also includes a basic fix for always pulling positions from the client
during an export.

R=gary.poster, benjamin.saller, matthew.scott
CC=
https://codereview.appspot.com/14695043