Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

chore: Hyperv and upstream Containerd package support #3688

Merged
merged 22 commits into from
Aug 13, 2020

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Aug 10, 2020

Reason for Change:

Containerd 1.4rc1 is now available. Hyperv support has progressed but is still in experimental support.

This pr enables:

Better support for configuring Containerd will be done in a separate PR after a discussion in issue #3687

Issue Fixed:

Fixes #2827

Requirements:

Notes:

@jsturtevant
Copy link
Contributor Author

/cc @AbelHu @marosset @ksubrmnn

@codecov
Copy link

codecov bot commented Aug 11, 2020

Codecov Report

Merging #3688 into master will increase coverage by 0.00%.
The diff coverage is 84.54%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #3688    +/-   ##
========================================
  Coverage   73.14%   73.15%            
========================================
  Files         147      147            
  Lines       25165    25301   +136     
========================================
+ Hits        18408    18508   +100     
- Misses       5623     5655    +32     
- Partials     1134     1138     +4     
Impacted Files Coverage Δ
pkg/api/common/const.go 40.00% <ø> (ø)
pkg/api/vlabs/types.go 71.83% <ø> (ø)
pkg/engine/engine.go 85.64% <0.00%> (-0.31%) ⬇️
pkg/engine/templates_generated.go 53.42% <38.88%> (-0.20%) ⬇️
pkg/api/vlabs/validate.go 80.00% <92.50%> (+0.50%) ⬆️
pkg/api/addons.go 97.79% <100.00%> (+0.02%) ⬆️
pkg/api/converterfromapi.go 94.17% <100.00%> (+0.07%) ⬆️
pkg/api/convertertoapi.go 93.68% <100.00%> (+0.07%) ⬆️
pkg/api/k8s_versions.go 100.00% <100.00%> (ø)
pkg/api/types.go 94.31% <100.00%> (+0.06%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f321785...3ca765c. Read the comment docs.

docs/topics/features.md Outdated Show resolved Hide resolved
docs/topics/features.md Outdated Show resolved Hide resolved
docs/topics/features.md Outdated Show resolved Hide resolved
docs/topics/features.md Outdated Show resolved Hide resolved
docs/topics/features.md Outdated Show resolved Hide resolved
docs/topics/features.md Outdated Show resolved Hide resolved
docs/topics/features.md Outdated Show resolved Hide resolved
@@ -31,41 +31,20 @@ cp bin/ctr.exe /output
#cp bin/containerd.exe /output # missing CRI plugin, so build from containerd/cri
cd \$GOPATH
cd src/github.com/containerd
git clone https://github.com/containerd/cri.git
git clone https://github.com/jterry75/cri.git
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using someone's fork of ContainerD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This the fork the Container Platform team maintains for making the updates required for hyperv. They are in active conversation with the Containerd team to integrate these changes into the upstream. This was the same process they had for the initial Containerd Support that is now in the 1.4 release.

Update docs/topics/features.md

Co-authored-by: Matt Boersma <Matt.Boersma@microsoft.com>
@marosset
Copy link
Contributor

I think we probably want to add a new property in the api model that sets if the default runtime class in the containerd config uses hyper-v isolation or not.
I can't think of a good way to run upstream test passes for both process isolation and hyper-v isolation containers without this.
Thoughts?

@jsturtevant
Copy link
Contributor Author

jsturtevant commented Aug 11, 2020

Agreed I created issue #3687 with a possible idea. As of right now this will allow running both based on the configuration file that is present in the hyperv package. This is not ideal but wanted to get thoughts on #3687 and get something to unlock the failing tests before modifying the api model to far.

Thinking on it a bit more the model change might be something like:

"windowsProfile": {
        "windowsSku": "Datacenter-Core-2004-with-Containers-smalldisk",
        "imageVersion": "latest",
        "defaultHandler": process/hyperv
          "hypervisorHandlers":[
            "17763",
            "18362",
            "18363",
            "19041"
          ]

This would allow user to specify:

  • process isolated default and no hyperv handlers,
  • process isolated default with configurable hypervisor handlers
  • hyperv default, process isolated, and configurable hypervisor handlers.

These fields would only be valid if using containerd.

@marosset
Copy link
Contributor

I think we need to have at least have a switch for the default runtime handler / isolation type for this to get checked in to master.
It looks like in the code the in this PR the switch to use hyper-v vs process isolation is the existence of a config file which will currently always exist.

We can use #3687 to figure out how to handle more customizing the non-default runtime classes defined in the containerd config.

}
"networkPlugin": "azure",
"containerRuntime": "containerd",
"windowsContainerdURL": "https://k8swin.blob.core.windows.net/k8s-windows/containerd/containerplat-aks-test-0.0.8.zip"
Copy link
Member

Choose a reason for hiding this comment

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

May I regard this package url as the _official url_for 20H1 so far?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this is the latest package

Copy link
Contributor

Choose a reason for hiding this comment

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

These are the official bits but we will probably need to host them along with our other artifacts for production use.
k8swin.blob.core.windows.net is just hosted in a single zone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to set up a different url for prod use. Will follow back once I have it

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.
That should not block this PR IMO

AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Aug 13, 2020
@marosset
Copy link
Contributor

/lgtm

@acs-bot
Copy link

acs-bot commented Aug 13, 2020

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jsturtevant jsturtevant merged commit 046db34 into Azure:master Aug 13, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Aug 17, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Aug 20, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Aug 20, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Aug 20, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Aug 20, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Aug 20, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Aug 20, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Aug 20, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Sep 14, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Sep 27, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Sep 30, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Sep 30, 2020
AbelHu added a commit to AbelHu/aks-engine that referenced this pull request Oct 20, 2020
AbelHu added a commit that referenced this pull request Oct 20, 2020
* chore: Hyperv and upstream Containerd package support (#3688)

* fix: fix issue where installing a different version of containerd vs what was pre-installed on VHD failed silently (#3743)

* feat: collect hyperv logs

* fix: do not log secret in provisioning script

Co-authored-by: Mark Rossetti <marosset@microsoft.com>
penggu pushed a commit to penggu/aks-engine that referenced this pull request Oct 28, 2020
* hyperv and upstream support

* update docs and scripts for hyperv

Co-authored-by: Matt Boersma <Matt.Boersma@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move Windows nodes to public ContainerD builds
6 participants