rancher / system-upgrade-controller

In your Kubernetes, upgrading your nodes
Apache License 2.0
723 stars 86 forks source link

Add ownerReferences to Job metadata #220

Closed tsde closed 1 year ago

tsde commented 1 year ago

This PR adds the ownerReferences field to the jobs, pointing to the corresponding plan that generated them. This allows to link the jobs to the plan and optimize GC, just like between a replicaset and its pods.

This is also useful for visualization in tools like ArgoCD. Without this field, you only get a view of the plan itself without its associated jobs: without_ownerref

With the field set, we have greater visibility about what's currently happening through the plan and can see the jobs running: with_ownerref

This could be considered as a breaking change as deletion of the plan implies deletion of the associated jobs, so you can potentially lose some kind of information if you're not careful. Using an optional flag to enable this could be implemented if needed

As for the change itself, I'm far from being a golang warrior so there may be a better way to implement this. The same goes for the go testing framework that I'm definitely not used to ^^, so I don't really know how to implement a test for this change (or maybe there's no need to for such a change).

brandond commented 1 year ago

I'm a little confused, because it looks like the wrangler apply controller should already be set up to do this - the WithOwner(obj) call should already set a reference to the plan. https://github.com/rancher/system-upgrade-controller/blob/ac25caa9da7f6ab5ce6298cc8e2e0bc7e8b120eb/pkg/generated/controllers/upgrade.cattle.io/v1/plan.go#L372-L375

I suspect that the issue may be that generic.GeneratingHandlerOptions's NoOwnerReferences field seems to be a bit unintuitively named, and if you set it to true then the apply will actually set a controller reference? As per the code at https://github.com/rancher/wrangler/blob/406367dc3d74082c8a6e12dea6a5f82962956080/pkg/generic/generating.go#L24-L26 https://github.com/rancher/system-upgrade-controller/blob/ac25caa9da7f6ab5ce6298cc8e2e0bc7e8b120eb/pkg/upgrade/handle_upgrade.go#L98-L100

brandond commented 1 year ago

In classic Darren style that field was added with no commentary as to why it seems to function opposite of how its named, in this commit: https://github.com/rancher/wrangler/pull/77/commits/47235df3f622ea514c013bc3fcdce5a7af5ec427

brandond commented 1 year ago

Oh right, the apply controller's WithOwner stuff uses the custom objectset.rio.cattle.io/owner-* fields; if you want a proper Kubernetes owner reference you need to use WithSetOwnerReference as well which is what's gated on that poorly-named NoOwnerReferences option.

brandond commented 1 year ago

Sure enough, check out https://github.com/rancher/system-upgrade-controller/pull/235 - that's all it takes to get this:

    name: apply-test-plan-on-k3s-server-1-with-c1fd4666ae0c7dfb25eb-83f45
    namespace: kube-system
    ownerReferences:
    - apiVersion: upgrade.cattle.io/v1
      blockOwnerDeletion: false
      controller: true
      kind: Plan
      name: test-plan
      uid: 6ecb288e-3089-4ecf-98a3-50b1a16167de
brandond commented 1 year ago

Closing - fixed in https://github.com/rancher/system-upgrade-controller/pull/235