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

kata containers enhancement proposal #366

Closed
wants to merge 10 commits into from

Conversation

ariel-adam
Copy link
Contributor

Providing the enhancement document for the kata containers project to be reviewed.

@ariel-adam
Copy link
Contributor Author

/assign @joelanford

@ariel-adam
Copy link
Contributor Author

/assign @mrunalp

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 22, 2020
@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci-robot openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 21, 2020
@openshift-bot
Copy link

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci-robot
Copy link

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cgwalters
Copy link
Member

/reopen

@openshift-ci-robot
Copy link

@cgwalters: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jensfr
Copy link

jensfr commented Feb 9, 2021

@mrunalp we updated the document and think it is ready to be merged if you agree.
The changes to the previous version are:

  1. it's qemu-kiwi not the full qemu-kvm that is being installed as an extension now
  2. we intend to bring the kata binaries to the workers by pulling and mounting a container image (as opposed to creating a running container and mount that)

@jimmyjones2
Copy link

I'm interested in using this to isolate less trustworthy workloads.

  • Will Firecracker be an option?
  • Will NetworkPolicy be applied on the host, outside of the VM (ie. so provides a higher level of assurance)

### Overview
Kubernetes provides support for RuntimeClasses. RuntimeClass is a feature for selecting the container runtime configuration. The container runtime configuration is used to run a Pod’s containers.

CRI-O today comes out of the box with a runc as the default runtime. CRI-O also supports RUntimeClasses and using this configuration, it will support a KataContainers runtime as well.
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/RUntimeClasses/RuntimeClasses/

Copy link

Choose a reason for hiding this comment

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

fixed, thank you!

1. ~200 mb extra, installed in the host, for those who'd be using kata runtime
2. Updates / Removal may be more complicated than having the RPMs as part of machine-os-content

#### Long term: Use RHCOS extensions (qemu-kiwi and dependencies only)
Copy link
Member

Choose a reason for hiding this comment

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

This section looks like a copy/paste from #317

Could this be re-written to just link to that enhancement and instead focus this section on what packages would be supplied in the extension for kata containers?

Copy link

Choose a reason for hiding this comment

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

Please update that section to indicate what we would put in the extension.

Do we want to discuss the reduced qemu package here?

Copy link

Choose a reason for hiding this comment

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

changed it, thank you!

1. ~20 mb extra, installed in the host, for those who'd be using kata runtime
2. machine-os-content would still carry this until there is a separate machine-os-content- extensions container

This approach has a dependency on the extensions framework being delivered into Openshift.
Copy link
Member

Choose a reason for hiding this comment

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

The RHCOS extensions framework was successfully delivered in 4.6; this statement could be dropped.

Copy link

Choose a reason for hiding this comment

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

dropped, thank you!

Copy link

@c3d c3d left a comment

Choose a reason for hiding this comment

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

Overall, I approve the description, but it needs a few updates based on work done since the PR was open.


## Summary

Kata containers is an open source project developing a container runtime using virtual machines and providing the same look and feel as with vanilla containers.
Copy link

Choose a reason for hiding this comment

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

same look and fill as vanilla containers (no with)

Copy link

Choose a reason for hiding this comment

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

fixed, thank you!

Copy link

Choose a reason for hiding this comment

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

I hope you did not carry over my typo (fill instead of feel)

## Summary

Kata containers is an open source project developing a container runtime using virtual machines and providing the same look and feel as with vanilla containers.
By leveraging hardware virtualization technologies kata containers provides powerful workload isolation compared to existing container solutions.
Copy link

Choose a reason for hiding this comment

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

add , after technologies.

Copy link

Choose a reason for hiding this comment

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

done


## Summary

Kata containers is an open source project developing a container runtime using virtual machines and providing the same look and feel as with vanilla containers.
Copy link

Choose a reason for hiding this comment

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

Try to use consistent capitalization, Kata Containers everywhere (making it clear it's a project name and not some terminology).

Copy link

Choose a reason for hiding this comment

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

done


## Summary

Kata containers is an open source project developing a container runtime using virtual machines and providing the same look and feel as with vanilla containers.
Copy link

Choose a reason for hiding this comment

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

Add link to Kata Containers project for easier review.

Copy link

Choose a reason for hiding this comment

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

done

1. ~200 mb extra, installed in the host, for those who'd be using kata runtime
2. Updates / Removal may be more complicated than having the RPMs as part of machine-os-content

#### Long term: Use RHCOS extensions (qemu-kiwi and dependencies only)
Copy link

Choose a reason for hiding this comment

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

Please update that section to indicate what we would put in the extension.

Do we want to discuss the reduced qemu package here?

- Configure CRI-O to use Kata Runtime on those worker nodes
- Installation of the runtimeClass on the cluster
- Updates the Kata runtime
- Uninstall Kata Runtime and reconfigure CRI-O to not use it.
Copy link

Choose a reason for hiding this comment

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

Add a reference to base scheduling metrics?

Copy link

Choose a reason for hiding this comment

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

@c3d Not sure what you'd like to see. Do you have one for me?

Copy link

Choose a reason for hiding this comment

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

I would add something like
- Provide usable CPU, memory, disk and network metrics to the scheduler

WDYT?

Choose a reason for hiding this comment

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

@c3d, this is not necessarily the responsibility of Kata Containers, nor necessarily CRI-O or kubelet. I'd argue against adding it, unless we have the full buy-in of the monitoring team to support those.

IMHO, adding this may actually increase the chance to have the addition blocked than anything else.

/cc @zanetworker

Copy link

Choose a reason for hiding this comment

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

I agree with you on this one @fidencio

Copy link

Choose a reason for hiding this comment

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

I agree with you both on the responsibilities. That is not my point however, nor is it the point of this section. The section describes the "Goals" of the operator, from the OpenShift perspective. I still believe that these goals should include adding whatever other configuration is needed for the orchestration layer to do its job. That includes metrics, pod overhead computations, and possibly other fine details that I'm sure we have not really covered yet.

What about extending the "Installation of the runtimeClass" bullet as follows:

- Installation of the runtimeClass on the cluster, as well as of the required components for the runtime to be controlled by the orchestration layer.

Come to think of it, this rewording is also necessary to describe the installation of the virtualization components.

Copy link

Choose a reason for hiding this comment

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

Made the change as suggested by @c3d

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: c3d
To complete the pull request process, please assign joelanford after the PR has been reviewed.
You can assign the PR to them by writing /assign @joelanford in a comment when ready.

The full list of commands accepted by this bot can be found 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

@jensfr
Copy link

jensfr commented Feb 16, 2021

I've addressed all review comments by @miabbott and @c3d

@jensfr
Copy link

jensfr commented Feb 22, 2021

All review comments are addressed. Gentle reminder for more reviews/acks :-)

@jensfr
Copy link

jensfr commented Feb 23, 2021

@mrunalp ping

ariel-adam and others added 10 commits March 4, 2021 17:56
Signed-off-by: Ariel Adam <aadam@redhat.com>
fix review findings from Micah Abbot
Fix c3d's review findings
Extend the goal "installation of the runtime" by also taking care of things like pod overhead etc
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
This commit silences "MD009/no-trailing-spaces Trailing space" markdown
lint error.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
This commit silences "MD024/no-duplicate-heading/no-duplicate-header
Multiple headings with the same content" markdown lint error.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
@fidencio
Copy link

fidencio commented Mar 6, 2021

@ariel-adam, can you close this one in favour of #677 ?

@ariel-adam
Copy link
Contributor Author

Closing this since we have #677 instead

@ariel-adam ariel-adam closed this Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.