Hot loop when using Recreate update strategy with Go hook

[enisoc]

As reported by @cohix, a lambda hook written in Go that generates a JSON response by serializing the Go structs from the Kubernetes API package can trigger a hot loop of child recreation because Metacontroller thinks the hook wants to set metadata.creationTimestamp to null.

This shows up in the Metacontroller logs as something like:

manage_children.go:143] reflect diff: a=observed, b=desired:

object[metadata][creationTimestamp]:
  a: "2018-08-20T21:21:50Z"
  b: <nil>

This happens because Metacontroller's apply semantics currently assume any field present in the hook response is a field the hook author explicitly wants to set. However, metadata.creationTimestamp is always serialized into the hook response regardless of whether the author wants it, because CreationTimestamp in the ObjectMetastruct is a non-pointer struct field, so the omitempty option has no effect:

https://github.com/kubernetes/apimachinery/blob/dcde72c465a0edef321bedc44fe1c16990970efe/pkg/apis/meta/v1/types.go#L175

Incidentally, metav1.Time has a custom JSON marshaler that serializes the zero value of the struct to null instead of "0001-01-01T00:00:00Z" (as you'd expect given it's a non-pointer field), but this doesn't change anything as far as Metacontroller is concerned. The problem is that the field gets serialized at all; we would prefer that it gets omitted.

---

At a high level, marshaling to JSON from a statically-typed language like Go makes it hard for hook authors to follow the guidance from Metacontroller that they should omit fields they don't care about. You would have to diligently ensure that all fields have proper omitempty semantics for uninitialized struct fields. In Go, that essentially requires using pointers for all struct fields, but if you import the structs from upstream Kubernetes, you don't have control over that.

One option could be to change Metacontroller so it doesn't try to update the field if the desired value matches the last-applied value. I actually thought it already does that because the intention was to match kubectl apply semantics, but then I remembered that Metacontroller is more aggressive about pushing towards desired state on purpose -- controllers generally ought to be more persistent than kubectl. Most controller authors would probably expect that their controller should "reset" the field back to the desired value if someone else changes it. With that said, I don't think we should rule out this option.

Another option is to somehow work around the problem by figuring out that, for some fields, null really means "I don't care" and not "please set this to null". However, that would require specific knowledge of the schema of objects being processed, which Metacontroller so far manages to avoid.

[danisla]

Is there any workaround for this? I’m using the kubernetes go library in my golang hook and seeing this same issue.

[enisoc]

To unblock people who prefer to write hooks in Go, I've proposed #94 which enforces the invariant that Metacontroller should never try to update read-only, system-populated metadata fields. That should fix the immediate issue here with creationTimestamp, but it remains to be seen if we'll encounter other problems outside ObjectMeta where the Kubernetes Go API failed to use pointers for omitempty struct fields.

[enisoc]

As mentioned above, the fix in #94 wouldn't work if there are other fields outside ObjectMeta that don't get omitted when left on their Go zero values.

For Pod, it seems like we are safe, but @danisla has found a new example in ReplicaSet (status.replicas is intentionally not omitempty), which shows that there will likely be other examples scattered throughout the Go API structs.

Given that, I'm leaning towards the other alternative proposed in the first post of this issue, which is to make Metacontroller's apply semantics match kubectl apply more closely in this scenario:

1. You apply something like replicas: 0.
2. Something else (e.g. Horizontal Pod Autoscaler) edits the object to set replicas: 5.
3. You apply again, but you still say replicas: 0.

In the case of kubectl apply, the HPA "wins" and your second apply leaves replicas at 5. That's because when you apply the same value as last time for a given field, kubectl assumes you mean that you don't care to change that field right now, so it remains at whatever value the live object on the server has.

By contrast, currently in that scenario Metacontroller assumes the controller is trying to maintain the state replicas: 0 even in the face of outside interference. I thought it would be surprising to a controller author if, for example, the user directly alters a field (e.g. with kubectl edit), and the controller doesn't reset it back to the "right" value. After all, the job of a controller is to continually push towards the desired state.

If we instead make Metacontroller match the kubectl apply behavior, it should prevent the whole class of endless updates discussed in this issue, but it might cause surprises in the other direction where people expect updates and they don't happen.

If any users want to weigh in on which behavior would be least surprising, that would help.

[danisla]

I think most of my use cases would prefer to match the behavior of kubectl apply. Can we have both and let the author pick the appropriate behavior via the controller config?

[rlguarino]

The example is a little weird, because HPA isn't going to be updating the status.replicas it will be updating the spec.replicas and spec.replicas does have omitempty set properly.

I think it depends on the update strategy for me. All of the custom controllers I've written use the Update method and want to reset the values on every write. If two controllers are in contention it's usually a bug and one of them needs to use a different way to update the field. e.g update the parent resources and let it's controller own things instead of trying to update the child directly.

I tend to think kubectl apply semantics are more useful when a human is on the other side of the client.

[enisoc]

The example is a little weird

Sorry for using a confusing example. I actually did mean spec.replicas in the HPA example, but I was using it as a stand-in for a hypothetical spec field that is not omitempty. Given that we found status.replicas is not omitempty, I'm worried there are also spec fields lurking in the API that are not omitempty.

I tend to think kubectl apply semantics are more useful when a human is on the other side of the client.

Thanks for weighing in. This seems to confirm my suspicion that there are people out there who would reasonably be surprised by Metacontroller giving up so easily on enforcing what you ask it to.

@danisla wrote:

Can we have both and let the author pick the appropriate behavior via the controller config?

We could do that, but I'm worried that it will be difficult to know when you should use that setting, especially if the failure mode for using the wrong setting is endless updates. It would also force you to choose "give up semantics" instead of "fight semantics" just because you wrote your hook in Go. Even if we support both semantics, the language you use really ought to be orthogonal to your choice of apply semantics.

Maybe we can do a grep survey of existing Kubernetes APIs and see if my fear that there are non-omitempty spec fields is founded. If it's only status fields, we can introduce special behavior for that section of the object. Of course, it's always possible that new API fields will be introduced that break the rule. Also, people can easily make that mistake while writing their own Go structs for custom APIs.

[danisla]

So my temporary workaround is to copy the child type structs from the k8s go API to my controller and just omit their Status field so that when it's marshaled to JSON, the status field is omitted. This feels like a hack, but it emphasizes the importance of knowing how your data gets serialized with Go.

Maybe we can communicate some of these golang and k8s API caveats in the metacontroller docs?

[danisla]

Another thing that bit me and caused a hot loop was not having a stable serialization of my parent spec.

My spec contained a field of typemap[string]string, which has no order when being marshaled to JSON. As a result, the metacontroller.k8s.io/last-applied-configuration would change frequently because the order of the map items would get shuffled by the encoder. This was fixed by converting my parent CRD to a list of structs rather than a map. Not sure if there is any way around this, or if metacontroller had some way of creating a more stable encoding of the last-applied-configuration.

[enisoc]

@danisla Although you may observe the order of map fields changing in the last-applied-configuration annotation over time, the way we check diffs to see if an update is required should ignore map ordering. So if your hook response is the same except for map ordering, Metacontroller should decline to do an update.

What you might be seeing is that something else (perhaps one of the issues above) is causing endless updates, and the map order changing is a side effect but not actually causing the updates. If you've really eliminated all the known causes of update hot loops, and Metacontroller still is choosing to do an update solely because you returned map items in a different order, that would be a bug in Metacontroller. Please let me know if you have a reproduction for that.

[danisla]

Thanks @enisoc, I'll keep debugging now that I know the map order shouldn't affect it.

[luisdavim]

Hi, I'm running into this hot loop as well could it be because the status of my resources have conditions that include a lastProbeTime? Something like this:

status:
  conditions:
  - lastProbeTime: "2019-05-22T14:15:49Z"
    status: "True"
    type: Foo
  - lastProbeTime: "2019-05-22T14:15:49Z"
    status: "False"
    type: bar

[luisdavim]

BTW, I'm using https://github.com/tidwall/gjson and https://github.com/tidwall/sjson so the problem is not the same as originally reported. I have a const with an empty template of the child resource I want to generate and then I use sjson.Set() to populate it. something like this:

const cnameCRD = `{"apiVersion": "example.com/v1","kind": "Cname","metadata": {"name": "","namespace": ""},"spec": {"record": "","target": ""}}`

// newCname Returns a new CNAME CRD in json format
func newCname(name, cname string) string {
	nameParts := strings.Split(name, ".")

	crd, _ := sjson.Set(cnameCRD, "metadata.name", nameParts[0])
	crd, _ = sjson.Set(crd, "metadata.namespace", nameParts[1])
	crd, _ = sjson.Set(crd, "spec.record", cname)
	crd, _ = sjson.Set(crd, "spec.target", name)

	return crd
}

[luisdavim]

And this is my response to metacontroller:

{
  "status": {
    "cnameRef": "",
    "hostsStatus": [],
    "conditions": [
      {
        "lastProbeTime": "2019-05-22T14:48:06Z",
        "status": "False",
        "type": "GSLBResolves"
      },
      {
        "lastProbeTime": "2019-05-22T14:48:06Z",
        "status": "False",
        "type": "CNAMEResolves"
      }
    ]
  },
  "children": [
    {
      "apiVersion": "example.com/v1",
      "kind": "Cname",
      "metadata": {
        "name": "service-1",
        "namespace": "new-crd"
      },
      "spec": {
        "record": "new-crd.dev.example.com",
        "target": "service-1.new-crd.dev.example.com"
      }
    }
  ]
}

[AmitKumarDas]

@luisdavim I have been using k8s.io/apimachinery's unstructured.Unstructured to send the desired response. So far I dont think I have encountered above issues. In fact I am avoiding use of k8s APIs totally. I am once again relying on unstructured's helper functions to extract the values when needed versus going typed go way.

[luisdavim]

@AmitKumarDas cool, do you have an example?

[AmitKumarDas]

https://github.com/mayadata-io/cstorpoolauto cc @luisdavim

Let me know if you need exact snippets