Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

helm/azure: Fatal error for CNI Azure installation #13024

Merged
merged 1 commit into from Sep 4, 2020

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Aug 31, 2020

Setting global.azure.enabled will change operator image to
operator-azure, which only accepts azure IPAM, and throw
fatal error for cluster-pool.

This PR is to enable node init azure step for both cases (e.g. CNI
and Azure IPAM)

Closes #13023

Signed-off-by: Tam Mach sayboras@yahoo.com

helm/azure: Fix fatal error for CNI Azure installation

TODO

  • Follow GSG for azure one more time
Testing Azure CNI Chaining
$ helm install cilium install/kubernetes/cilium \
  --namespace cilium \
  --set global.cni.chainingMode=generic-veth \
  --set global.cni.customConf=true \
  --set global.nodeinit.enabled=true \
  --set nodeinit.azure=true \
  --set global.cni.configMap=cni-configuration \
  --set global.tunnel=disabled \
  --set global.masquerade=false
NAME: cilium
LAST DEPLOYED: Mon Aug 31 22:45:39 2020
NAMESPACE: cilium
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
You have successfully installed Cilium.

Your release version is 1.8.90.

For any further help, visit https://docs.cilium.io/en/v1.8/gettinghelp

$ kgpoowiden cilium                                                  
NAME                               READY   STATUS    RESTARTS   AGE     IP            NODE                                NOMINATED NODE   READINESS GATES
cilium-5hwmr                       1/1     Running   0          2m29s   10.240.0.35   aks-nodepool1-14356785-vmss000001   <none>           <none>
cilium-lj9fw                       1/1     Running   0          2m29s   10.240.0.4    aks-nodepool1-14356785-vmss000000   <none>           <none>
cilium-node-init-b28ts             1/1     Running   0          2m29s   10.240.0.4    aks-nodepool1-14356785-vmss000000   <none>           <none>
cilium-node-init-brjrn             1/1     Running   0          2m29s   10.240.0.35   aks-nodepool1-14356785-vmss000001   <none>           <none>
cilium-operator-65456488b8-74p79   1/1     Running   0          2m29s   10.240.0.35   aks-nodepool1-14356785-vmss000001   <none>           <none>
cilium-operator-65456488b8-z645x   1/1     Running   0          2m29s   10.240.0.4    aks-nodepool1-14356785-vmss000000   <none>           <none>

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 31, 2020
@sayboras sayboras added area/helm Impacts helm charts and user deployment experience release-note/bug This PR fixes an issue in a previous release of Cilium. release-note/misc This PR makes changes that have no direct user impact. labels Aug 31, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 31, 2020
@sayboras sayboras added the area/azure Impacts Azure based IPAM. label Aug 31, 2020
@sayboras sayboras marked this pull request as ready for review August 31, 2020 12:49
@sayboras sayboras requested review from a team as code owners August 31, 2020 12:49
@sayboras sayboras requested a review from a team August 31, 2020 12:49
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.3 Aug 31, 2020
@sayboras sayboras force-pushed the feature/azure-helm branch 2 times, most recently from 6027208 to 9ecddc5 Compare August 31, 2020 13:03
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending one question.

@@ -66,7 +66,7 @@ Deploy Cilium release via Helm:
--set global.cni.chainingMode=generic-veth \\
--set global.cni.customConf=true \\
--set global.nodeinit.enabled=true \\
--set global.azure.enabled=true \\
--set nodeinit.azure=true \\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need to set global.azure.enabled also?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting global.azure.enabled=true will change operator image to cilium-operator-azure as per https://github.com/cilium/cilium/blob/master/install/kubernetes/cilium/charts/operator/templates/deployment.yaml#L60. And cilium-operator-azure only allows ipam.azure mode.

Understand that global.azure.enabled might cause some confusion, keen to hear your input to make it clearer as well as backward compatible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we rename .azure in nodeinit to .expect-azure-vnet or similar to make it more clear?

And then maybe add an additional comment on the of this value the about its use-case? AFAIU, this is when we want to use azure but without using cilium-operator-azure that only allows ipam.azure.

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kkourt 's suggestion seems reasonable. Other than that, makes sense, sounds like this should resolve the error in Azure chaining guide.

Setting `global.azure.enabled` will change operator image to
`operator-azure`, which only accepts azure IPAM, and throw
fatal error for `cluster-pool`.

This PR is to enable node init to wait for azure vnet for both
cases (e.g. CNI and Azure IPAM)

Closes cilium#13023

Signed-off-by: Tam Mach <sayboras@yahoo.com>
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@aanm aanm merged commit 011546b into cilium:master Sep 4, 2020
@aanm aanm added this to Needs backport from master in 1.8.4 Sep 4, 2020
@aanm aanm removed this from Needs backport from master in 1.8.3 Sep 4, 2020
@sayboras sayboras deleted the feature/azure-helm branch September 4, 2020 11:26
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport pending to v1.8 in 1.8.3 Sep 7, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.3 Sep 9, 2020
@joestringer joestringer moved this from Needs backport from master to Backport done to v1.8 in 1.8.4 Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/azure Impacts Azure based IPAM. area/helm Impacts helm charts and user deployment experience release-note/bug This PR fixes an issue in a previous release of Cilium. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.3
Backport done to v1.8
1.8.4
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

Fatal error while following GSG for azure
6 participants