PUBAPI-1428: CreateMachine: add "affinity" rules, deprecate "locality" hints

Details

Issue Type:Bug
Priority:4 - Normal
Status:Resolved
Created at:2017-08-23T23:34:42.000Z
Updated at:2018-11-28T22:29:55.725Z

People

Created by:Former user
Reported by:Former user
Assigned to:Former user

Resolution

Fixed: A fix for this issue is checked into the tree and tested.
(Resolution Date: 2017-09-26T14:55:22.000Z)

Fix Versions

2017-09-28 Battle Square (Release Date: 2017-09-28)

Related Issues

Description

current CreateMachine locality

CreateMachine has a "locality" field to specify locality hints, e.g.:

"locality": {
  "strict": false,
  "near": ["af7ebb74-59be-4481-994f-f6e05fa53075"],
  "far": ["da568166-9d93-42c8-b9b2-bce9a6bb7e0a", "d45eb2f5-c80b-4fea-854f-32e4a9441e53"]
}

i.e. to strictly or non-strictly find servers that are "near" (the same server)
or "far" (a different server) from the given VM uuids. These are
enforced/implemented in the "locality-hints"
(https://github.com/joyent/sdc-designation/search?q=locality-hints&type=Code)
files in sdc-designation.git (aka DAPI).

Some limitations with this:

current docker affinities

In DOCKER-630, support was added to sdc-docker for Docker swarm-style
affinities. See https://apidocs.joyent.com/docker/features/placement#affinity-filter-syntax

Some benefits/changes from this:

The pattern and tag naming can be quite helpful for re-usable templates, config
files, so one doesn't have to edit these with specific instance UUIDs. A coming
hoped for example is a Terraform config file.

current triton CLI affinity

Around this time, the 'triton instance create ...' command was updated to
support locality hints using basically the same Docker swarm affinity syntax:

    -a RULE, --affinity=RULE
                Affinity rules for selecting a server for this instance. Rules
                have one of the following forms: `instance==INST` (the new
                instance must be on the same server as INST), `instance!=INST`
                (new inst must *not* be on the same server as 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.

Regarding the use of "instance==FOO" rather than "container==FOO": The triton
CLI accepts both for consistency.

proposal: cloudapi CreateMachine with affinity

The proposal for this ticket is to deprecate "locality" with CreateMachine in
favour of a new "affinity" array of strings of the form supported by sdc-docker
and the Triton CLI (see above). It would be an error to specify both "locality"
and "affinity".

The first pass of this would have CloudAPI CreateMachine implement this the
same way that sdc-docker does: by translating the array of affinity rule strings
into the current "locality" object format that is passed to VMAPI, and then
on to sdc-designation (aka DAPI) for server selection. This implies two
limitations:

1. mixing hard and soft localities would not yet work (currently sdc-docker
handles that case by dropping the soft localities); and
2. DOCKER-1039 is a known issue: Hard affinity rules using instance names or
tags for concurrent provisions will race. The correct fix for that (to
handle the translation from instance name/tags to UUIDs in DAPI's
server selection -- which is serialized in the DC) will fix the issue for
both sdc-docker and CloudAPI.

This would technically be a feature addition, so would only need a minor
version bump in CloudAPI.

Discussion today: https://jabber.joyent.com/logs/sdc@conference.joyent.com/2017/08/23.html#20:37:38.127090

Aside on using API minor versions

Using current CloudAPI version numbers for examples. Currently CloudAPI is
at package.json version "8.2.1", but starting with version 8.0.0 for sanity
reasons, CloudAPI API-version handling just does the major versions. So
for example, ping looks like this:

$ triton -p nightly-1 cloudapi /--ping -i
HTTPS/1.1 200 OK
...
server: Joyent Triton 8.2.1
api-version: 8.0.0

{
    "ping": "pong",
    "cloudapi": {
        "versions": [
            "7.0.0",
            "7.1.0",
            "7.2.0",
            "7.3.0",
            "8.0.0"
        ]
    }
}

Happy to discuss with others, and to RFD-it if necessary, but my mini-proposal
is to:

1. stick with only exposing major versions for the restify API-versioning we
use (i.e. version negotiation with the Accept-Version and API-Version
headers); and
2. switch to "server: cloudapi/8.2.1 ..." for the "Server" header, which will
allow clients to reliably parse the "Server" header to determine the minor
or patch-level version, if it matters.

In our example, the `triton` CLI could use the Server header to watch for
at least cloudapi version 8.3 with this new "affinity" support before using
it.

The argument for #1 (i.e. not using Accept-Version and server.versions) with
our restify servers is the explosion of code changes. My understanding is
that adding new published API versions at the minor version level leads to
stuff like this in the code:

datasets.js
621:        version: ['7.0.0', '7.1.0', '7.2.0', '7.3.0', '8.0.0']
627:        version: ['7.0.0', '7.1.0', '7.2.0', '7.3.0', '8.0.0']
633:        version: ['7.0.0', '7.1.0', '7.2.0', '7.3.0', '8.0.0']
639:        version: ['7.0.0', '7.1.0', '7.2.0', '7.3.0', '8.0.0']
645:        version: ['7.0.0', '7.1.0', '7.2.0', '7.3.0', '8.0.0']
651:        version: ['7.0.0', '7.1.0', '7.2.0', '7.3.0', '8.0.0']
666:        version: ['7.0.0', '7.1.0', '7.2.0', '7.3.0', '8.0.0']

...

Comments

Comment by Former user
Created at 2017-08-24T07:27:17.000Z

@accountid:62561aa44f1d57006a24d40c, in the mini-proposal, do you also need a #3?

3. Add an affinity array to the CreateMachine API that:
a. conflicts with any provided locality object (or is used after all affinity constraints have been applied)
b. is order preserving and accepts the Docker swarm API-like affinity input strings

I am less concerned with how we version and negotiate the capabilities so much as I am with exposing the functionality via CloudAPI.


Comment by Former user
Created at 2017-08-24T19:05:27.000Z

@accountid:62562f2562802600683f33a8 I said:

The proposal for this ticket is to deprecate "locality" with CreateMachine in favour of a new "affinity" array of strings of the form supported by sdc-docker and the Triton CLI (see above). It would be an error to specify both "locality" and "affinity".

Does that cover your "3."?


Comment by Former user
Created at 2017-08-24T20:17:59.000Z

Yup. I got down to the enumerated part of under Happy to discuss with others, and to RFD-it if necessary, but my mini-proposal is to: and wanted to make sure that whomever picked this up didn't act on the enumerated part versus the meat of the suggestion above.


Comment by Former user
Created at 2017-09-12T07:03:41.000Z
Updated at 2017-09-12T07:06:03.000Z

@accountid:62561aa34f1d57006a24d409, @accountid:70121:628e32ce-80c2-4ddb-b37b-9905c709fbc3 FWIW, I've started taking a short crack at this. If I get anywhere by midday Tuesday, I'll let you know and show you what I've got Marsell.


Comment by Former user
Created at 2017-09-12T22:10:44.000Z

@accountid:62563d8856e5a8006dd983f4 Update: I have a first stab at this running in nightly-1 and @accountid:62562f24ed2b3e0074fca077 is doing the Terraform work to use it. I'm hoping to get the cloudapi part of this in for the Triton bi-weekly release this Thu.


Comment by Former user
Created at 2017-09-13T12:35:51.000Z

@accountid:62561aa44f1d57006a24d40c: I've started integrating and testing on nightly-1. I've found some edge cases with the filters themselves. We'll touch base when you're online.


Comment by Former user
Created at 2017-09-13T21:56:49.000Z

@accountid:62561aa44f1d57006a24d40c: Any word on when this might get released or any further work that it needs?

After today it looks like we're good on the `triton-go` and Terraform side of this support. I'd like to start testing on the production setup to definitively know.


Comment by Former user
Created at 2017-09-14T02:31:28.000Z

Just to recap for those not following along in chat, the pre-release @accountid:62561aa44f1d57006a24d40c has given me does implement the Docker API affinity rules under CloudAPI. Most of the rules are far more productive than the existing locality support, which is great. However, some of the placement rules, especially globs that match on multiple CNs, sometimes lead to unexpected results (even with the understanding that these rules are best guesses). This tends to show itself more prominently under Terraform considering it provisions in parallel by default. However, I believe some of the fixes that are still in play will likely help the situation, especially if a CN can be chosen out of a group and/or DOCKER-1039.

tldr; Terraform support is ready, this looks very promising, and hopefully just needs some adjustment. Will continue to track.


Comment by Former user
Created at 2017-09-16T17:25:01.000Z

@accountid:62562f24ed2b3e0074fca077 Is this as visible of a problem when you constrain Terraform's parallelism to 1? If so, we can actually implement a mutex in the provider to limit the concurrency if there are any locality constraints in use. If you have good results with terraform when -parallelism=1, let me know and I can either drop the patch or provide you with the necessary guidance to get that done.


Comment by Former user
Created at 2017-09-16T17:51:10.000Z

@accountid:62562f2562802600683f33a8 Overall, I don't really see Terraform as a problem to support this. I definitely welcome the suggestion though, locks certainly could help here and we can look into that together. I consider the provider work to be largely "feature complete" in terms of supporting the new functionality in CloudAPI (link to my branch).

But the fact that affinity in CloudAPI isn't yet deployed to a public endpoint makes this a tough call for me to pre-release to the public TF provider, no? Only someone with access to the nightly rig could even test this and hopefully the functionality is still on nightly-1.


Comment by Former user
Created at 2017-09-16T18:10:49.000Z

Let me add a bit, I feel like if there's an ability for an affinity rule with a glob to match on one CN in a set of CNs (or at least choosing a CN for continuing with resolving a set of rules) than what we have will most likely be feature complete. That was an expectation I had but it's definitely worth discussing other enhancements around this in the provider.


Comment by Former user
Created at 2017-09-17T05:32:19.000Z

@accountid:62562f24ed2b3e0074fca077 I don't think this is Terraform's bug, but Terraform can work around this by serializing all API requests for machine-related activities in order to increase the reliability and experience while using the tool. I'd rather see CreateMachine be serialized when an affinity rule is present, and therefore correct, versus buggy and frustrating. The workaround inside of the Triton provider is pretty simple: acquire a mutex if an affinity rule is in use. It's not the most graceful solution, but it would paper over this problem for Triton consumers using Terraform (the most apt combination of people to stumble over this). ? tty Monday.

https://github.com/terraform-providers/terraform-provider-postgresql/blob/0b04acbb63890b24c7a3669d7c4f49ab2b137a42/postgresql/config.go#L91-L96
https://github.com/terraform-providers/terraform-provider-postgresql/blob/master/postgresql/resource_postgresql_database.go#L112-L113


Comment by Former user
Created at 2017-09-23T00:03:57.000Z

@accountid:62562f24ed2b3e0074fca077

Sorry for the delay!

I have a CR up with a cloudapi impl that fixes the case of a rule matching multiple CNs. It also should display synchronous errors for cases where the rules are unresolvable. For example (using an unreleased node-triton that uses the new param):

$ triton create minimal-64-lts sample-128M -n tmp3 -a 'role==/^data/' -a 'instance==webhead3'
triton instance create: error: error creating instance: cannot satisfy affinity rule "instance==webhead3", its servers (44454c4c-5200-1037-8056-b4c04f524432) do not intersect with servers from previous rules (44454c4c-5700-1047-804d-b3c04f585131, 00000000-0000-0000-0000-002590918ccc)

$ triton create minimal-64-lts sample-128M -n tmp3 -a 'role==/^data/' -a 'instance==webhead3'
triton instance create: error: error creating instance: cannot satisfy affinity rule "role==/^data/", "!=" rules eliminate all its servers

This still has the DOCKER-1039 (racy) issue. Limitations are documented here: https://github.com/joyent/sdc-cloudapi/blob/97b3c9a69e413a6f339a1f9f2684ce56be0cf0dd/lib/triton-affinity.js#L58-L84

I'll try to get this in on Monday and up on nightly-1 for you to test with on Monday.


Comment by Former user
Created at 2017-09-25T23:35:31.000Z

Manual testing notes (in addition to the cloudapi integration and unit tests) against nightly-1:

Setup:

triton-select nightly-1

triton create -n webhead0 minimal-64-lts sample-128M -w -a 'instance!=webhead*'
triton create -n webhead1 minimal-64-lts sample-128M -a 'instance!=webhead*' -w
triton create -n webhead2 minimal-64-lts sample-128M -a 'instance!=*head*' -w
triton create -n webhead3 minimal-64-lts sample-128M -a 'instance!=*head*' -w

triton ls -j | json -ga compute_node id name tags -o jsony-0 | sort

triton create -n db0 minimal-64-lts sample-128M -a 'instance==webhead0' -t role=database -w
triton create -n db1 minimal-64-lts sample-128M -a 'instance==webhead1' -t role=database -w

triton ls -j | json -ga compute_node id name tags -o jsony-0 | sort

Tests:

triton create -n tmp0 minimal-64-lts sample-128M -a 'instance!=webhead3'
triton ls -j | json -ga compute_node id name tags -o jsony-0 | sort  # ensure not on webhead3's CN

triton create -n tmp1 minimal-64-lts sample-128M -a 'container*&#=webhead3'   # sync error

triton create -n tmp1 minimal-64-lts sample-128M -a 'container=webhead3'   # sync error

triton create -n tmp1 minimal-64-lts sample-128M -a 'container==webhead3'
triton ls -j | json -ga compute_node id name tags -o jsony-0 | sort  # ensure *on* webhead3's CN

triton create minimal-64-lts sample-128M -n tmp2 -a 'role!=~datab*se'
triton ls -j | json -ga compute_node id name tags -o jsony-0 | sort  # ensure not with a role=database

triton create minimal-64-lts sample-128M -n tmp3 -a 'role==/^data/' -a 'instance!=webhead*' # error "cannot satisfy"

triton create minimal-64-lts sample-128M -n tmp3 -a 'role==/^data/' -a 'instance==webhead3' # error "cannot satisfy"

triton create minimal-64-lts sample-128M -n tmp3 -a 'instance==webhead*' -a 'role==database' -a 'instance!=webhead3'
triton ls -j | json -ga compute_node id name tags -o jsony-0 | sort  # ensure works

Comment by Former user
Created at 2017-09-26T14:54:22.000Z

sdc-cloudapi commit fb7d393 (branch master, by Trent Mick)

PUBAPI-1428 CreateMachine: add "affinity" rules, deprecate "locality" hints
Reviewed by: Josh Wilsdon <josh@wilsdon.ca>
Approved by: Josh Wilsdon <josh@wilsdon.ca>


Comment by Former user
Created at 2017-09-26T18:07:03.000Z

sdc-cloudapi commit 4fc5ddb (branch master, by Trent Mick)

PUBAPI-1444 update recent PUBAPI-1428 docs with CR suggestions
Reviewed by: Josh Wilsdon <josh@wilsdon.ca>
Approved by: Josh Wilsdon <josh@wilsdon.ca>


Comment by Former user
Created at 2017-09-26T19:03:33.000Z

apidocs.joyent.com commit 9682894 (branch master, by Trent Mick)

PUBAPI-1428 CreateMachine: add "affinity" rules, deprecate "locality" hints
PUBAPI-1444 update recent PUBAPI-1428 docs with CR suggestions


Comment by Former user
Created at 2017-09-26T21:36:41.000Z

I did a substantial amount of testing today primarily centered around Terraform use cases with the new affinity rules. This feature has definitely come together and is much more usable than the old locality hints. @accountid:62562f2562802600683f33a8 was correct that removing parallelism helps the roll out of affinity rules through TF so I'm planning to do the integration work for that enhancement tomorrow AM.


Comment by Former user
Created at 2017-09-27T21:20:21.000Z

I've touched up and posted my PR on GitHub that implements Terraform support for this incoming feature. I'd really like to see this hit JPC before we merge but that's a nebulous discussion left to those with more input.

https://github.com/terraform-providers/terraform-provider-triton/pull/42


Comment by Former user
Created at 2017-09-28T01:29:06.000Z

@accountid:62561aa44f1d57006a24d40c Thanks for the reminder. Either way, the main public consumer I had in mind was HashiCorp's own testing account. I'm not sure if east3b is something they'd be able to target or not. Last time I checked they did need to clean up their usage of Triton (hanging test resources).


Comment by Former user
Created at 2017-09-28T20:06:51.000Z
Updated at 2017-09-28T20:07:00.000Z

@accountid:62562f24ed2b3e0074fca077 There is a per-account field to allow usage of us-east-3b. We could add that for whatever test/dev accounts HashiCorp has.