CRI: Pass sandbox annotations to _other platforms by dcantah · Pull Request #8060 · containerd/containerd

dcantah · 2023-02-07T12:06:08Z

!windows and !linux weren't getting passed the sandbox annotations.

This additionally adds in a commit on top to refactor how these annotations were getting passed. All of the CRI sandbox and container specs all get assigned almost the exact same default annotations (sandboxID, name, metadata, container type etc.) so I added a helper to return the right set for a sandbox or regular workload container.

k8s-ci-robot · 2023-02-07T12:06:10Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ruiwen-zhao · 2023-02-10T19:53:29Z

LGTM. Does it make sense to add checks to unit tests like in

containerd/pkg/cri/server/sandbox_run_linux_test.go

Lines 71 to 88 in d5ae2c1

    
           t.Logf("Check PodSandbox annotations") 
        
           assert.Contains(t, spec.Annotations, annotations.SandboxID) 
        
           assert.EqualValues(t, spec.Annotations[annotations.SandboxID], id) 
        
           assert.Contains(t, spec.Annotations, annotations.ContainerType) 
        
           assert.EqualValues(t, spec.Annotations[annotations.ContainerType], annotations.ContainerTypeSandbox) 
        
           assert.Contains(t, spec.Annotations, annotations.SandboxNamespace) 
        
           assert.EqualValues(t, spec.Annotations[annotations.SandboxNamespace], "test-ns") 
        
           assert.Contains(t, spec.Annotations, annotations.SandboxUID) 
        
           assert.EqualValues(t, spec.Annotations[annotations.SandboxUID], "test-uid") 
        
           assert.Contains(t, spec.Annotations, annotations.SandboxName) 
        
           assert.EqualValues(t, spec.Annotations[annotations.SandboxName], "test-name") 
        
           assert.Contains(t, spec.Annotations, annotations.SandboxLogDir) 
        
           assert.EqualValues(t, spec.Annotations[annotations.SandboxLogDir], "test-log-directory")

and

containerd/pkg/cri/server/container_create_linux_test.go

Lines 174 to 191 in d5ae2c1

    
           t.Logf("Check PodSandbox annotations") 
        
           assert.Contains(t, spec.Annotations, annotations.SandboxID) 
        
           assert.EqualValues(t, spec.Annotations[annotations.SandboxID], sandboxID) 
        
           assert.Contains(t, spec.Annotations, annotations.ContainerType) 
        
           assert.EqualValues(t, spec.Annotations[annotations.ContainerType], annotations.ContainerTypeContainer) 
        
           assert.Contains(t, spec.Annotations, annotations.SandboxNamespace) 
        
           assert.EqualValues(t, spec.Annotations[annotations.SandboxNamespace], "test-sandbox-ns") 
        
           assert.Contains(t, spec.Annotations, annotations.SandboxUID) 
        
           assert.EqualValues(t, spec.Annotations[annotations.SandboxUID], "test-sandbox-uid") 
        
           assert.Contains(t, spec.Annotations, annotations.SandboxName) 
        
           assert.EqualValues(t, spec.Annotations[annotations.SandboxName], "test-sandbox-name") 
        
           assert.Contains(t, spec.Annotations, annotations.ImageName) 
        
           assert.EqualValues(t, spec.Annotations[annotations.ImageName], testImageName)

cpuguy83

Change LGTM although it seems like this could be extracted out so every platform can reuse the same code (maybe? I didn't peak through the other paltform files but I assume it's the same).

dcantah · 2023-02-10T22:37:33Z

@ruiwen-zhao Let me take a look later today. @cpuguy83 it likely can, these should get passed on all platforms so this was the lazy approach thinking about it. I'll make a generic func that'll return these all and have all the platforms call it.

estesp

LGTM

estesp · 2023-02-13T16:15:10Z

Needs a rebase; conflict in container_create_linux.go

!windows and !linux weren't getting passed the sandbox annotations. Signed-off-by: Danny Canter <[email protected]>

All of the CRI sandbox and container specs all get assigned almost the exact same default annotations (sandboxID, name, metadata, container type etc.) so lets make a helper to return the right set for a sandbox or regular workload container. Signed-off-by: Danny Canter <[email protected]>

mxpv

LGTM once green!

dcantah marked this pull request as ready for review February 7, 2023 12:06

dcantah added kind/enhancement area/cri Container Runtime Interface (CRI) easy-to-review Easy to review labels Feb 7, 2023

dcantah added this to the 1.7 milestone Feb 7, 2023

cpuguy83 approved these changes Feb 10, 2023

View reviewed changes

estesp approved these changes Feb 13, 2023

View reviewed changes

cpuguy83 approved these changes Feb 13, 2023

View reviewed changes

mxpv reviewed Feb 13, 2023

View reviewed changes

Comment thread pkg/cri/sbserver/container_create.go Outdated

dcantah force-pushed the cri-annots-other branch from 0aef2c0 to ff86019 Compare February 13, 2023 21:03

CRI: Pass sandbox annotations to _other platforms

5aab634

!windows and !linux weren't getting passed the sandbox annotations. Signed-off-by: Danny Canter <[email protected]>

mxpv reviewed Feb 13, 2023

View reviewed changes

Comment thread pkg/cri/annotations/annotations.go

dcantah force-pushed the cri-annots-other branch from ff86019 to 646bc3a Compare February 13, 2023 21:05

mxpv approved these changes Feb 13, 2023

View reviewed changes

mxpv merged commit 3548f59 into containerd:main Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRI: Pass sandbox annotations to _other platforms#8060

CRI: Pass sandbox annotations to _other platforms#8060
mxpv merged 2 commits intocontainerd:mainfrom
dcantah:cri-annots-other

dcantah commented Feb 7, 2023 •

edited

Loading

Uh oh!

k8s-ci-robot commented Feb 7, 2023

Uh oh!

ruiwen-zhao commented Feb 10, 2023

Uh oh!

cpuguy83 left a comment

Uh oh!

dcantah commented Feb 10, 2023

Uh oh!

estesp left a comment

Uh oh!

estesp commented Feb 13, 2023

Uh oh!

Uh oh!

Uh oh!

mxpv left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

dcantah commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 7, 2023

Uh oh!

ruiwen-zhao commented Feb 10, 2023

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

dcantah commented Feb 10, 2023

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

estesp commented Feb 13, 2023

Uh oh!

Uh oh!

Uh oh!

mxpv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dcantah commented Feb 7, 2023 •

edited

Loading

mxpv left a comment •

edited

Loading