TRITON-167 Anti-affinity rules fail when no instances match the name

TRITON-168 Regex anti-affinity rules fail unexpectedly
Reviewed by: Trent Mick <trentm@gmail.com>
Approved by: Trent Mick <trentm@gmail.com>

CHANGES.md

@@ -8,6 +8,35 @@ Known issues:
 
 (nothing yet)
 
+## 6.0.0
+
+This release containes some breaking changes with the --affinity flag to
+`triton instance create`. It also does not work with cloudapi endpoints older
+than 8.0.0 (mid 2016); for an older cloudapi endpoint, use node-triton 5.9.0.
+
+- [TRITON-167, TRITON-168] update support for
+  `triton instance create --affinity=...`. It now fully supports regular
+  expressions, tags and globs, and works across a wider variety of situations.
+
+  An example of regular expressions:
+  triton instance create --affinity='instance!=/^production-db/' ...
+
+  An example of globs:
+  triton instance create --affinity='instance!=production-db*' ...
+
+  And an example of tags:
+  triton instance create --affinity='role!=db'
+
+  See <https://apidocs.joyent.com/cloudapi/#affinity-rules> for more details
+  how affinities work.
+
+  However:
+  * Use of regular expressions requires a cloudapi version of 8.8.0 or later.
+  * 'inst' as a affinity shorthand no longer works. Use 'instance' instead.
+    E.g.: --affinity='instance==db1' instead of --affinity='inst==db1'
+  * The shorthand --affinity=<INST> no longer works. Use
+    --affinity='instance===<INST>' instead.
+
 ## 5.10.0
 
 - [TRITON-19] add support for deletion protection on instances. An instance with

lib/do_instance/do_create.js

@@ -95,103 +95,6 @@ function do_create(subcmd, opts, args, cb) {
 
     vasync.pipeline({arg: {cli: this.top}, funcs: [
         common.cliSetupTritonApi,
-        /* BEGIN JSSTYLED */
-        /*
-         * Parse --affinity options for validity to `ctx.affinities`.
-         * Later (in `resolveLocality`) we'll translate this to locality hints
-         * that CloudAPI speaks.
-         *
-         * Some examples. Inspired by
-         * <https://docs.docker.com/swarm/scheduler/filter/#how-to-write-filter-expressions>
-         *
-         *      instance==vm1
-         *      container==vm1      # alternative to 'instance'
-         *      inst==vm1           # alternative to 'instance'
-         *      inst=vm1            # '=' is shortcut for '=='
-         *      inst!=vm1           # '!='
-         *      inst==~vm1          # '~' for soft/non-strict
-         *      inst!=~vm1
-         *
-         *      inst==vm*           # globbing (not yet supported)
-         *      inst!=/vm\d/        # regex (not yet supported)
-         *
-         *      some-tag!=db        # tags (not yet supported)
-         *
-         * Limitations:
-         * - no support for tags yet
-         * - no globbing or regex yet
-         * - we resolve name -> instance id *client-side* for now (until
-         *   CloudAPI supports that)
-         * - Triton doesn't support mixed strict and non-strict, so we error
-         *   out on that. We *could* just drop the non-strict, but that is
-         *   slightly different.
-         */
-        /* END JSSTYLED */
-        function parseAffinity(ctx, next) {
-            if (!opts.affinity) {
-                next();
-                return;
-            }
-
-            var affinities = [];
-
-            // TODO: stricter rules on the value part
-            // JSSTYLED
-            var affinityRe = /((instance|inst|container)(==~|!=~|==|!=|=~|=))?(.*?)$/;
-            for (var i = 0; i < opts.affinity.length; i++) {
-                var raw = opts.affinity[i];
-                var match = affinityRe.exec(raw);
-                if (!match) {
-                    next(new errors.UsageError(format('invalid affinity: "%s"',
-                        raw)));
-                    return;
-                }
-
-                var key = match[2];
-                if ([undefined, 'inst', 'container'].indexOf(key) !== -1) {
-                    key = 'instance';
-                }
-                assert.equal(key, 'instance');
-                var op = match[3];
-                if ([undefined, '='].indexOf(op) !== -1) {
-                    op = '==';
-                }
-                var strict = true;
-                if (op[op.length - 1] === '~') {
-                    strict = false;
-                    op = op.slice(0, op.length - 1);
-                }
-                var val = match[4];
-
-                // Guard against mixed strictness (Triton can't handle those).
-                if (affinities.length > 0) {
-                    var lastAff = affinities[affinities.length - 1];
-                    if (strict !== lastAff.strict) {
-                        next(new errors.TritonError(format('mixed strict and '
-                            + 'non-strict affinities are not supported: '
-                            + '%j (%s) and %j (%s)',
-                            lastAff.raw,
-                            (lastAff.strict ? 'strict' : 'non-strict'),
-                            raw, (strict ? 'strict' : 'non-strict'))));
-                        return;
-                    }
-                }
-
-                affinities.push({
-                    raw: raw,
-                    key: key,
-                    op: op,
-                    strict: strict,
-                    val: val
-                });
-            }
-
-            if (affinities.length) {
-                log.trace({affinities: affinities}, 'affinities');
-                ctx.affinities = affinities;
-            }
-            next();
-        },
 
         /*
          * Make sure if volumes were passed, they're in the correct form.
@@ -270,64 +173,6 @@ function do_create(subcmd, opts, args, cb) {
             next();
         },
 
-        /*
-         * Determine `ctx.locality` according to what CloudAPI supports
-         * based on `ctx.affinities` parsed earlier.
-         */
-        function resolveLocality(ctx, next) {
-            if (!ctx.affinities) {
-                next();
-                return;
-            }
-
-            var strict;
-            var near = [];
-            var far = [];
-
-            vasync.forEachPipeline({
-                inputs: ctx.affinities,
-                func: function resolveAffinity(aff, nextAff) {
-                    assert.ok(['==', '!='].indexOf(aff.op) !== -1,
-                        'unexpected op: ' + aff.op);
-                    var nearFar = (aff.op == '==' ? near : far);
-
-                    strict = aff.strict;
-                    if (common.isUUID(aff.val)) {
-                        nearFar.push(aff.val);
-                        nextAff();
-                    } else {
-                        tritonapi.getInstance({
-                            id: aff.val,
-                            fields: ['id']
-                        }, function (err, inst) {
-                            if (err) {
-                                nextAff(err);
-                            } else {
-                                log.trace({val: aff.val, inst: inst.id},
-                                    'resolveAffinity');
-                                nearFar.push(inst.id);
-                                nextAff();
-                            }
-                        });
-                    }
-                }
-            }, function (err) {
-                if (err) {
-                    next(err);
-                    return;
-                }
-
-                ctx.locality = {
-                    strict: strict
-                };
-                if (near.length > 0) ctx.locality.near = near;
-                if (far.length > 0) ctx.locality.far = far;
-                log.trace({locality: ctx.locality}, 'resolveLocality');
-
-                next();
-            });
-        },
-
         function loadMetadata(ctx, next) {
             mat.metadataFromOpts(opts, log, function (err, metadata) {
                 if (err) {
@@ -431,8 +276,8 @@ function do_create(subcmd, opts, args, cb) {
             if (ctx.volMounts) {
                 createOpts.volumes = ctx.volMounts;
             }
-            if (ctx.locality) {
-                createOpts.locality = ctx.locality;
+            if (opts.affinity) {
+                createOpts.affinity = opts.affinity;
             }
             if (ctx.metadata) {
                 Object.keys(ctx.metadata).forEach(function (key) {
@@ -574,9 +419,7 @@ do_create.options = [
             'INST), `instance==~INST` (*attempt* to place on the same server ' +
             'as INST), or `instance!=~INST` (*attempt* to place on a server ' +
             'other than INST\'s). `INST` is an existing instance name or ' +
-            'id. There are two shortcuts: `inst` may be used instead of ' +
-            '`instance` and `instance==INST` can be shortened to just ' +
-            '`INST`. Use this option more than once for multiple rules.',
+            'id. Use this option more than once for multiple rules.',
         completionType: 'tritonaffinityrule'
     },
 

lib/tritonapi.js

@@ -133,7 +133,7 @@ var errors = require('./errors');
 
 // ---- globals
 
-var CLOUDAPI_ACCEPT_VERSION = '~8||~7';
+var CLOUDAPI_ACCEPT_VERSION = '~8';
 
 
 

package.json

@@ -1,7 +1,7 @@
 {
   "name": "triton",
   "description": "Joyent Triton CLI and client (https://www.joyent.com/triton)",
-  "version": "5.10.0",
+  "version": "6.0.0",
   "author": "Joyent (joyent.com)",
   "homepage": "https://github.com/joyent/node-triton",
   "dependencies": {

test/integration/cli-affinity.test.js

@@ -81,7 +81,8 @@ test('affinity (triton create -a RULE ...)', testOpts, function (tt) {
     var db0Alias = ALIAS_PREFIX + '-db0';
     var db0;
     tt.test('  setup: triton create -n db0', function (t) {
-        var argv = ['create', '-wj', '-n', db0Alias, imgId, pkgId];
+        var argv = ['create', '-wj', '-n', db0Alias, '-t', 'role=database',
+            imgId, pkgId];
         h.safeTriton(t, argv, function (err, stdout) {
             var lines = h.jsonStreamParse(stdout);
             db0 = lines[1];
@@ -92,9 +93,9 @@ test('affinity (triton create -a RULE ...)', testOpts, function (tt) {
     // Test db1 being put on same server as db0.
     var db1Alias = ALIAS_PREFIX + '-db1';
     var db1;
-    tt.test('  setup: triton create -n db1 -a db0', function (t) {
-        var argv = ['create', '-wj', '-n', db1Alias, '-a', db0Alias,
-            imgId, pkgId];
+    tt.test('  triton create -n db1 -a instance==db0', function (t) {
+        var argv = ['create', '-wj', '-n', db1Alias, '-a',
+            'instance==' + db0Alias, imgId, pkgId];
         h.safeTriton(t, argv, function (err, stdout) {
             var lines = h.jsonStreamParse(stdout);
             db1 = lines[1];
@@ -105,11 +106,11 @@ test('affinity (triton create -a RULE ...)', testOpts, function (tt) {
         });
     });
 
-    // Test db2 being put on server *other* than db0.
+    // Test db2 being put on a server without a db.
     var db2Alias = ALIAS_PREFIX + '-db2';
     var db2;
-    tt.test('  setup: triton create -n db2 -a \'inst!=db0\'', function (t) {
-        var argv = ['create', '-wj', '-n', db2Alias, '-a', 'inst!='+db0Alias,
+    tt.test('  triton create -n db2 -a \'instance!=db*\'', function (t) {
+        var argv = ['create', '-wj', '-n', db2Alias, '-a', 'instance!=db*',
             imgId, pkgId];
         h.safeTriton(t, argv, function (err, stdout) {
             var lines = h.jsonStreamParse(stdout);
@@ -121,11 +122,45 @@ test('affinity (triton create -a RULE ...)', testOpts, function (tt) {
         });
     });
 
+
+    // Test db3 being put on server *other* than db0.
+    var db3Alias = ALIAS_PREFIX + '-db3';
+    var db3;
+    tt.test('  triton create -n db3 -a \'instance!=db0\'', function (t) {
+        var argv = ['create', '-wj', '-n', db3Alias, '-a',
+            'instance!='+db0Alias, imgId, pkgId];
+        h.safeTriton(t, argv, function (err, stdout) {
+            var lines = h.jsonStreamParse(stdout);
+            db3 = lines[1];
+            t.notEqual(db0.compute_node, db3.compute_node,
+                format('inst %s landed on different CN (%s) as inst %s (%s)',
+                    db3Alias, db3.compute_node, db0Alias, db0.compute_node));
+            t.end();
+        });
+    });
+
+    // Test db4 being put on server *other* than db0 (due ot db0's tag).
+    var db4Alias = ALIAS_PREFIX + '-db4';
+    var db4;
+    tt.test('  triton create -n db4 -a \'role!=database\'', function (t) {
+        var argv = ['create', '-wj', '-n', db4Alias, '-a', 'role!=database',
+            imgId, pkgId];
+        h.safeTriton(t, argv, function (err, stdout) {
+            var lines = h.jsonStreamParse(stdout);
+            db4 = lines[1];
+            t.notEqual(db0.compute_node, db4.compute_node,
+                format('inst %s landed on different CN (%s) as inst %s (%s)',
+                    db4Alias, db4.compute_node, db0Alias, db0.compute_node));
+            t.end();
+        });
+    });
+
     // Remove instances. Add a test timeout, because '-w' on delete doesn't
     // have a way to know if the attempt failed or if it is just taking a
     // really long time.
     tt.test('  cleanup: triton rm', {timeout: 10 * 60 * 1000}, function (t) {
-        h.safeTriton(t, ['rm', '-w', db0.id, db1.id, db2.id], function () {
+        h.safeTriton(t, ['rm', '-w', db0.id, db1.id, db2.id, db3.id, db4.id],
+        function () {
             t.end();
         });
     });