Aks-engine: upgrade command doesn't update addon images

[mo-saeed]

Is this a request for help?: Yes

---

Is this an ISSUE or FEATURE REQUEST? (choose one): ISSUE

---

What version of aks-engine?: 0.33.2

---

Kubernetes version: 1.13.5

What happened:

• existing cluster with aks-engine 0.33.2
• upgrading cluster with aks-engine 0.36.1
• addons section in api-model.json wasn't updated with the new version of the addons images included in 0.36.1 version
• addons like cluster-autoscaler kept with the same old version "1.13.2" not the new one "1.13.4"

What you expected to happen:
upgrade command should update all addons images

How to reproduce it (as minimally and precisely as possible):

• existing cluster with aks-engine 0.33.2
• upgrade cluster with aks-engine 0.36.1

Anything else we need to know:

[CecileRobertMichon]

@mo-saeed I couldn't repro this behavior. Here's what I get:

• Created a 1.11.9 cluster with aks-engine v0.33.2
• Apimodel has "image": "k8s.gcr.io/cluster-autoscaler:v1.3.7",
• Upgrade to 1.11.10 with aks-engine 0.36.2 (should not be different from 0.36.1)
• Apimodel now has "image": "k8s.gcr.io/cluster-autoscaler:v1.3.8",

However, jumping on the cluster, I do see that the cluster-autoscaler pod is still using Image: k8s.gcr.io/cluster-autoscaler:v1.3.7 but that the image in /etc/kubernetes/addons/cluster-autoscaler-deployment.yaml is 1.3.8. Looking into this.

I've added some unit tests at https://github.com/Azure/aks-engine/pull/1385 to validate the default image behavior for upgrade and scale scenarios.

[CecileRobertMichon]

Update: the cluster-autoscaler deployment addonmanager.kubernetes.io/mode was EnsureExists. That's why cluster-autoscaler didn't get the latest /etc/kubernetes/addons yaml after the upgrade. I changed it to Reconcile in #1385 and will audit the code for other addons using EnsureExists.

[mo-saeed]

@CecileRobertMichon
1- Apimodel change
yes you are right, I think i know now why it wasn't updated in my case. the cluster autoscaler in my existing cluster was disabled "enabled": false, I did a test and modified it manually to true and did the upgrade then the apimodel got the correct autoscaler image version.
I am not sure if this should be the behaviour anyway, for me i would say the image version should be updated in both cases (autoscaler enabled or disabled).

2- Actual change in cluster
Thank you for figuring this out, I think this fix needs to be applied for all other addons besides the cluster-autoscaler, for example after upgrade i still have the older version of networkmonitor image containernetworking/networkmonitor:v0.0.5

Thank you !

[CecileRobertMichon]

Updated other addons that had either Deployments or DaemonSets set to EnsureExists in the same PR, including networkmonitor.

[CecileRobertMichon]

I am not sure if this should be the behaviour anyway, for me i would say the image version should be updated in both cases (autoscaler enabled or disabled).

I agree, although it doesn't have any functional impact it's counter-intuitive. My guess is that there's an if enabled somewhere that we can remove.

[mo-saeed]

@CecileRobertMichon is there any updates regarding the addons upgrade issue ? this one https://github.com/Azure/aks-engine/pull/1401 should fix it ?

If possible, I have also a question regarding the addons: is azure-ip-masq-agent necessary to be installed in case of using azure-cni network plugin (not kubenet) ? as per my understand it can be only necessary incase of any plugin using basic networking (e.g. kubenet...) as NAT is needed

[CecileRobertMichon]

1385 addressed cluster-autoscaler only. #1401 is to audit all the addon specs and set reconcile where appropriate. After discussion, we realized that it didn't make sense to have reconcile set for only part of the spec (eg. the deployment resource) since aks-engine new versions might contain updates to the rest of the spec that go hand in hand with the new image.

azure-ip-masq-agent is currently installed if k.NetworkPlugin != NetworkPluginCilium. This might be a bug, I agree with you that it probably shouldn't be installed for azure CNI. @jackfrancis is working on a pretty big refactor in #1409 so we should wait for that to go in first before we change the default.