Group updater: Flavor changes don't register as Instance changes about deploykit HOT 3 CLOSED

Capturing some context, the problem surfaces here:

type Spec struct {
    Instance InstancePlugin
    Flavor   FlavorPlugin
}

func (c Spec) InstanceHash() string {
    unstable, err := json.Marshal(c.Instance)
    ...

Only Spec.Instance is used to generate a hash for an instance's configuration, ignoring Spec.Flavor. This should make it obvious why flavor changes don't induce updates. Unfortunately, the fix is not as simple as including Spec.Flavor to generate that hash. This is because the Flavor configuration is required to produce the flavor.AllocationMethod, meaning most Flavor Specs have something like a Size or IPs field.

As an example, consider this configuration:

{
    "ID": "cattle",
    "Properties": {
        "Instance": {
            "Plugin": "instance-file"
        },
        "Flavor": {
            "Plugin": "flavor-vanilla",
            "Properties": {
                "Size": 5,
                "Init": [
                    "echo hello"
                ],
            }
        }
    }
}

To scale the cattle group up, we could increase the Size field. If the code snippet above used Spec.Flavor to produce a hash of the configuration, the inner Size field would be indistinguishable from other Flavor property changes. The result would be rolling 'updates' of instances that did not change.

I see a few possible paths to address this:

(1) Support configuration canonicalization in the Flavor SPI

The goal here would be to ask the Flavor to identify and remove configuration fields related to scaling.
For example, change this flavor.Plugin function:

type Plugin interface {
  Validate(flavorProperties json.RawMessage) (AllocationMethod, error)
}

to:

type Details struct {
  AllocationMethod AllocationMethod
  CanonicalInstanceForm *json.RawMessage
}

type Plugin interface {
  Validate(flavorProperties json.RawMessage) (Details, error)
}

With the expectation that the Flavor strips out any configuration fields relevant to the Group rather than individual Instances. This results in a small (though non-trivial) additional burden of complexity on Flavor implementations.

(2) Move AllocationMethod configuration to a first-class Group construct

Remove the AllocationMethod return value from flavor.Plugin:

type Plugin interface {
  Validate(flavorProperties json.RawMessage) (AllocationMethod, error)
}

and add an Allocation field to Group Spec:

 type Spec struct {
       Instance   InstancePlugin
       Flavor     FlavorPlugin
       Allocation AllocationMethod
}

Note: this would partially undo #167 (i would recommend the AllocationMethod remain specific to our Group implementation). I think there was merit to that PR, but it complicated the problem we have here.

(3) Use the instance.Spec result from flavor.Prepare() to identify changes

Flavors already effectively distill their configurations into instance.Spec:

type Plugin interface {
  Prepare(flavorProperties json.RawMessage, spec instance.Spec) (instance.Spec, error)
}

so we might be able to exploit that.

This would stray from the current model of only using the user's configuration as an input to recognize that an instance has been changed. However, it is actually a more accurate signal and captures more reasons an instance might be configured differently. Implementing this is not so easy, however, as we would ideally be able to construct an instance.Spec from live instances, which we do not have. Additionally, since Flavors and Instances can both modify instance.Spec fields, it may prove awkward to identify modifications specifically made by the Flavor.

Recommendation

From the above options, i suggest (2) and possibly exploring (3).
(1) seemed attractive at first, but it places unnecessary burden on plugin implementations
(2) is relatively straightforward and may turn out to be necessary anyway, especially if we want to build support for composition of Flavors.
(3) is the most future-proof solution i've thought up, but a clean approach is not obvious.

from deploykit.

I agree that 2) is the first step to take. It properly separates the configuration from allocation in the flavor plugin. Allocation method should be left specific to the group implementation and I think that's a very reasonable expectation of a group plugin.

As for 3) I agree that it may be the most semantically correct as the config of an instance is a sum of expected operations by instance AND flavor. However i think doing that implicitly by only looking at the result of flavor's prepare doesn't make obvious the way 2) does, which I think solves most of the use cases for now anyhow.

Another point to consider is that prepare of a flavor may not be idempotent in particular due to having to assign an attachment or setting the logical Id. Depending on how group computes the delta this would make flavor implementation complicated. So better off start off with 2) first and separate out the parts that are most relevant to detect a change from the parts that don't and often involve some state (like alloc of ip or a volume id for attachment). Thinking out loud Allocation method may even become a separate plugin type - something to ponder for a future PR...

from deploykit.

Another point to consider is that prepare of a flavor may not be idempotent

My thoughts exactly. We would need to insist that Prepare() be a pure function, which may not be practical.

from deploykit.