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.

Sign up for GitHub

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

Jump to bottom

kata-deploy: Allow setting up snapshotters per runtime handler #8655

Merged
merged 1 commit into from Dec 22, 2023
Merged

kata-deploy: Allow setting up snapshotters per runtime handler #8655

merged 1 commit into from Dec 22, 2023

Conversation

fidencio
Copy link
Member

Since containerd 1.7.0 we can easily set a specific snapshotter to be used with a runtime handler, and we should take advantage of this, mostly as it'll help setting up any runtime using devmapper or nydus snapshotters.

This implementation here has a few caveats:

  • The format expected for the SNAPSHOTTER_HANDLER_MAPPING is: shim:snapshotter,shim:snapshotter,...
  • It only works with containerd 1.7 or newer
  • We never change the default containerd snapshotter
  • We don't do any check on our side to verify whether the snapshotter required is properly deployed
  • Users will have to add an annotation to their pods, in order to use the snapshotter set up per runtime handler
    • Example: metadata: ... annotations: io.containerd.cri.runtime-handler: kata-fc

Fixes: #8615

NOTE: Due to the lack of time from my side, as I'm vanishing for a few weeks, I've opted for not yet change the tests related to devmapper to use this new "feature". Together with this decision comes a head's up that this is not tested anywhere yet, but it'll help us to unblock @ChengyuZhu6's series.

@ChengyuZhu6
Copy link
Member

Thanks very much @fidencio !

@fidencio fidencio force-pushed the topic/kata-deploy-add-snapshotter-support branch from 911c46f to 0454b87 Compare December 13, 2023 13:28
@katacontainersbot katacontainersbot added the size/medium Average sized task label Dec 13, 2023
@fidencio fidencio added force-skip-ci Stop the CI running for a PR (remember: if you break it, you fix it!) ok-to-test labels Dec 13, 2023
@fidencio
Copy link
Member Author

/test

fidencio added a commit to fidencio/cc-operator that referenced this pull request Dec 13, 2023
@fidencio
controller: Handle snapshotter per handler
b11460f 
Let's start properly setting a specific snapshotter per runtime handler
configured for containerd.

This work depends on a work done on the Kata Containers side to better
support setting snapshotters per runtime handler.

The PR from the Kata Containers side is:
kata-containers/kata-containers#8655.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
@fidencio fidencio mentioned this pull request Dec 13, 2023
Merged
fidencio added a commit to fidencio/cc-operator that referenced this pull request Dec 13, 2023
@fidencio
controller: Handle snapshotter per handler
5f42870 
Let's start properly setting a specific snapshotter per runtime handler
configured for containerd.

This work depends on a work done on the Kata Containers side to better
support setting snapshotters per runtime handler.

The PR from the Kata Containers side is:
kata-containers/kata-containers#8655.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
@fidencio fidencio force-pushed the topic/kata-deploy-add-snapshotter-support branch 6 times, most recently from 698132c to 91a6437 Compare December 13, 2023 20:51
@fidencio
Copy link
Member Author

/test

stevenhorsman
stevenhorsman reviewed Dec 14, 2023
View reviewed changes
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

I've tested lots of individual bits of this PR and the processing logic seems correct, apart from the case that I'll mentioned

tools/packaging/kata-deploy/scripts/kata-deploy.sh Outdated Show resolved Hide resolved
@stevenhorsman stevenhorsman added the do-not-merge PR has problems or depends on another label Dec 14, 2023
@stevenhorsman
Copy link
Member

Add do-not-merge on this whilst we investigate issues with tomlq in the previous PR as mentioned in confidential-containers/operator#305

beraldoleal
beraldoleal approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@beraldoleal beraldoleal left a comment •
edited

Choose a reason for hiding this comment

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

Except of @stevenhorsman comment, LGTM. Thanks @fidencio .

@fidencio fidencio force-pushed the topic/kata-deploy-add-snapshotter-support branch from 91a6437 to e95aec1 Compare December 20, 2023 09:24
@fidencio
Copy link
Member Author

Add do-not-merge on this whilst we investigate issues with tomlq in the previous PR as mentioned in confidential-containers/operator#305

I'm removing the do-not-merge as the issue was solved.

@fidencio fidencio removed the do-not-merge PR has problems or depends on another label Dec 21, 2023
@fidencio
kata-deploy: Allow setting up snapshotters per runtime handler
6cc6ca5 
Since containerd 1.7.0 we can easily set a specific snapshotter to be
used with a runtime handler, and we should take advantage of this,
mostly as it'll help setting up any runtime using devmapper or nydus
snapshotters.

This implementation here has a few caveats:
* The format expected for the SNAPSHOTTER_HANDLER_MAPPING is:
  `shim:snapshotter,shim:snapshotter,...`
* It only works with containerd 1.7 or newer
* We **never** change the default containerd snapshotter
* We don't do any check on our side to verify whether the snapshotter
  required is properly deployed
* Users will have to add an annotation to their pods, in order to use
  the snapshotter set up per runtime handler
  * Example:
    ```
    metadata:
      ...
      annotations:
        io.containerd.cri.runtime-handler: kata-fc
    ```

Fixes: kata-containers#8615

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
@fidencio fidencio force-pushed the topic/kata-deploy-add-snapshotter-support branch from e95aec1 to 6cc6ca5 Compare December 21, 2023 10:20
@fidencio
Copy link
Member Author

/test

stevenhorsman
stevenhorsman approved these changes Dec 21, 2023
View reviewed changes
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@stevenhorsman stevenhorsman merged commit c5f939c into kata-containers:main Dec 22, 2023
170 of 176 checks passed
stevenhorsman pushed a commit to stevenhorsman/cc-operator that referenced this pull request Dec 22, 2023
@fidencio @stevenhorsman
controller: Handle snapshotter per handler
083f0f5 
Let's start properly setting a specific snapshotter per runtime handler
configured for containerd.

This work depends on a work done on the Kata Containers side to better
support setting snapshotters per runtime handler.

The PR from the Kata Containers side is:
kata-containers/kata-containers#8655.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
stevenhorsman pushed a commit to fidencio/cc-operator that referenced this pull request Dec 22, 2023
@fidencio @stevenhorsman
controller: Handle snapshotter per handler
b78fbde 
Let's start properly setting a specific snapshotter per runtime handler
configured for containerd.

This work depends on a work done on the Kata Containers side to better
support setting snapshotters per runtime handler.

The PR from the Kata Containers side is:
kata-containers/kata-containers#8655.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
fidencio added a commit to fidencio/cc-operator that referenced this pull request Dec 23, 2023
@fidencio
controller: Handle snapshotter per handler
5c2e16a 
Let's start properly setting a specific snapshotter per runtime handler
configured for containerd.

This work depends on a work done on the Kata Containers side to better
support setting snapshotters per runtime handler.

The PR from the Kata Containers side is:
kata-containers/kata-containers#8655.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
fidencio added a commit to fidencio/cc-operator that referenced this pull request Dec 26, 2023
@fidencio
controller: Handle snapshotter per handler
4ee7f81 
Let's start properly setting a specific snapshotter per runtime handler
configured for containerd.

This work depends on a work done on the Kata Containers side to better
support setting snapshotters per runtime handler.

The PR from the Kata Containers side is:
kata-containers/kata-containers#8655.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
fidencio added a commit to fidencio/cc-operator that referenced this pull request Jan 5, 2024
@fidencio
controller: Handle snapshotter per handler
0ba092b 
Let's start properly setting a specific snapshotter per runtime handler
configured for containerd.

This work depends on a work done on the Kata Containers side to better
support setting snapshotters per runtime handler.

The PR from the Kata Containers side is:
kata-containers/kata-containers#8655.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
fidencio added a commit to fidencio/cc-operator that referenced this pull request Jan 5, 2024
@fidencio
controller: Handle snapshotter per handler
a0400af 
Let's start properly setting a specific snapshotter per runtime handler
configured for containerd.

This work depends on a work done on the Kata Containers side to better
support setting snapshotters per runtime handler.

The PR from the Kata Containers side is:
kata-containers/kata-containers#8655.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
fidencio added a commit to confidential-containers/operator that referenced this pull request Jan 5, 2024
@fidencio
controller: Handle snapshotter per handler
4f8e8c8 
Let's start properly setting a specific snapshotter per runtime handler
configured for containerd.

This work depends on a work done on the Kata Containers side to better
support setting snapshotters per runtime handler.

The PR from the Kata Containers side is:
kata-containers/kata-containers#8655.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beraldoleal beraldoleal beraldoleal approved these changes

@amshinde amshinde amshinde approved these changes

@stevenhorsman stevenhorsman stevenhorsman approved these changes

Assignees
No one assigned
Labels
force-skip-ci Stop the CI running for a PR (remember: if you break it, you fix it!) ok-to-test size/medium Average sized task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kata-deploy: Provide a way for the user to select which snapshotter will be set for a specific runtime class
6 participants
@fidencio @ChengyuZhu6 @stevenhorsman @beraldoleal @amshinde @katacontainersbot