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 containerd/containerd/api module #3526

Merged
merged 8 commits into from
Jul 11, 2024
Merged

use containerd/containerd/api module #3526

merged 8 commits into from
Jul 11, 2024

Conversation

akhilerm
Copy link
Contributor

@akhilerm akhilerm commented May 4, 2024 •
edited
Loading

  • use containerd/containerd/api module instead of having a copy in third_party
  • use containerd/errdefs module instead of keeping a local copy

@k8s-ci-robot
Copy link
Collaborator

Hi @akhilerm. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@bobbypage
Copy link
Collaborator

/ok-to-test

@bobbypage
Copy link
Collaborator

Very exciting to see this happen!

@bobbypage bobbypage marked this pull request as ready for review May 4, 2024 03:41
@akhilerm
Copy link
Contributor Author

akhilerm commented May 4, 2024

@bobbypage Can you approve the github action workflows also?

@dims
Copy link
Collaborator

dims commented May 4, 2024

/ok-to-test

@akhilerm
Copy link
Contributor Author

akhilerm commented May 7, 2024

/cc @bobbypage @dims for review

akhilerm
akhilerm commented May 7, 2024
View reviewed changes
go.mod Outdated
@@ -1,73 +1,74 @@
module github.com/google/cadvisor

go 1.19
go 1.22.0

replace golang.org/x/net => golang.org/x/net v0.7.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bobbypage Shouldnt this be removed? I am not sure why the version was pinned. Also v0.23.0 fixes CVE-2023-45288

@akhilerm
Copy link
Contributor Author

/hold
will merge once containerd/containerd/api v1.8.0 is released

@akhilerm akhilerm force-pushed the test-containerd-api-module branch from 2f42059 to 7547a2f Compare June 21, 2024 03:53
@akhilerm
Copy link
Contributor Author

@bobbypage Can you approve the workflow. I have rebased against the latest master branch

@iwankgb
Copy link
Collaborator

iwankgb commented Jun 22, 2024

@akhilerm, you have conflicts against master.

@akhilerm akhilerm force-pushed the test-containerd-api-module branch from 7547a2f to 144fff5 Compare June 22, 2024 17:41
@akhilerm
Copy link
Contributor Author

rebased against master and pushed the changes.

@akhilerm akhilerm force-pushed the test-containerd-api-module branch from 144fff5 to cad3611 Compare June 22, 2024 17:49
@akhilerm
Copy link
Contributor Author

The pull-cadvisor-e2e job is failing due to env setup error

/retest pull-cadvisor-e2e

@k8s-ci-robot
Copy link
Collaborator

@akhilerm: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test pull-cadvisor-e2e

Use /test all to run all jobs.

In response to this:

The pull-cadvisor-e2e job is failing due to env setup error

/retest pull-cadvisor-e2e

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-sigs/prow repository.

@akhilerm
Copy link
Contributor Author

/test pull-cadvisor-e2e

1 similar comment
@akhilerm
Copy link
Contributor Author

/test pull-cadvisor-e2e

iwankgb
iwankgb reviewed Jun 28, 2024
View reviewed changes
Copy link
Collaborator

@iwankgb iwankgb left a comment

Choose a reason for hiding this comment

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

Just two minor comments :)

container/containerd/containers/containers.go Outdated Show resolved Hide resolved
container/containerd/handler_test.go Outdated Show resolved Hide resolved
iwankgb
iwankgb requested changes Jun 28, 2024
View reviewed changes
cmd/go.mod Outdated Show resolved Hide resolved
@akhilerm akhilerm force-pushed the test-containerd-api-module branch 2 times, most recently from 076b07e to 3a487fd Compare June 29, 2024 15:15
@akhilerm
Copy link
Contributor Author

/hold

for containerd/typeurl#45 to merge

@akhilerm akhilerm force-pushed the test-containerd-api-module branch 2 times, most recently from 9aaa8d3 to 72387d4 Compare June 29, 2024 17:43
@thaJeztah
Copy link
Contributor

Hmm looks like some images used in tests are using a deprecated format, or is it detecting it wrong?

--- PASS: TestDockerContainerSpec (0.40s)
=== RUN   TestDockerContainerCpuStats
    framework.go:337: Failed to run "sudo" [docker run -d registry.k8s.io/busybox ping www.google.com] in "localhost" with error: "exit status 125". Stdout: "", Stderr: Unable to find image 'registry.k8s.io/busybox:latest' locally
        latest: Pulling from busybox
        docker: [DEPRECATION NOTICE] Docker Image Format v1 and Docker Image manifest version 2, schema 1 support is disabled by default and will be removed in an upcoming release. Suggest the author of registry.k8s.io/busybox:latest to upgrade the image to the OCI Format or Docker Image manifest v2, schema 2. More information at [https://docs.docker.com/go/deprecated-image-specs/.](https://docs.docker.com/go/deprecated-image-specs/)
        See 'docker run --help'.

@iwankgb
Copy link
Collaborator

iwankgb commented Jul 6, 2024

@thaJeztah, it's fixed on master :)

@akhilerm akhilerm force-pushed the test-containerd-api-module branch from 72387d4 to 8ac20a1 Compare July 7, 2024 06:50
@akhilerm
Copy link
Contributor Author

akhilerm commented Jul 7, 2024

rebased and pushed.

@akhilerm akhilerm force-pushed the test-containerd-api-module branch from 8ac20a1 to 83f1c5f Compare July 9, 2024 02:48
@akhilerm
Copy link
Contributor Author

akhilerm commented Jul 9, 2024

@iwankgb Can you approve the workflows once more.

Once we have a solution for kubernetes-sigs/prow#194, one time approval should do the job.

@iwankgb
Copy link
Collaborator

iwankgb commented Jul 11, 2024

@akhilerm, you have some conflicts in go.mod :(

akhilerm added 7 commits July 11, 2024 20:22
@akhilerm
use containerd/api module instead of third_party
3152233 
- use containerd/api v1.8.0-rc.2 instead of relying on third_party directory
- remove script to copy /api from containerd/containerd

Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
@akhilerm
fix gogo/protobuf -> google.golang.org/protobuf
ad3cbfc 
Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
@akhilerm
use Marshal funcs from containerd and fix typeurl
a3c5683 
Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
@akhilerm
replace errdefs pkg with containerd/errdefs
eca9e05 
Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
@akhilerm
remove anypb import aliasing
1de4dcb 
Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
@akhilerm
Update containerd/containerd/api to v1.7.19
47bd5bf 
Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
@akhilerm
sort based on goimports
6fe1eb0 
Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
@akhilerm akhilerm force-pushed the test-containerd-api-module branch from b64ac55 to 6fe1eb0 Compare July 11, 2024 14:54
@akhilerm
update typeurl to v2.2.0
c73fbd8 
Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
@iwankgb iwankgb merged commit c9375fe into google:master Jul 11, 2024
14 checks passed
@akhilerm akhilerm deleted the test-containerd-api-module branch July 11, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah left review comments

@iwankgb iwankgb iwankgb approved these changes

Assignees
No one assigned
Labels
ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@akhilerm @k8s-ci-robot @bobbypage @dims @iwankgb @thaJeztah