Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Starting section on CRI multiarch/version support #1254

Closed
wants to merge 4 commits into from

Conversation

PatrickLang
Copy link
Contributor

@PatrickLang PatrickLang commented Sep 23, 2019

One of the details we need to work through is how to disambiguate the OS, architecture and version used when pulling an image and creating a sandbox. Today this is done with a label sandbox-platform during pull, and with annotations when creating a sandbox+container. Instead of having this in weakly typed labels/annotations, we should make it a strongly typed field in CRI.

@feiskyer and @Random-Liu - can you help by reviewing this?

@jterry75 should be able to help clarify some of the Windows specifics as well from the containerd side

/hold
/sig windows
/sig node

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/windows Categorizes an issue or PR as relevant to SIG Windows. sig/node Categorizes an issue or PR as relevant to SIG Node. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: PatrickLang

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2019
}
```

Today, PodSandboxConfig has an annotation that can be used as a workaround, but it's stated that "Whenever possible, however, runtime authors SHOULD consider proposing new typed fields for any new features instead.".
Copy link
Member

@feiskyer feiskyer Sep 24, 2019

Choose a reason for hiding this comment

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

could you add a part describing how could those new configs be used from Pod spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

The proposed additions are:

```
string os = 9;

Choose a reason for hiding this comment

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

@PatrickLang - If we do this we likely don't need to have separate RuntimeHandlers for runhcs-wcow-hypervisor and runhcs-wcow-hypervisor-1903 above. This makes it explicit at activation time rather an implicit from the RuntimeHandler (which I like better anyways)

@PatrickLang
Copy link
Contributor Author

cc @yliaog

I'm working on more updates to this PR with examples and tradeoffs of different ways to disambiguate os/arch/version

Adding 3 options for proceeding with RuntimeClass
@PatrickLang
Copy link
Contributor Author

/assign @tallclair

Can you help review from RuntimeClass standpoint?

Copy link
Member

@tallclair tallclair left a comment

Choose a reason for hiding this comment

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

I don't have any objects to the RuntimeClass approaches proposed here. IIUC, neither require any changes to RuntimeClass to support those 2 options?

Comment on lines 182 to 184



Copy link
Member

Choose a reason for hiding this comment

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

nit: empty lines

metadata:
name: mypod
spec:
runtimeClassName: WindowsServer1809
Copy link
Member

Choose a reason for hiding this comment

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

An advantage of this approach is that by making the runtimeClassName explicit, the user must update the deployment which:

  • Forces them to ensure they have an image built for the new versions
  • Makes it clear what version they're expecting, and which version a given pod is running
  • Automatically triggers a rolling update to move to the new version, with the supported workflows

This disadvantage is it places more work on the dev teams to keep their deployments up-to-date, and also has a risk of a combinatorial explosion of runtime classes.

Copy link

Choose a reason for hiding this comment

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

Interesting point. It also enables us to have different ideas around things like RuntimeOverhead and such per version.

@tallclair - Given your feedback would you recommend that we do not have a handler that is "versionless"? IE: no runhcs-wcow-process but only runhcs-wcow-process-1809 etc

kind: RuntimeClass
metadata:
name: windows-1809-amd64
handler: 'windows-hyperv-1809' # only handler accepted when using dockershim
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review feedback - this doesn't get passed to pull, so it doesn't solve the problem of selecting the right image at pull time if its multiarch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that comment is wrong - copy/paste error. handler: windows-hyperv-1809 is an example using containerd


###### Details of Multi-arch images

Here's one example of a container image that has a multi-arch manifest with entries for Linux & Windows, multiple CPU architectures, and multiple Windows os versions.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

review feedback - put a bold thing that says multi-arch is actually multi-version too

spec:
os: windows
osversion: 1809
architecture: amd64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

open issue - would also need variant. per OCI distribution, there can be multiple architectures with different variants. see arm/v7, arm/v8

digest platform
------ --------
sha256:f04192a946a4473d0001ed9c9422a9e3c9c659de8498778d29bfe6c98672ad9f @{architecture=amd64; os=linux}
sha256:b7da46cdbc9a0c0ed154cabbb0591dea596e67d62d84c2a3ef34aaefe98f186d @{architecture=arm; os=linux; variant=v7}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

find a arm/v8 sample?

handler: 'docker' # only handler accepted when using dockershim
scheduling:
nodeSelector:
kubernetes.io/os: 'windows'
Copy link
Contributor Author

@PatrickLang PatrickLang Oct 3, 2019

Choose a reason for hiding this comment

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

todo: open mini kep for os.version. that's needed regardless of this. propose setting it from kubelet

Copy link
Member

Choose a reason for hiding this comment

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

If kubelet needs to query the Windows OS version through something like syscall.GetVersion, we will also need to make sure there is a manifest generated: golang/go#17835 (comment)

[plugins.cri.containerd.runtimes.windows-hyperv-1809]
runtime_type = "io.containerd.runhcs.v1"
[plugins.cri.containerd.runtimes.runhcs-wcow-hypervisor.options]
SandboxImage = "{{WINDOWSSANDBOXIMAGE}}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

take out the template - change to a multiarch image for this example

nodeSelector:
kubernetes.io/os: 'windows'
kubernetes.io/arch: 'amd64'
beta.kubernetes.io/osversion: '1903'
Copy link
Contributor Author

@PatrickLang PatrickLang Oct 3, 2019

Choose a reason for hiding this comment

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

question: can this match a set instead of a specific value? How to handle if a given Pod could run on multiple OS/os.version/arch instead of a single one?

kubernetes.io/os: 'windows'
kubernetes.io/arch: 'amd64'
beta.kubernetes.io/osversion: '1903'
capability.microsoft.com/isolation: 'hyperv'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add comment - that's a proposed new label not an existing one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clarify - checking for the hyperv capability belongs in a label because not all cpu types / VM sizes may support Hyper-V

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 may need to be a more generic name like supportsVirtualizationInstructions

@PatrickLang
Copy link
Contributor Author

I'm closing this in favor of #1302 which has more details around the use cases and is being reviewed with the appropriate SIGs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants