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

rework containerd config #1818

Merged
merged 2 commits into from
Aug 27, 2020
Merged

rework containerd config #1818

merged 2 commits into from
Aug 27, 2020

Conversation

BenTheElder
Copy link
Member

now that we skip preflight we can simply use a fixed pause image version controlled by containerd.

fixes: #1719 as well

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 26, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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 requested review from aojea and munnerz August 26, 2020 22:09
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 26, 2020
@BenTheElder BenTheElder force-pushed the zfs-stash branch 2 times, most recently from b1f8cf5 to bb01db5 Compare August 27, 2020 00:46
@BenTheElder
Copy link
Member Author

/retest

@BenTheElder BenTheElder added this to the v0.9.0 milestone Aug 27, 2020
@BenTheElder
Copy link
Member Author

[sig-cli] Kubectl client Simple pod should support port-forward

everything checks out now except one thing: the kubelet flag regarding the pause image may not match.
this flag does two things:

  • configures dockershim to use it to implement pod sandboxes (not relevant to CRI, it's up to the implementation)
  • configures the imageGC to exclude this image (and only this image... it's a special codepath)

the latter we might care about if we we enable imageGC someday, but we also have that issue with other images

@BenTheElder
Copy link
Member Author

/test all

@BenTheElder
Copy link
Member Author

/cc @amwat

@k8s-ci-robot k8s-ci-robot requested a review from amwat August 27, 2020 07:24
aojea
aojea reviewed Aug 27, 2020
View reviewed changes
images/base/files/usr/local/bin/entrypoint
@@ -18,6 +18,23 @@ set -o errexit
set -o nounset
set -o pipefail

configure_containerd() {
# we need to switch to the 'native' snapshotter on zfs
if [[ "$(stat -f -c %T /kind)" == 'zfs' ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need this https://github.com/kubernetes-sigs/kind/blob/master/pkg/cluster/internal/providers/docker/provision.go#L208 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to handle btrfs too?

Copy link
Member Author

@BenTheElder BenTheElder Aug 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

we have prior art for zfs's best option being falling back to the native driver because the zfs mode is sketchy at least in the kind context and overlay driver doesn't work.

we don't for btrfs.

if someone reports this with evidence, we can add it. I think btrfs is already fixed but can't confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

see: #1719

@aojea
Copy link
Contributor

aojea commented Aug 27, 2020

/lgtm
/hold
unhold when necessary :)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 27, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2020
@BenTheElder
Copy link
Member Author

/unhold
/retest

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 27, 2020
@k8s-ci-robot k8s-ci-robot merged commit c13c54b into kubernetes-sigs:master Aug 27, 2020
@BenTheElder BenTheElder deleted the zfs-stash branch August 27, 2020 20:22
@BenTheElder BenTheElder mentioned this pull request Sep 1, 2020
Closed
BenTheElder
BenTheElder commented Nov 13, 2020
View reviewed changes
images/base/files/usr/local/bin/entrypoint
configure_containerd() {
# we need to switch to the 'native' snapshotter on zfs
if [[ "$(stat -f -c %T /kind)" == 'zfs' ]]; then
sed -i 's/snapshotter = "overlayfs"/snapshotter = "native"/' /etc/containerd/config.toml
Copy link
Member Author

Choose a reason for hiding this comment

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

this line itself is fine except we forgot to put the explicit snapshotter in the config. #1891

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea aojea left review comments

@munnerz munnerz Awaiting requested review from munnerz

@amwat amwat Awaiting requested review from amwat

Assignees

@aojea aojea

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.9.0
Development

Successfully merging this pull request may close these issues.

Create cluster fails - kind-control-plane does not work on zfs
3 participants
@BenTheElder @k8s-ci-robot @aojea