feat: Windows containerd VHD support #4137
Conversation
|
I still need to address logic in cmd/upgrade.go |
Codecov Report
@@ Coverage Diff @@
## master #4137 +/- ##
==========================================
- Coverage 72.07% 72.05% -0.02%
==========================================
Files 141 141
Lines 21640 21642 +2
==========================================
- Hits 15596 15595 -1
- Misses 5093 5096 +3
Partials 951 951
Continue to review full report at Codecov.
|
marosset
force-pushed
the
win-containerd-image-config
branch
from
31a2a26
to
9c74303
Compare
|
@jsturtevant this should be ready for review |
marosset
changed the title
vhd/release-notes/aks-windows-2019-containerd/2019-datacenter-core-ctrd-17763.1637.201210.txt
Show resolved
Hide resolved
| } | ||
| if windowsProfile.WindowsOffer == "" { | ||
| windowsProfile.WindowsOffer = AKSWindowsServer2019OSImageConfig.ImageOffer | ||
| windowsProfile.WindowsOffer = defaultImageConfig.ImageOffer |
There was a problem hiding this comment.
this seems like it could get into a bad state if the profile for the windows publisher doesn't not equal defaultimageconfig. Would it be simpler to calculate the default config based on profile (which seems to be happening partially above) and then update accordingly?
There was a problem hiding this comment.
There are many different offers available to users that we do not curate in aks-engine (1909, 2004, 20H2 images, etc).
I don't think it is possible for us to maintain default values for all of these offers.
I agree users can get into a bad state if they only specify windowsPublisher but am not sure what else to do here.
If things get messed up ARM does have a detailed message saying the publisher/offer/sku/image cannot be found.
There was a problem hiding this comment.
Maybe we can nest these and only set offer if windowsPublisher == defaultImageConfig.ImagePublisher?
There was a problem hiding this comment.
Makes sense on all the versions. If we assume they can get into a bad state could this logic be simplified and spit out a warning or maybe even an error that that tells they are going to get into a bad state with the values provided if they don't use one of our known defaults?
There was a problem hiding this comment.
Maybe we can nest these and only set offer if windowsPublisher == defaultImageConfig.ImagePublisher?
for some reason this last comment didn't show up in my ui but I think we were thinking in the same direction
There was a problem hiding this comment.
Good idea. I'll check if we think things are not good, use the default image, a log a warming.
There was a problem hiding this comment.
Let's remove the containerd-specific WindowsProfile default stuff here that we do to support E2E tests now that these defaults have matriculated to the actual api model defaults flow:
https://github.com/Azure/aks-engine/blob/master/test/e2e/engine/template.go#L253
marosset
force-pushed
the
win-containerd-image-config
branch
from
32bc858
to
5bbdbfd
Compare
|
@marosset I wonder that is plan actually also drop Docker from those containerd images as old comment on code proposes it already? aks-engine/vhd/packer/configure-windows-vhd.ps1 Lines 335 to 339 in bd397bc |
…alues for windows
marosset
force-pushed
the
win-containerd-image-config
branch
from
5bbdbfd
to
a223f75
Compare
|
/lgtm @jackfrancis did your requested changes get addressed? |
|
@jsturtevant yep! |
|
[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:
Issue Fixed:
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: