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

Use sandboxed CRI by default #8994

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Use sandboxed CRI by default #8994

merged 2 commits into from
Aug 24, 2023

Conversation

mxpv
Copy link
Member

@mxpv mxpv commented Aug 21, 2023

This will use the sandboxed CRI by default leaving an option to fallback to the legacy implementation via DISABLE_CRI_SANDBOXES env.

@mxpv mxpv requested a review from mikebrow August 21, 2023 17:53
@mxpv mxpv added the area/cri Container Runtime Interface (CRI) label Aug 21, 2023
@containerd containerd deleted a comment from k8s-ci-robot Aug 21, 2023
samuelkarp
samuelkarp approved these changes Aug 21, 2023
View reviewed changes
Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

LGTM. We likely also should update the prow jobs so we can have both enabled and disabled modes tested for e2e.

@cpuguy83
Copy link
Member

CI does not like this.

AkihiroSuda
AkihiroSuda reviewed Aug 21, 2023
View reviewed changes
RELEASES.md Outdated Show resolved Hide resolved
@dcantah
Copy link
Member

dcantah commented Aug 22, 2023

CI does not like this.

oof yea, all seem to be:

time="2023-08-22T16:25:10Z" level=fatal msg="validate service connection: validate CRI v1 runtime API for endpoint \"unix:https:///run/containerd-test/containerd.sock\": rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing dial unix /run/containerd-test/containerd.sock: connect: no such file or directory\""

mikebrow
mikebrow approved these changes Aug 22, 2023
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

just a couple nits..

RELEASES.md Outdated Show resolved Hide resolved
pkg/cri/cri.go Outdated
if os.Getenv("ENABLE_CRI_SANDBOXES") != "" {
log.G(ctx).Info("using experimental CRI Sandbox server - unset ENABLE_CRI_SANDBOXES to disable")
if os.Getenv("DISABLE_CRI_SANDBOXES") == "" {
log.G(ctx).Info("using experimental CRI Sandbox server - use DISABLE_CRI_SANDBOXES=1 to fallback to legacy CRI")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log.G(ctx).Info("using experimental CRI Sandbox server - use DISABLE_CRI_SANDBOXES=1 to fallback to legacy CRI")
log.G(ctx).Info("using CRI Sandbox server - use DISABLE_CRI_SANDBOXES=1 to fallback to legacy CRI")

Copy link
Member

Choose a reason for hiding this comment

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

Probably this should be Debug() log

Copy link
Member

Choose a reason for hiding this comment

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

if so.. catch the else case log below

Copy link
Member Author

Choose a reason for hiding this comment

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

Debug is disabled by default on production environments, I think it'd good to keep it visible to raise awareness that we substitute CRI implementation? We can remove this in 2.1 with the legacy CRI.

mikebrow
mikebrow reviewed Aug 22, 2023
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

more changes needed
https://github.com/containerd/containerd/blob/main/containerd.service#L22
https://github.com/containerd/containerd/blob/main/Vagrantfile#L275
https://github.com/containerd/containerd/blob/main/contrib/Dockerfile.test#L97
https://github.com/containerd/containerd/blob/main/integration/sandbox_run_rollback_test.go#L296
https://github.com/containerd/containerd/blob/main/pkg/cri/config/config.go#L81
https://github.com/containerd/containerd/blob/main/script/test/cri-integration.sh#L47-L49
https://github.com/containerd/containerd/blob/main/script/test/utils.sh#L218-L220

@mikebrow
Copy link
Member

mikebrow commented Aug 22, 2023 •
edited
Loading

We likely also should update the prow jobs so we can have both enabled and disabled modes tested for e2e.

noting over on the k8s side.. https://github.com/kubernetes/test-infra/blob/dc4b748b13259a634c779c0d6d0c11b976dc9d33/config/jobs/containerd/containerd/containerd-presubmit-jobs.yaml#L212-L213

@mxpv
Use sandboxed CRI by default
c3f3cad 
Signed-off-by: Maksym Pavlenko <[email protected]>
@mxpv
Update RELEASES
c92f4a1 
Signed-off-by: Maksym Pavlenko <[email protected]>
@containerd containerd deleted a comment from k8s-ci-robot Aug 23, 2023
@AkihiroSuda AkihiroSuda merged commit f48bbef into containerd:main Aug 24, 2023
44 checks passed
@mxpv mxpv deleted the cri branch August 24, 2023 15:16
ionutbalutoiu added a commit to ionutbalutoiu/k8s-e2e-runner that referenced this pull request Aug 29, 2023
@ionutbalutoiu
Set DISABLE_CRI_SANDBOXES=1 containerd env
e865697 
Recently, in the containerd `main` branch, the following PR was merged:
containerd/containerd#8994

This makes containerd to use the new sandboxes CRI, by default.

From my observations, this is causing some issues with `kubectl exec`
with Kubernetes latest `master` branch code, deployed with containerd
latest `main` branch code.

Fallback to the legacy CRI implementation via `DISABLE_CRI_SANDBOXES=1`,
until this issue is fixed. This will be a NOOP for stable containerd
deployments.

Signed-off-by: Ionut Balutoiu <[email protected]>
@ionutbalutoiu ionutbalutoiu mentioned this pull request Aug 29, 2023
Merged
@jsturtevant jsturtevant mentioned this pull request Aug 31, 2023
Closed
@dims dims mentioned this pull request Sep 29, 2023
Merged
@dmcgowan dmcgowan mentioned this pull request Nov 2, 2023
Merged
Thompson1985
Thompson1985 reviewed Mar 28, 2024
View reviewed changes
Copy link

@Thompson1985 Thompson1985 left a comment

Choose a reason for hiding this comment

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

Reviewed

@dmcgowan dmcgowan mentioned this pull request Apr 19, 2024
Merged
@dmcgowan dmcgowan mentioned this pull request May 17, 2024
Merged
@dmcgowan dmcgowan mentioned this pull request Jun 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikebrow mikebrow mikebrow approved these changes

@Thompson1985 Thompson1985 Thompson1985 left review comments

@dmcgowan dmcgowan dmcgowan approved these changes

@samuelkarp samuelkarp samuelkarp approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@dcantah dcantah dcantah approved these changes

Assignees
No one assigned
Labels
area/cri Container Runtime Interface (CRI) impact/changelog
Projects
Pull Request Review
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@mxpv @cpuguy83 @dcantah @mikebrow @dmcgowan @samuelkarp @AkihiroSuda @Thompson1985