-
Notifications
You must be signed in to change notification settings - Fork 522
Conversation
@jackfrancis fyi on azurecni version update |
Codecov Report
@@ Coverage Diff @@
## master #4319 +/- ##
=======================================
Coverage 72.05% 72.06%
=======================================
Files 141 141
Lines 21632 21634 +2
=======================================
+ Hits 15588 15590 +2
Misses 5093 5093
Partials 951 951
Continue to review full report at Codecov.
|
tests failed because I forgot to generate the signed package. That is now in process |
vhd/packer/configure-windows-vhd.ps1
Outdated
"https://kubernetesartifacts.azureedge.net/azure-cni/v1.2.0_hotfix/binaries/azure-vnet-cni-singletenancy-windows-amd64-v1.2.0_hotfix.zip", | ||
"https://kubernetesartifacts.azureedge.net/azure-cni/v1.2.2/binaries/azure-vnet-cni-singletenancy-windows-amd64-v1.2.2.zip" | ||
"https://kubernetesartifacts.azureedge.net/azure-cni/v1.2.2/binaries/azure-vnet-cni-singletenancy-windows-amd64-v1.2.2.zip", | ||
"https://kubernetesartifacts.azureedge.net/azure-cni/v1.2.2/binaries/azure-vnet-cni-singletenancy-windows-amd64-v1.2.7.zip" |
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.
azure-cni/v1.2.2/binaries
=>
azure-cni/v1.2.7/binaries
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.
I'm really surprised the VHD builds for windows didn't fail because of this.
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.
do you mean the E2E tests should have failed because of what 1.2.7 fixes?
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.
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.
I would have expected this to fail during the VHD build
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.
Hmm, looks like this might be a regression that got introduced when we switched to using curl.exe.
curl -L https://kubernetesartifacts.azureedge.net/azure-cni/v1.2.2/binaries/azure-vnet-cni-singletenancy-windows-amd64-v1.2.7.zip
<?xml version="1.0" encoding="utf-8"?><Error><Code>BlobNotFound</Code><Message>The specified blob does not exist.
RequestId:786d8a4f-601e-00df-2d04-168166000000
Time:2021-03-10T23:26:09.4129514Z</Message></Error>
C:\Users\marosset>echo %errorlevel%
0
curl.exe is returning success here...
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.
AgentBaker uses https://github.com/Azure/AgentBaker/blob/master/vhdbuilder/packer/test/windows-vhd-content-test.ps1 to check the cached files
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 curl doesn't exit non zero on 404s and others: https://superuser.com/a/657174
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.
Yea, it looks like if you pass -f
it'll exit 22 for 404s.
Let me update this and other usage to check $LASTEXITCODE and error.
Co-authored-by: Mark Rossetti <marosset@microsoft.com>
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsturtevant, marosset 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:
fixes issue with azure cni when passing acl policies with containerd: https://github.com/Azure/azure-container-networking/releases/tag/v1.2.7
Issue Fixed:
#3833
Credit Where Due:
Does this change contain code from or inspired by another project?
If "Yes," did you notify that project's maintainers and provide attribution?
Requirements:
Notes: