Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Remove dependencies on k8s.io/kubernetes #1463

Merged

Conversation

dims
Copy link
Member

@dims dims commented Apr 25, 2020

Break the link to k8s.io/kubernetes by copying some code over. We should only use public/versioned components from kubernetes (i.e, we should not depend on k8s.io/kubernetes as it is internal API subject to lots of changes)

"k8s.io/kubernetes/pkg/kubelet/cri/remote"    -> "github.com/containerd/cri/integration/remote"
"k8s.io/kubernetes/pkg/kubelet/cri/streaming" -> "github.com/containerd/cri/pkg/streaming"
"k8s.io/kubernetes/pkg/kubelet/util"          -> "github.com/containerd/cri/integration/util"
"k8s.io/kubernetes/pkg/util/bandwidth"        -> "github.com/containerd/cri/pkg/server/bandwidth"

Notes on how to copy stuff next time:

  • Copy files from specified directories
  • Edit the doc.go to drop the trailing comment in the package statements (that pin the package name to k8s.io/kubernetes)
  • Run ltag -t "../project/script/validate/template" -v to fix up the Copyright statements.
  • Run golangci-lint run to make sure there are no issues.

@k8s-ci-robot
Copy link

Hi @dims. Thanks for your PR.

I'm waiting for a containerd 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/test-infra repository.

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.

Overall comment... will k8s not move the streaming package out, I thought it was scheduled for extraction?

This would be a big fork... lots to keep in sync when keps/fixes drop.

integration/main_test.go Outdated Show resolved Hide resolved
pkg/server/service.go Show resolved Hide resolved
pkg/util/remote.go Outdated Show resolved Hide resolved
@mikebrow
Copy link
Member

/ok-to-test

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

The copyright headers must be kept

@dims
Copy link
Member Author

dims commented Apr 25, 2020

@mikebrow yep. thinking aloud here by filing this WIP.

So the thought here is may be we should just create a repo here in containerd for things we need here to cut the cord to k8s.io/kubernetes. We may be able to convince folks in k/k to use that repo and drop the code from k/k.

@dims dims mentioned this pull request Apr 25, 2020
@dims
Copy link
Member Author

dims commented Apr 25, 2020

@mikebrow we may be able to get away with a separate go.mod file for integration, but that would not work well with vndr ( i haven't tried yet ). but we can't get away from doing something about streaming for sure.

@dims
Copy link
Member Author

dims commented Apr 25, 2020

@mikebrow
Copy link
Member

The copyright headers must be kept

if we go this way ... since it's an abrupt copy... we can paste our new headers above the old ones.

@mikebrow
Copy link
Member

@mikebrow yep. thinking aloud here by filing this WIP.

So the thought here is may be we should just create a repo here in containerd for things we need here to cut the cord to k8s.io/kubernetes. We may be able to convince folks in k/k to use that repo and drop the code from k/k.

That makes sense.. Thanks. Nice to have a wip PR even if only just for testing the process and getting closer to go mod while waiting for the k8s decision. Natives are getting very restless on this whole go mod thing. Personally I'm fine with vndr but I realize we are holding others up.

@mikebrow
Copy link
Member

mikebrow commented Apr 25, 2020

@mikebrow we may be able to get away with a separate go.mod file for integration, but that would not work well with vndr ( i haven't tried yet ). but we can't get away from doing something about streaming for sure.

true we can do a /tmp for integration..

@dims
Copy link
Member Author

dims commented May 1, 2020

@mikebrow going to see if i can push for something in k/k - please chime in on kubernetes/kubernetes#90552

@dims dims closed this May 1, 2020
@mikebrow
Copy link
Member

@dims can we just split this up into multiples PRs... e.g. one for the integration/ dependencies on k8s namely kubelet/remote and kubelet/util.....

Then we can tackle server/streaming hopefully by importing it from a common repo..
There's also pkg/util/bandwidth..

@dims
Copy link
Member Author

dims commented May 12, 2020

Ack @mikebrow will do soon hopefully in a few days

@dims dims reopened this Jun 18, 2020
@dims dims force-pushed the remove-dependencies-on-k8s.io/kubernetes branch 6 times, most recently from 2ac430e to 24599d0 Compare June 18, 2020 20:34
@dims
Copy link
Member Author

dims commented Jun 18, 2020

/test pull-cri-containerd-node-e2e

Signed-off-by: Davanum Srinivas <[email protected]>
@dims dims force-pushed the remove-dependencies-on-k8s.io/kubernetes branch from 24599d0 to 79a5297 Compare June 22, 2020 18:32
dims added 4 commits June 22, 2020 14:48
Signed-off-by: Davanum Srinivas <[email protected]>
Signed-off-by: Davanum Srinivas <[email protected]>
Signed-off-by: Davanum Srinivas <[email protected]>
Signed-off-by: Davanum Srinivas <[email protected]>
@dims dims force-pushed the remove-dependencies-on-k8s.io/kubernetes branch from 79a5297 to cbb7c28 Compare June 22, 2020 18:49
Signed-off-by: Davanum Srinivas <[email protected]>
@dims
Copy link
Member Author

dims commented Jun 22, 2020

/test pull-cri-containerd-node-e2e

@dims dims changed the title [WIP] Remove dependencies on k8s.io/kubernetes Remove dependencies on k8s.io/kubernetes Jun 22, 2020
@dims
Copy link
Member Author

dims commented Jun 22, 2020

/test pull-cri-containerd-node-e2e

1 similar comment
@mikebrow
Copy link
Member

/test pull-cri-containerd-node-e2e

@dims
Copy link
Member Author

dims commented Jun 23, 2020

Added notes on how to copy stuff next time.

@dims
Copy link
Member Author

dims commented Jun 23, 2020

Added notes on how to do this next time :)

@dims
Copy link
Member Author

dims commented Jun 23, 2020

Some stats on what we pulled in:

[dims@dims-a01 08:31] ~/go/src/github.com/containerd/cri ⟩ gocloc integration/remote
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Go                               7            248            259            790
-------------------------------------------------------------------------------
TOTAL                            7            248            259            790
-------------------------------------------------------------------------------
[dims@dims-a01 08:31] ~/go/src/github.com/containerd/cri ⟩ gocloc pkg/streaming
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Go                              12            293            494           1298
-------------------------------------------------------------------------------
TOTAL                           12            293            494           1298
-------------------------------------------------------------------------------
[dims@dims-a01 08:31] ~/go/src/github.com/containerd/cri ⟩ gocloc integration/util
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Go                               7            122            207            271
-------------------------------------------------------------------------------
TOTAL                            7            122            207            271
-------------------------------------------------------------------------------
[dims@dims-a01 08:31] ~/go/src/github.com/containerd/cri ⟩ gocloc pkg/server/bandwidth
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Go                               6            102            194            378
-------------------------------------------------------------------------------
TOTAL                            6            102            194            378
-------------------------------------------------------------------------------

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.

LGTM

@dmcgowan
Copy link
Member

Is there going to be a follow up change here to reduce the scope of these moved libraries?

Copy link
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

LGTM

@dmcgowan dmcgowan merged commit 98aadbb into containerd:master Jun 23, 2020
@dims
Copy link
Member Author

dims commented Jun 23, 2020

@dmcgowan yes will poke at it as time permits.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants