@piosz @mwielgus
Ref #23470

Process question:
It looks like this was LGTM with an unresolved question, and then was manually merged? Why the rush?

We should make kubectl apply work (probably needs a couple improvements, which we want anyway) and/or switch to Deployment Manager.

For (b), the kubernetes.io/cluster-service label should be removed from the pod template if that's used for reconciling.

What does the resizer base its decisions on? Anyway, we could probably just remove those values from the spec and use the LimitRange for the namespace.

Apply issues we'd probably want to fix: #19805, #19809, #22342, #17238, #16569

And cascading deletion should not be implemented in the client -- we plan to fix that in 1.3. #12143

Is there a reason we can't revert the PR for now?

That was a pretty terrible review :(. Sorry about that.

a) Without name change we will never update Deployment object

The comment states that updates for non-renamed addons is not implemented. I missed that. I had initially recommended against the rename update for deployments since that's not the intended usage of deployments and I'm pretty sure it wouldn't work without modifying the addon updater. In the transition we would have two deployments fighting over the replicasets. Attempting to delete the old deployment would delete the new replicaset.

b) Once we add reconciling ReplicaSet object in addon updater it will clash with Deployment controller - controller will create ReplicaSet, which will be delete by addon updater as it's not explicitly listed in yaml files

Agree with Brian, only top level addon objects should carry the annotation.

c) Even if we fix addon-updater to simply compare object spec (so that we don't have to rely on object name changes) we have addon-resizer, that will modify object spec (request/limit fields) that will show as a difference when comparing spec and addon-updater will override it

We should modify an annotation when we want the deployment updater to reapply changes. We can compare the annotations, not the spec. That should solve this issue.

I will submit a revert.

I'm pretty sure it wouldn't work without modifying the addon updater. In the transition we would have two deployments fighting over the replicasets. Attempting to delete the old deployment would delete the new replicaset.

This might work if we update the selector between deployment versions. It would make me sad that we would use deployment this way. Is this stuff required for 1.2?

@bgrant0607 sorry about the manual merge. I'm new to github, and thought the "merge" button was how we quicked the merge bot. I was unaware that "lgtm" meant auto-submit to our merge bot. I am unaware which question was unresolved, but it wouldn't greatly surprise me (a PR's flat comment space seems sub-optimal for multitasking).

@fgrzadkowski @mikedanese Why don't we just name the heapster deployment with a version in 1.2.x? The current use of deployments is to change resources, not software versions. When we want to support deployments properly for all addons, then we can remove the version from the name.

#23512

@Q-Lee was this fixed by #23512? If so, we should close it.

@roberthbailey No, I don't see that anything was done to fix this.

I manually verified addon updates worked when I reviewed #23512. a, b, and c in the issue description are no longer a problem. This is the best we can do without making major changes to addon manager (#23233).

I left it open because kube-addon-updater.sh still doesn't behave how we want with Deployments. But the addon-resizer+heapster are currently working as Deployments in another way.

That's a bullet in #23233 so we can track it there