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