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.

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

Adds a crictl package for kubeadm installs #64836

Merged
merged 1 commit into from Jun 7, 2018

Conversation

@chuckha
Copy link
Member

chuckha commented Jun 6, 2018

Closes kubernetes/kubeadm#811

Signed-off-by: Chuck Ha ha.chuck@gmail.com

What this PR does / why we need it:
This PR packages crictl into a deb and rpm so we can reference / rely on it in kubeadm.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#811

Special notes for your reviewer:
I think this change doesn't actually deploy the deb, I'll have to port this over to test-infra, but this is the first step.

Also I might need help on the release note

Release note:

kubernetes now packages cri-tools (crictl) in addition to all the other kubeadm tools in a deb and rpm.

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews @luxas @timstclair

@timothysc
Copy link
Member

timothysc left a comment

Just as a general note, these aren't the actual release artifacts you'll need to PR a similar change to the release repo as well.

I'll hold comments on specs b/c we should clean house on * imo in 1.12

@@ -58,6 +58,14 @@ http_file(
urls = mirror("https://storage.googleapis.com/kubernetes-release/network-plugins/cni-plugins-amd64-v0.6.0.tgz"),
)

CRI_TOOLS_VERSION = "1.0.0-beta.1"

This comment has been minimized.

@timothysc

timothysc Jun 6, 2018

Member

ug, when it the release?

This comment has been minimized.

@luxas

This comment has been minimized.

@luxas

luxas Jun 6, 2018

Member

Any chance we can sync this with the cri_tools_version rule?

This comment has been minimized.

@ixdy

ixdy Jun 7, 2018

Member

I agree (in principal) with @luxas - I'd rather this be specified only in one place.

I don't think there's any way for another rule to depend on this constant here in the WORKSPACE file, but we could create a build/workspace.bzl file, put CRI_TOOLS_VERSION in it, and then depend on that and in the k8s_deb rule.

(ETCD_VERSION should probably be treated similarly.)

This comment has been minimized.

@feiskyer

feiskyer Jun 7, 2018

Member

1.0.0-beta.1 is released on May 4. We're planning to release v1.11.0 for k8s v1.11

@@ -167,6 +174,12 @@ The Container Networking Interface tools for provisioning container networks.
version_file = "//build:cni_package_version",
)

k8s_deb(

This comment has been minimized.

@timothysc

timothysc Jun 6, 2018

Member

So is there a deb rule but not an rpm rule?

/cc @ixdy

This comment has been minimized.

@chuckha

chuckha Jun 6, 2018

Author Member

yes, that's correct.

It might help to know the k8s deb rule is not very exciting.

I expect there is no k8s_rpm because the metadata is different for pkg_rpm and is mostly found in the spec files so there's no need to wrap pkg_rpm. Might be useful for ensuring we only build for one architecture, but I don't know that it's worth the overhead.

@k8s-ci-robot k8s-ci-robot requested a review from ixdy Jun 6, 2018

Binaries to interface with the container runtime.

%prep
tar -xz -f {crictl-v1.0.0-beta.1-linux-amd64.tar.gz}

This comment has been minimized.

@detiber

detiber Jun 6, 2018

Member

s/v1.0.0-beta.1/%{version}/

This comment has been minimized.

@chuckha

chuckha Jun 6, 2018

Author Member

if this pulls version from the version file, this is a non-starter, please see kubernetes-sigs/cri-tools#324.

This comment has been minimized.

@detiber

detiber Jun 6, 2018

Member

No, this is set automatically to the version that is specified in the Version tag above during build time.

This comment has been minimized.

@chuckha

chuckha Jun 6, 2018

Author Member

I am very happy to get rid of this hard coded version! excellent tip.

This comment has been minimized.

@chuckha

chuckha Jun 6, 2018

Author Member

turns out the version is populated by the version I pass into the pkg_rpm which means this will have an _ which then won't find the file. Overall bummed, but it's all TODO'd.

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented Jun 6, 2018

@k8s-ci-robot k8s-ci-robot requested a review from Random-Liu Jun 6, 2018

@dims

This comment has been minimized.

Copy link
Member

dims commented Jun 6, 2018

/area kubeadm

@chuckha chuckha force-pushed the chuckha:crictl branch 2 times, most recently from 26d9a73 to 7c1144a Jun 6, 2018

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented Jun 6, 2018

/retest

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented Jun 6, 2018

/test pull-kubernetes-e2e-gce-100-performance

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented Jun 6, 2018

/test pull-kubernetes-kubemark-e2e-gce-big

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented Jun 6, 2018

/test pull-kubernetes-node-e2e

build/BUILD Outdated
@@ -96,6 +96,13 @@ genrule(
cmd = "echo 0.5.1 >$@",
)

genrule(
name = "cri_tools_version",

This comment has been minimized.

@luxas

luxas Jun 6, 2018

Member

cri_tools_package_version and cri_tools_version to be consistent with cni above?

This comment has been minimized.

@chuckha

chuckha Jun 6, 2018

Author Member

yeah, that sounds right. best to be consistent here.

@@ -86,6 +87,12 @@ pkg_tar(
deps = ["@kubernetes_cni//file"],
)

pkg_tar(
name = "cri_tools-data",

This comment has been minimized.

@luxas

luxas Jun 6, 2018

Member

cri_tools_data?

This comment has been minimized.

@chuckha

chuckha Jun 6, 2018

Author Member

the -data is required because i'm using the k8s_deb rule:

https://github.com/kubernetes/repo-infra/blob/master/defs/deb.bzl#L26

@@ -58,6 +58,14 @@ http_file(
urls = mirror("https://storage.googleapis.com/kubernetes-release/network-plugins/cni-plugins-amd64-v0.6.0.tgz"),
)

CRI_TOOLS_VERSION = "1.0.0-beta.1"

This comment has been minimized.

@luxas
http_file(
name = "cri_tools",
sha256 = "bdc838174778223a1af4bdeaaed4bd266120c0e152588f78750fb86221677fb4",
urls = mirror("https://github.com/kubernetes-incubator/cri-tools/releases/download/v%s/crictl-v%s-linux-amd64.tar.gz" % (CRI_TOOLS_VERSION, CRI_TOOLS_VERSION)),

This comment has been minimized.

@luxas

luxas Jun 6, 2018

Member

great parameterization here, we actually get this wrong for CNI I see... 🤔

This comment has been minimized.

@chuckha

chuckha Jun 6, 2018

Author Member

nah, i saw that too and checked it and it's actually right.

This comment has been minimized.

@chuckha

chuckha Jun 6, 2018

Author Member

unless you mean literally that we don't parameterize the cni -- the url is correct though

edit yes this must be what you mean, i thought you were talking about the fact that mirror() goes to google storage and the link is for google storage.

@@ -58,6 +58,14 @@ http_file(
urls = mirror("https://storage.googleapis.com/kubernetes-release/network-plugins/cni-plugins-amd64-v0.6.0.tgz"),
)

CRI_TOOLS_VERSION = "1.0.0-beta.1"

This comment has been minimized.

@luxas

luxas Jun 6, 2018

Member

Any chance we can sync this with the cri_tools_version rule?

@@ -58,6 +58,14 @@ http_file(
urls = mirror("https://storage.googleapis.com/kubernetes-release/network-plugins/cni-plugins-amd64-v0.6.0.tgz"),
)

CRI_TOOLS_VERSION = "1.0.0-beta.1"

http_file(

This comment has been minimized.

@chuckha

chuckha Jun 6, 2018

Author Member

I REALLY wish http_file let me rename the file after downloading.

@chuckha chuckha force-pushed the chuckha:crictl branch from 7c1144a to df0ee2f Jun 6, 2018

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented Jun 6, 2018

sorry @detiber the %{version} is going to have to wait :'(

@detiber

This comment has been minimized.

Copy link
Member

detiber commented Jun 7, 2018

/lgtm

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 7, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@chuckha @detiber @ixdy @luxas @timothysc

Pull Request Labels
  • sig/cluster-lifecycle: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/bug: Fixes a bug discovered during the current release.
Help
@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Jun 7, 2018

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from cblecker Jun 7, 2018

@@ -58,6 +58,14 @@ http_file(
urls = mirror("https://storage.googleapis.com/kubernetes-release/network-plugins/cni-plugins-amd64-v0.6.0.tgz"),
)

CRI_TOOLS_VERSION = "1.0.0-beta.1"

This comment has been minimized.

@ixdy

ixdy Jun 7, 2018

Member

I agree (in principal) with @luxas - I'd rather this be specified only in one place.

I don't think there's any way for another rule to depend on this constant here in the WORKSPACE file, but we could create a build/workspace.bzl file, put CRI_TOOLS_VERSION in it, and then depend on that and in the k8s_deb rule.

(ETCD_VERSION should probably be treated similarly.)

k8s_deb(
name = "cri_tools",
description = """Container Runtime Interface tools (crictl)""",
version_file = "//build:cri_tools_package_version",

This comment has been minimized.

@ixdy

ixdy Jun 7, 2018

Member

it isn't work using version_file if it never changes. just use the version attribute instead (which takes a string, rather than a file).

@k8s-ci-robot k8s-ci-robot added size/M and removed lgtm size/S labels Jun 7, 2018

@chuckha chuckha force-pushed the chuckha:crictl branch from 4cb8a19 to 732287b Jun 7, 2018

Adds a crictl package for kubeadm installs
Closes kubernetes/kubeadm#811

Signed-off-by: Chuck Ha <ha.chuck@gmail.com>

@chuckha chuckha force-pushed the chuckha:crictl branch from 732287b to 03456a3 Jun 7, 2018

@@ -0,0 +1,21 @@
Name: cri-tools
Version: OVERRIDE_THIS

This comment has been minimized.

@dixudx

dixudx Jun 7, 2018

Member

Forgot to change here?


%prep
# TODO(chuckha): update this to use %{version} when the dash is removed from the release
tar -xzf {crictl-v1.0.0-beta.1-linux-amd64.tar.gz}

This comment has been minimized.

@detiber

detiber Jun 7, 2018

Member

You could set a shell variable before the tar command to replace the '_' with a '-' to avoid duplicating the version here.

This comment has been minimized.

@chuckha

chuckha Jun 7, 2018

Author Member

Let's track it in a ticket as discussed offline and leave this TODO here for now.

This comment has been minimized.

@chuckha
@detiber

This comment has been minimized.

Copy link
Member

detiber commented Jun 7, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 7, 2018

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented Jun 7, 2018

@ixdy is there anything else I should do before approval?

@ixdy

This comment has been minimized.

Copy link
Member

ixdy commented Jun 7, 2018

/lgtm

@ixdy

This comment has been minimized.

Copy link
Member

ixdy commented Jun 7, 2018

as noted in #64836 (review), these rules are used for CI but not the official release (yet).

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 7, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, detiber, ixdy

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-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 7, 2018

Automatic merge from submit-queue (batch tested with PRs 64836, 64890). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 9ad7b5c into kubernetes:master Jun 7, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation chuckha authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@zparnold

This comment has been minimized.

Copy link
Member

zparnold commented Jun 13, 2018

Hello there! @chuckha I'm Zach Arnold working on Docs for the 1.11 release. This PR was identified as one needing some documentation in the https://github.com/kubernetes/website repo around your contributions (thanks by the way!) When you have some time, could you please modify/add/remove the relevant content that needs changing in our documentation repo? Thanks! Please let me or my colleague Misty know (@zparnold/@Misty on K8s Slack) if you need any assistance with the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.