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

Separate out cri related code into self contained package #90552

Merged
merged 4 commits into from May 20, 2020
Merged

Separate out cri related code into self contained package #90552

merged 4 commits into from May 20, 2020

Conversation

dims
Copy link
Member

@dims dims commented Apr 28, 2020

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Previous attempts to extract cri related code met with stiff resistance. We had PRs earlier to move streaming to staging/ etc. So in this PR we just consider moving code into a package that be used standalone w/o dragging in all of k/k.

Essentially the pkg/kubelet/cri uses only stuff in staging/ and does not use anything else inside k8s.io/kubernetes. We have a new .import-restrictions in that directory to ensure that things stay that way.

two things to note:

  • had to move logreduction into component-base (anyone have a better suggestion?)
  • copied some of the utils code into remote/ so we will have some duplication.

Also note that i've prototyped this on the containerd/cri side as well:
containerd/cri#1463

If we do this, containerd/cri repo vendor/ directory will pull in 15k LOC less than they do today

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 28, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2020
@dims dims force-pushed the separate-out-cri-related-code-into-self-contained-package branch from 2cd8478 to 01b7bb2 Compare April 29, 2020 10:09
@dims dims changed the title [WIP] Separate out cri related code into self contained package Separate out cri related code into self contained package Apr 29, 2020
@k8s-ci-robot k8s-ci-robot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 29, 2020
@dims
Copy link
Member Author

dims commented Apr 29, 2020

/release-note-none
/priority important-soon

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 29, 2020
@dims
Copy link
Member Author

dims commented Apr 29, 2020

/assign @derekwaynecarr @yujuhong @Random-Liu

@dims
Copy link
Member Author

dims commented May 18, 2020

/assign @tallclair

@mattjmcnaughton
Copy link
Contributor

/assign @mattjmcnaughton

I think we're on the same page conceptually @dims ! Thank you for your work here. I'll defer to one of the other folks tagged around the details, as they have a more complete picture of the whole system than I do.

@dims
Copy link
Member Author

dims commented May 19, 2020

@derekwaynecarr @liggitt here's the plan on the containerd side containerd/cri#1479 (comment)

@mikebrow
Copy link
Member

@derekwaynecarr @liggitt here's the plan on the containerd side containerd/cri#1479 (comment)

@dimms I think you meant to link to containerd/cri#1463 1479 is a different issue.. (that of merging the containerd/containerd and containerd/cri repos.)

@dims
Copy link
Member Author

dims commented May 20, 2020

whoops! thanks @mikebrow

@derekwaynecarr
Copy link
Member

I tried to think if I had any alternate options for splitting this out, but nothing struck me, so this will do for now.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 20, 2020
dims added 4 commits May 20, 2020 10:57
@dims
Move packages for slightly better UX for consumers
5692926 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@dims
update bazel BUILD files
0608e8b 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@dims
Copy util directory for use in cri/remote package
e049e59 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@dims
fix import restrictions and BUILD
c7e79d2 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@dims dims force-pushed the separate-out-cri-related-code-into-self-contained-package branch from 0dc69ee to c7e79d2 Compare May 20, 2020 14:58
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 20, 2020
@derekwaynecarr
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2020
@liggitt
Copy link
Member

liggitt commented May 20, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, dims, liggitt

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 20, 2020
@dims
Copy link
Member Author

dims commented May 20, 2020

/test pull-kubernetes-node-e2e

@dims
Copy link
Member Author

dims commented May 20, 2020

/retest

@dims
Copy link
Member Author

dims commented May 20, 2020

/test pull-kubernetes-node-e2e

1 similar comment
@dims
Copy link
Member Author

dims commented May 20, 2020

/test pull-kubernetes-node-e2e

@k8s-ci-robot k8s-ci-robot merged commit 6eefe7d into kubernetes:master May 20, 2020
@gyuho
Copy link
Member

gyuho commented Jul 13, 2020

Does this PR introduce a user-facing change?:

This change should've highlighted that k8s.io/kubernetes/pkg/kubelet/remote is now k8s.io/kubernetes/pkg/kubelet/cri/remote.

@dims dims mentioned this pull request Sep 23, 2020
Closed
@andrewsykim andrewsykim mentioned this pull request Oct 15, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer Awaiting requested review from feiskyer

Assignees

@lavalamp lavalamp

@liggitt liggitt

@mattjmcnaughton mattjmcnaughton

@Random-Liu Random-Liu

@derekwaynecarr derekwaynecarr

@yujuhong yujuhong

@tallclair tallclair

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

None yet