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

Document krunkit for AI Workloads #2200

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Oct 11, 2024

No description provided.

@rhatdan
Copy link
Member Author

rhatdan commented Oct 11, 2024

@baude @TomSweeneyRedHat @Luap99 @mheon PTAL

| Platform | Default | Optional |
| -------- | ------------------------| -------- |
| Linux | QEMU | None |
| Windows | WSL | None |
Copy link
Member

Choose a reason for hiding this comment

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

HyperV?

@@ -886,7 +886,12 @@ default_sysctls = [

# Virtualization provider used to run Podman machine.
# If it is empty or commented out, the default provider will be used.
#
# Linux: QEMU (Default)
# Windows QEMU (Default)
Copy link
Member

Choose a reason for hiding this comment

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

That's not correct

@rhatdan rhatdan force-pushed the krunkit branch 3 times, most recently from 59e2637 to 6279837 Compare October 11, 2024 20:50
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

#
# Linux: QEMU (Default)
# Windows: there are currently two options:
# WSL2 - Windows Subsystem for Linux (Default)
Copy link
Member

Choose a reason for hiding this comment

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

this is not correct we require the use of wsl not WSL2.

# Mac: there are currently two options:
# vfkit - Default Apple Hypervisor (Default)
# krunkit - Launch virtual machines using the libkrun platform, optimized
# for sharing GPU with the machine.
Copy link
Member

Choose a reason for hiding this comment

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

there should be an empty comment line here to be consistent with the other descriptions above

Comment on lines 976 to 980
| Platform | Default | Optional |
| -------- | ---------------------------------| -------- |
| Linux | QEMU | None |
| Windows | WSL: Windows Subsystem for Linux | HyperV: Windows Server Virtualization |
| Mac | vfkit: Apple Hypervisor | krunkit: Launch machine via libkrun platform, optimized for sharing GPU with the machine |
Copy link
Member

Choose a reason for hiding this comment

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

I find this table confusing, as an user it is not obvious what value must be use for provider IMO. This either needs to highlight what key to use in the table or I think a list like done in containers.conf file might be better suited, especially if we ever were to add a third provider for whatever reason...

Also it may be helpful to point out that the case of the name does not matter because otherwise a user might be wondering if the case is important given all keys are written with a different case here

Comment on lines 976 to 980
| Platform | Default | Optional |
| -------- | ---------------------------------| -------- |
| Linux | QEMU | None |
| Windows | WSL: Windows Subsystem for Linux | HyperV: Windows Server Virtualization |
| Mac | vfkit: Apple Hypervisor | krunkit: Launch machine via libkrun platform, optimized for sharing GPU with the machine |
Copy link
Member

Choose a reason for hiding this comment

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

it is not called vfkit, it must use applehv
same for krunkit, you must use libkrun as provider name

Comment on lines 894 to 895
# vfkit - Default Apple Hypervisor (Default)
# krunkit - Launch virtual machines using the libkrun platform, optimized
Copy link
Member

Choose a reason for hiding this comment

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

same here, wrong provider names

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

openshift-ci bot commented Oct 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, rhatdan, vrothberg

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:
  • OWNERS [Luap99,rhatdan,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit aa3ed52 into containers:main Oct 15, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants