-
Notifications
You must be signed in to change notification settings - Fork 558
Conversation
LGTM from a Calico perspective for the changes to parts/k8s/addons/kubernetesmasteraddons-calico-daemonset.yaml |
@dtzar is this PR ready for further testing/merging when the merge conflict is fixed? |
@jackfrancis - Resolved the merge conflict. This is ready for further testing/merging. Would be great to let Circle CI do all the existing e2e tests as they are now. Sorry, I just haven't had a chance to work on creating a new e2e test for Calico. |
We'll do the standard drill, and then validate that As you mention, we don't have calico-specific E2E validations yet, do you want to wait for those to merge? What's your manual testing telling you at this point? |
I've only manually tested a K8s 1.9 cluster a couple times by going through the advanced network policy guide and everything worked great there in regards to Calico. There should be zero effective change on 1.7 (since this code won't hit there). It would be good to manually test Calico on 1.8 and possibly 1.10 if we want to be safe (it should work!). It would be ideal to have the e2e tests, but I don't want nor do I think we need to wait for those to merge since this improves things significantly from where we are at today. |
@@ -10,19 +10,13 @@ | |||
"masterProfile": { | |||
"count": 1, | |||
"dnsPrefix": "", | |||
"vmSize": "Standard_D2_v2" | |||
"vmSize": "Standard_DS2_v2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for the change to the VM SKU in the example api model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this uses the faster premium storage based VMs and is the default on many other examples.
@dtzar How are you getting past the lack of CNI implementation in your testing?
I'm getting this in kubelet logs:
tldr; current calico policy sets kubelet runtime config to |
# Deprecated in 1.10, Removed in 1.11. kubernetes.io/docs/tasks/administer-cluster/guaranteed-scheduling-critical-addon-pods | ||
scheduler.alpha.kubernetes.io/critical-pod: '' | ||
spec: | ||
annotations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I gave myself a problem here which I didn't catch because I wasn't using the updated acs-engine binary with the change. Fixing now.
After my latest fix, I validated manually that calico worked properly with the advanced policy demo on a 1.8 cluster. Something went wrong on 1.10 - looking into this now. |
Wasn't able to fix the 1.10 issue, but did check again that 1.9 works also. |
Codecov Report
@@ Coverage Diff @@
## master #2521 +/- ##
=======================================
Coverage 47.03% 47.03%
=======================================
Files 86 86
Lines 12715 12715
=======================================
Hits 5980 5980
Misses 6182 6182
Partials 553 553 Continue to review full report at Codecov.
|
I updated to the latest 3.1 Calico, rebased from master with a new binary, and tested it manually against a 1.8, 1.9, and 1.10 cluster and all work great. I also started working on an e2e test, but would like to merge that in a separate PR as I expect it will take me more time to complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
What this PR does / why we need it:
Updates Calico from 2.6 to the latest 3.0 which enables additional functionality.
Which issue this PR fixes
This lays the foundation for the requested IPAM support via these issues
#2227
#1930
Potentially fixes #1621
fixes #2202
If applicable: