chore: Update calico to 3.5 and allow calico to work with azure CNI #454
Conversation
Codecov Report
@@ Coverage Diff @@
## master #454 +/- ##
======================================
Coverage 73.3% 73.3%
======================================
Files 130 130
Lines 17708 17708
======================================
Hits 12981 12981
Misses 4026 4026
Partials 701 701 |
song-jiang
force-pushed
the
song-calico-3.5
branch
from
400dbdc
to
7fed98e
Compare
|
@jackfrancis should we hold this until we cut the release? /hold |
|
Yes. We want to run this through the appropriate back-channel cluster configuration tests appropriate for this surface area. |
|
I mean to say that this doesn't need to go in ASAP with the next aks-engine release, so we're on the same page. :) |
|
@song-jiang Can you add more description about the differences between |
| mv $CNI_BIN_DIR/10-azure.conflist $CNI_CONFIG_DIR/ | ||
| chmod 600 $CNI_CONFIG_DIR/10-azure.conflist | ||
| if [[ "${NETWORK_POLICY}" == "calico" ]] || [[ "${NETWORK_POLICY}" == "" ]]; then | ||
| sed -i 's#"mode":"bridge"#"mode":"transparent"#g' $CNI_CONFIG_DIR/10-azure.conflist |
There was a problem hiding this comment.
Do we really want to change the Azure CNI configuration here? This bridge mode setting will now apply to non-calico scenarios given the || [[ "${NETWORK_POLICY}" == "" ]] condition
There was a problem hiding this comment.
The purpose for this change is to make life easier for users who want to apply calico for network policy on clusters previously installed without specifying a network policy provider.
My understanding is it should not affect any user experience if they choose a non-calico network policy provider.
I am not sure if user really care about using a bridge mode network or a transparent mode network. Would love to hear your input on this. Thanks.
There was a problem hiding this comment.
@sharmasushant @tamilmani1989 can you comment on this change?
There was a problem hiding this comment.
By default we should have bridge mode and not transparent mode. To my understanding, the config should be set to transparent mode only if user speicifes calico policy. @sharmasushant can comment on this.
There was a problem hiding this comment.
@tamilmani1989 I'm running this through some final smoke tests. Please comment if we still have open concerns about this change before merging.
|
@sharmasushant @tamilmani1989 Could you kindly read through this PR description and comment on @song-jiang's description of the difference between Based on the reading through the changeset, the Any thoughts? |
|
@jackfrancis The supported default mode for azure cni is |
|
@jackfrancis @sharmasushant @tamilmani1989 Thanks for your input and clarifications. Apologies for my misunderstanding. I will make changes to my PR accordingly. |
|
@jackfrancis Could you take another look? |
There was a problem hiding this comment.
@song-jiang as per email conversation with tigera, we agreed upon azure cni +azure ipam + calico network policy. NetworkPlugin: "calico+azureipam" - this is something new (calico cni +azure ipam) that we didn't talk about over the email thread.
parts/k8s/kubernetesinstalls.sh
Outdated
| @@ -169,7 +169,7 @@ installClearContainersRuntime() { | |||
| } | |||
|
|
|||
| installNetworkPlugin() { | |||
| if [[ "${NETWORK_PLUGIN}" = "azure" ]]; then | |||
| if [[ "${NETWORK_PLUGIN}" = "azure" ]] || [[ "${NETWORK_PLUGIN}" = "calico+azureipam" ]]; then | |||
There was a problem hiding this comment.
what is calico+azureipam? I may be missing something here. Can you please explain us which scenario this is used?
Same questions here. |
|
@tamilmani1989 "NetworkPlugin: calico+azureipam, NetworkPolicy: calico" is the replacement of current implementation of "NetworkPlugin: azure, NetworkPolicy: calico" which uses calico CNI plugin and azure IPAM. We leave that as an option for anyone who might be using it and continue wish to do so. I will check with PM on email conversations made earlier. |
|
@song-jiang I have a preference for simply moving the implementation forward. If we have good reason to change the implementation, we should stand by that opinion, rather than offering two implementations that basically do the same thing, which I would argue is confusing for users. |
|
@jackfrancis Agree it is less confusing to have just one option. Let me check with PM and come back to you. |
|
@song-jiang I think we have agreed on this with PM - can you confirm and give @jackfrancis an idea of when you're likely to get this update done? |
song-jiang
force-pushed
the
song-calico-3.5
branch
from
addab61
to
0ad9de9
Compare
song-jiang
changed the title
|
@ahrkrak Yes. It has been confirmed with PM we will drop option |
song-jiang
force-pushed
the
song-calico-3.5
branch
from
85fb75a
to
8c910d9
Compare
song-jiang
changed the title
|
@song-jiang please rebase on latest master to get up to date unit test coverage stats (the last rebase seems to be from 8 days ago). |
pkg/api/vlabs/validate_test.go
Outdated
| @@ -704,6 +704,10 @@ func Test_Properties_ValidateNetworkPluginPlusPolicy(t *testing.T) { | |||
| networkPlugin: "azure", | |||
| networkPolicy: "flannel", | |||
| }, | |||
| { | |||
| networkPlugin: "azure", | |||
There was a problem hiding this comment.
this combination is already in networkPluginPlusPolicyAllowed in validate.go while this is adding it to the unit test expecting it to not be valid. Unless I'm missing something, it seems contradictory.
There was a problem hiding this comment.
I see. My mistake. Thanks!
|
@tamilmani1989 It is unlikely pods on same host have mixed veth prefixes. During upgrade process, either calico CNI (old node) or azure CNI (new node) will be used. However, what I found is we could have a scenario where new node gets old version of So current status on upgrading from existing (AKS or bare-metal) cluster with my PR
I am working on possible solutions to fix this. cc @jackfrancis @sauryadas |
song-jiang
force-pushed
the
song-calico-3.5
branch
from
f53cee5
to
ba8660c
Compare
|
Don't want to delay this PR! Would be nice to see Calico at 3.5.3 or 3.6.0 to have the latest fixes/functionality, ensure all works, and avoid another PR to update the daemonset yamls. |
Is this fixed now? |
|
@feiskyer Not yet. It is still in progress. |
song-jiang
force-pushed
the
song-calico-3.5
branch
from
ba8660c
to
f0ada3c
Compare
|
We have agreed an upgrade strategy that will work, but needs to be documented. @jackfrancis is picking up ownership of the docs changes. |
jackfrancis
force-pushed
the
song-calico-3.5
branch
from
f0ada3c
to
c1f8add
Compare
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
smoke tests checked out lgtm @tamilmani1989 for sign off on changes as well |
|
@song-jiang @ahrkrak
can you please confirm? |
|
Correct. Currently #908 just includes a reference to the yaml that you have to kubectl delete. That removes any older calico-nodes, which will then get updated by the addon manager to the newer version. @jackfrancis will update the actual docs to include the howto instructions for doing that. |
|
/lgtm |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, song-jiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Reason for Change:
Upgrade calico to v3.5
Allow calico to work with azure CNI by setting
NetworkPolicy: "calico" , NetworkPlugin: "azure"In this case, calico per-host daemon works with azure CNI plugin and azure IPAM to support network policy.
Continue support kubenet network plugin by setting
NetworkPolicy: "calico" , NetworkPlugin: "kubenet"Issue Fixed:
Requirements:
Notes: