Bootstrapping and resources

[nwt]

Here's a first stab at bootstrapping (#290) in the form of a resource plugin API and example along with a bootstrap plugin.

The bootstrap plugin implements the group API for expediency, and I've only implemented CommitGroup for now. I'm pretty sure we want a different API so we can give that method a better name (Bootstrap? CreateResources?). I'm pretty unsure what other methods will be useful.

A schema example is in docs/bootstrap.json. Make it go with

make build-binaries &&
mkdir -p resources && {
    build/infrakit-resource-file --dir resources &
    resource_pid=$!
    build/infrakit-bootstrap &
    bootstrap_pid=$!
} &&
build/infrakit group --name bootstrap commit docs/bootstrap.json &&
kill $resource_pid $bootstrap_pid

[FrenchBen]

@nwt looks like you need to update the vendor.conf to include github.com/twmb/algoimpl/go/graph

[chungers]

More comments to follow, but for starters, can you add some tests?

The code in pkg/plugin/bootstrap, that's all for the resource plugin, right? Let's keep the naming consistent, so maybe rename the package to just pkg/plugin/resource?

The code that does the template and dependency analysis (via the topolgical sort) -- can we make that code reusable for any kind of templates that has the {{.Some.Reference.To.Object.Property}} notation? Maybe add this functionality to the pkg/template where we generalize and standardized on using the golang template? Just a thought.

[chungers]

This reference here, .Resource.resource_b.ID, isn't explicit -- in the sense that I can't find anywhere in the document that says it satisifies the contract of having a field called ID. This can be a problem in that it's impossible to validate this input (e.g, is it ID or maybe it's Id?)

Another concern here is that this expression .Resources.resource_b.ID is closely coupled to the structure of this document and if there's a shuffling in schema all the expressions will have to updated to reflect that.

Let me know if you agree. I have some ideas how we can mitigate this.

[chungers]

Another thing I would like to discuss is whether it makes sense to use a Golang template here as an input to the plugins. It's fine to use a Golang template and we plan to do more with it in the future, but I am less certain about plugins accepting a template (which may not be a valid JSON at all) as input because plugins communicate using JSON-RPC currently.

Your example here is a valid JSON because your reference is inside a string, but if I want to reference a complex object then something like

{
    "id" : {{.Resources.somethingElse.NumericValue}},
    "value" : {{.Resources.another_resource.ComplexObject}}
}

can't be marshaled and unmarshaled as proper JSON.

In general, golang templates work fine around the edges prior to becoming the input to the plugins. I think the best thing here is to introduce a struct into the document like:

{
    "id" : { "ref" : "somethingElse.NumericValue" },
    "value" : { "ref" : "another_resource.ComplexObject"}
}

This is similar to the approach taken by Cloudformation and Azure templates. This is a valid JSON and is easy to parse (no regex in your bootstrapper code). It can also be sent as-is as input via JSON-RPC. When it is evaluated, the reference nodes (the {ref}) can be replaced with actual values and the output is still a proper JSON.

[nwt]

I'd figured references would always occur inside strings, largely because in working with a prototype AWS resource plugin, IDs turned out to be the only useful dynamic value. That may well be an artifact of my use case, but it's worth thinking through now because IDs are the only dynamic value currently returned by Provision and DescribeInstances/DescribeResources.

[nathanleclaire]

FWIW, being able to interpolate strings directly (vs. Ref style) can be pretty nice, since you might want to do something like

{
    "Name": "{{.SwarmName}}-VPC-{{.Count}}"
}

to concat strings, whereas Azure ends up looking something like

{
    "Name": "[concat(variables('SwarmName'), 'VPC', count())]"
}

which I find a lot less readable. The tricky part is to determine how nested you'll get, since Terraform for instance has somewhat a problem where these giant monstrosities of join(split('/', concat(blah, blah, blah))) get built up, and a nested way to do it might be nicer... In that case, they partially have that issue because you need to use the string interpolation syntax to return other types such as lists. I find their use of some things like * really hard to reason about as well...

Ultimately I think it all depends on use case. Might be kinda nice to work towards getting a "short-hand" syntax for exclusively strings but also have Ref available (and other "object functions"...) when you need to do something slightly more complicated.

[nwt]

My scope for the reference feature has been limited to resource IDs, which seems sufficient for bootstrapping a group. Does that scope need to expand?

[chungers]

Sorry for the late reply. I think @nathanleclaire brought up some good points: having these structures at the start may be unnecessarily making things more complicated -- given that we are currently looking at just resource ID's. Let's keep this as-is for now. Thoughts?

[nwt]

Sounds good to me.

[chungers]

This is virtually identical to the instance spi, except for the addition of a resourceType in some of methods.

That you had to duplicate code tells me there's an issue with our current model. Perhaps we should visit the SPI of instance itself?

What if we can do something to the instance spi itself or change the way a plugin exposes the RPC endpoints, etc. so that all the different resource types all implement the same spi -- compute, networks, volumes, etc., that would be nice. Let me investigate this. Thoughts?

[nathanleclaire]

I'm just wandering in here but one idea I'd had about this kind of thing historically is to have a few different interfaces which dictate the behavior of various resources. For instance, the vast majority of instances can be Create-ed and Destroy-ed, but only some things (e.g., instances) can be Stop-ed and Start-ed...

Maybe your RPC server could accept attempted calls on a particular resource and throw issue if that particular plugin resource doesn't embed the given interface...

e.g.,

// Create, Read, Delete
type CRDer interface {
    Create() error
    Read() (json.RawMessage, error)
    Delete() error
}

type StartStopper interface {
    Start() error
    Stop() error
}

type Instance interface {
    CRDer
    StartStopper
}

type SecurityGroup interface {
    CRDer
}

(It's worth considering more uncommon verbs like Attach as well... You can actually Attach and Detach security groups from an instance. That's a totally different operation than just Create-ing some with given properties and could end up being pretty relevant for, e.g., VPC peering).

[chungers]

@nathanleclaire that's a good suggestion. Currently, the instance has the notion of an attachment, but that attachment / detachment is implicit inside the implementation of the instance plugin. I wonder if it would better to make that more explicit and use the interface mechanism you described here.

[FrenchBen]

We may want to simplify this to follow what Microsoft does in Azure for their resource type:
"type": "Microsoft.Storage/storageAccounts"

The above would become:

"resource_b": {
  "Plugin": "resource-file/b",
  "Properties": {
  }
}

This would also allow us to re-use the instance type plugin

[chungers]

This is a pretty good idea.

With a little plumbing change, we can make it work so that a single plugin endpoint (eg. aws) can export multiple instance plugin objects, each one bound to a type (e.g. subnet). So you could then use the Plugin name of aws/subnet and it will lookup the plugin with socket file aws and then route the RPC to the instance.Plugin implementation that's bound to the type name subnet.

There's something nice about this -- a single unified spi for immutable resource. The existing instance plugins for provisioning compute instances is just a sub-type exported. With the way we implement the RPC server and how the plugin objects are exposed, you can even introspect and see what 'types' an instance plugin can handle.

Let me put together a PR as a proof of concept. If that looks good, then it will save us a lot of duplication of boilerplate code.

[nwt]

I like it, too.

Another option is to let plugins infer the type from what they find in Properties.

I don't know which I prefer, but both are better than a Type field.

[chungers]

See my PR #360 -- which implements this idea.

[nwt]

looks like you need to update the vendor.conf to include github.com/twmb/algoimpl/go/graph

@FrenchBen: I sure did. Done in c88e0f1.

[nathanleclaire]

FWIW, Go convention is to avoid snake_case, this would usually just be spiresource (standard lib has httptest for instance)

[nwt]

The code in pkg/plugin/bootstrap, that's all for the resource plugin, right? Let's keep the naming consistent, so maybe rename the package to just pkg/plugin/resource?

@chungers: Done in 0b338e9. I also renamed cmd/bootstrap to cmd/resource. Happy to undo that if that wasn't part of what you were thinking.

[chungers]

@nwt - how about this... let's not make the resource plugin implement the group SPI. Rather, let's define a new SPI:

1. Add a new package: pkg/spi/resource
2. Let's create a new spi.go and define something with what we for sure need for the resource plugin:

type Plugin interface {
    Commit(spec *resource.Spec, pretend bool) (plan string, err error)
    Destroy(resource resource.ID) error
    DescribeResources() ([]resource.Description, error) 
}

In essence, the Resource plugin manages a collection of resources (which are created by the instance/ plugins). It does things like analyze dependencies among this collection of resources in the spec JSON and calls the instance plugin for creation, etc. It also provides the point of contact for end users to list, commit and destroy resources.

[nwt]

Sounds good to me.

[nwt]

@chungers: Do we want to reuse instance.ID and instance.Description in this interface (instead of defining resource.ID and resource.Description)?

And do you envision that the resource plugin will maintain a mapping from ID to instance.Plugin for dispatching Destroy, similar to the group plugin?

[chungers]

Considering the resource plugin delegates resource CRD (create/read/destroy) to the instance plugin, using instance.ID may make sense.

On the issue of Destroy -- I am not so sure if destroying individual resources (the Destroy method with an id) makes a lot of sense now... How about a Destroy(spec *resource.Spec, pretend bool) instead? This is effectively the inverse of commit and we should be able to implement this by computing the reverse order of resource creation. This should also work around the problem of maintaining association of Id to the instance plugin, right?

One thing we are ignoring for now is the issue that some resources cannot be destroyed if the code is running from within or on that resource -- eg. destroying a VPC in which the host that calls the destroy is running.

What do you think? Can you make this change and add the implementation to reverse the topological sort for destroying all the resources?

[nwt]

I like it. I'll get started on an implementation (with tests).

[chungers]

See comment below on creating a new SPI for resource. Ideally this package won't have any import dependencies on the group pkg. All the specs and types that currently reference the group pkg should just be recreated and put in the resource pkg. This way, the resource plugin SPI is free to evolve on its own.

[chungers]

Can you add a couple of tests for this with some example input? In particular, the provisioning order (dependency) code can use a few tests to show usage and get some test coverage. Thanks!

[nwt]

Will do.

[nwt]

@chungers: fbab575 implements the resource SPI proposed in #357 (comment).

I still owe tests but wanted to share this sooner rather than later.

I also took a shortcut in cli/infrakit/resource.go by calling pkg/plugin/resource.NewResourcePlugin directly instead of using RPC. That's just temporary.

[nwt]

@chungers:

1. Are you happy with what I've done in cmd/cli/resource.go (calling pkg/plugin/resource.NewResourcePlugin directly), or would you prefer to have a cmd/resource/main.main?
2. If we're going to keep resource.Plugin.DescribeResources in the interface, I think we should change the signature. What would you say to this?

DescribeResources(spec Spec) (plan string, err error)

[codecov]

Codecov Report

Merging #357 into master will decrease coverage by 0.24%.
The diff coverage is 83.26%.

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
- Coverage    70.1%   69.85%   -0.25%     
==========================================
  Files          27       74      +47     
  Lines        1378     4332    +2954     
==========================================
+ Hits          966     3026    +2060     
- Misses        312     1032     +720     
- Partials      100      274     +174

Impacted Files	Coverage Δ
pkg/rpc/resource/service.go	73.33% <73.33%> (ø)
pkg/plugin/resource/plugin.go	83.42% <83.42%> (ø)
pkg/rpc/resource/client.go	92.85% <92.85%> (ø)
pkg/types/link.go	3.84% <0%> (ø)
pkg/types/path.go	77.45% <0%> (ø)
pkg/plugin/group/testplugin.go	76.19% <0%> (ø)
pkg/template/util.go	63.63% <0%> (ø)
pkg/rpc/event/client.go	62.5% <0%> (ø)
pkg/plugin/group/scaler.go	91.21% <0%> (ø)
pkg/types/reflect.go	72.09% <0%> (ø)
... and 43 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edad9dd...a6a2eba. Read the comment docs.

[chungers]

@nwt

Are you happy with what I've done in cmd/cli/resource.go (calling pkg/plugin/resource.NewResourcePlugin directly), or would you prefer to have a cmd/resource/main.main?

Since this is a separate plugin /binary / daemon, I think it's better that we follow the convention in the cmd/manager or cmd/group. So I'd go with your second option -- cmd/resource/main.go, etc.

If we're going to keep resource.Plugin.DescribeResources in the interface, I think we should change the signature. What would you say to this?
DescribeResources(spec Spec) (plan string, err error)

I think this is reasonable. Let's go with your proposal.

[nwt]

@chungers: I just saw the comment in https://github.com/docker/infrakit/blob/master/pkg/manager/spec.go about avoiding maps in user-facing representations. In light of that, should we change the syntax from

{
  "ID": "Fancy Resources",
  "Properties": {
    "Resources": {
      "A": {
        "Plugin": "instance-file",
        "Properties": {}
      },
      "B": {
        "Plugin": "instance-file",
        "Properties": {
          "Note": "Depends on {{ resource `A` }}"
        }
      }
    }
  }
}

to

{
  "ID": "Fancy Resources",
  "Properties": {
    "Resources": [
      {
        "Name": "A",
        "Plugin": "instance-file",
        "Properties": {}
      },
      {
        "Name": "B",
        "Plugin": "instance-file",
        "Properties": {
          "Note": "Depends on {{ resource `A` }}"
        }
      }
    ]
  }
}

?

[chungers]

@nwt - Yes. Sorry I missed this, but it is preferable that we use a list instead of a map. This is because with a list we known every element struct contains all of the fields. That the system may represent it internally as a map is really a matter indexing choices/ implementation detail. So if you could follow this convention, that'd be great.

Let's use this convention

"Resources": [
      {
        "ID": "A",
        "Plugin": "instance-file",
        "Properties": {}
      },
      {
        "ID": "B",
        "Plugin": "instance-file",
        "Properties": {
          "Note": "Depends on {{ resource `A` }}"
        }
      }
  ]

So, for an infrakit object, we can always expect an ID field that is a peer of Properties, the same way we expect a Plugin field to denote the plugin responsible for its management.

[nwt]

@chungers: I've switched from map to list as discussed in #357 (comment).

Is there anything else you'd like to see in here? Manager integration, maybe?

[chungers]

LGTM - thanks for doing this and sorry it took so long to merge this.

[fermayo]

🎉 😍