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

2nd try at using a vanity GCR name #57824

Merged
merged 1 commit into from Feb 8, 2018

Conversation

@thockin
Member

thockin commented Jan 4, 2018

The 2nd commit here is the changes relative to the reverted PR. Please focus review attention on that.

This is the 2nd attempt. The previous try (#57573) was reverted while we
figured out the regional mirrors (oops).

New plan: k8s.gcr.io is a read-only facade that auto-detects your source
region (us, eu, or asia for now) and pulls from the closest. To publish
an image, push k8s-staging.gcr.io and it will be synced to the regionals
automatically (similar to today). For now the staging is an alias to
gcr.io/google_containers (the legacy URL).

When we move off of google-owned projects (working on it), then we just
do a one-time sync, and change the google-internal config, and nobody
outside should notice.

We can, in parallel, change the auto-sync into a manual sync - send a PR
to "promote" something from staging, and a bot activates it. Nice and
visible, easy to keep track of.

xref kubernetes/release#281

TL;DR:

  • The new staging-k8s.gcr.io is where we push images. It is literally an alias to gcr.io/google_containers (the existing repo) and is hosted in the US.
  • The contents of staging-k8s.gcr.io are automatically synced to {asia,eu,us)-k8s.gcr.io.
  • The new k8s.gcr.io will be a read-only alias to whichever regional repo is closest to you.
  • In the future, images will be promoted from staging to regional "prod" more explicitly and auditably.
Use "k8s.gcr.io" for pulling container images rather than "gcr.io/google_containers".  Images are already synced, so this should not impact anyone materially.
   
Documentation and tools should all convert to the new name. Users should take note of this in case they see this new name in the system.
@@ -177,15 +177,15 @@ filename | sha256 hash
### Other notable changes
* kube-apiserver now drops unneeded path information if an older version of Windows kubectl sends it. ([#44586](https://github.com/kubernetes/kubernetes/pull/44586), [@mml](https://github.com/mml))
* Bump gcr.io/google_containers/glbc from 0.8.0 to 0.9.2. Release notes: [0.9.0](https://github.com/kubernetes/ingress/releases/tag/0.9.0), [0.9.1](https://github.com/kubernetes/ingress/releases/tag/0.9.1), [0.9.2](https://github.com/kubernetes/ingress/releases/tag/0.9.2) ([#43098](https://github.com/kubernetes/kubernetes/pull/43098), [@timstclair](https://github.com/timstclair))
* Bump k8s.gcr.io/glbc from 0.8.0 to 0.9.2. Release notes: [0.9.0](https://github.com/kubernetes/ingress/releases/tag/0.9.0), [0.9.1](https://github.com/kubernetes/ingress/releases/tag/0.9.1), [0.9.2](https://github.com/kubernetes/ingress/releases/tag/0.9.2) ([#43098](https://github.com/kubernetes/kubernetes/pull/43098), [@timstclair](https://github.com/timstclair))

This comment has been minimized.

@cblecker

cblecker Jan 4, 2018

Member

Is it worth updating historial changelogs?

This comment has been minimized.

@thockin

thockin Jan 4, 2018

Member

I waffle on this. The image may eventually cease to exist at the old URL? Or maybe we simply say it won't?

This comment has been minimized.

@cblecker

cblecker Jan 4, 2018

Member

My thought is, the application configuration and manifests for, say 1.4, will still be looking for the image at gcr.io/google_containers/glbc. Modifying the changelog, doesn't go back and create a new patch release of 1.4 updating the config/manifests to look at the new place.

I'd say it would be worth doing for any version we are actually going to go back and change. If this is just for 1.10 forward, then I'd suggest leaving the historical change logs be.

This comment has been minimized.

@thockin
@cblecker

This comment has been minimized.

Member

cblecker commented Jan 4, 2018

FYI cross is failing across the board: #57822

push: build
gcloud docker -- push gcr.io/google_containers/$(IMAGE):$(TAG)
gcloud docker -- push k8s-staging.gcr.io/$(IMAGE):$(TAG)

This comment has been minimized.

@dekkagaijin

dekkagaijin Jan 4, 2018

Contributor

Thinking about it now, I think it's a good idea to manually authenticate for k8s-staging for as long as the workflow continues to use gcloud. This makes the registry change backwards-compatible with older versions of gcloud.

s/gcloud docker -- push k8s-staging.gcr.io/gcloud docker -s k8s-staging -- push k8s-staging.gcr.io/g

@@ -14,7 +14,7 @@
all: build
REGISTRY ?= gcr.io/google-containers
REGISTRY ?= k8s-staging.gcr.io

This comment has been minimized.

@dekkagaijin

dekkagaijin Jan 4, 2018

Contributor

I'd modify this and other similar Makefiles to use gcloud docker -s $(REGISTRY) -- push (subbing REGISTRY for PREFIX or whatever the case may be, but I understand that this is a more involved change.
Otherwise, it'll require another gcloud docker update to be pushed to authenticate to k8s-staging.gcr.io, which wouldn't be the end of the world.

@@ -478,7 +478,7 @@ function stage-images() {
local docker_cmd=("docker")
if [[ "${KUBE_DOCKER_REGISTRY}" == "gcr.io/"* ]]; then
if [[ "${KUBE_DOCKER_REGISTRY}" =~ "gcr.io/" ]]; then
local docker_push_cmd=("gcloud" "docker")

This comment has been minimized.

@dekkagaijin

dekkagaijin Jan 4, 2018

Contributor

surprised this works without gcloud getting angry at the lack of --, but this might have to change as well depending on what the value of KUBE_DOCKER_REGISTRY is. How's this:

if [[ "${KUBE_DOCKER_REGISTRY}" =~ "staging-k8s\.gcr\.io/" ]]; then
  local docker_push_cmd=("gcloud" "docker" "-s" "staging-k8s.gcr.io" "--")
elif  [[ "${KUBE_DOCKER_REGISTRY}" =~ "gcr\.io/" ]]; then
  local docker_push_cmd=("gcloud" "docker" "--")
else
  local docker_push_cmd=("${docker_cmd[@]}")
fi

This comment has been minimized.

@thockin

thockin Jan 17, 2018

Member

Once staging becomes a default, we only need the -- part, right?

@dekkagaijin

This comment has been minimized.

Contributor

dekkagaijin commented Jan 16, 2018

Pushes should now be made to staging-k8s.gcr.io. k8s.gcr.io will be turned into a read-only alias for the local replicas (i.e. (asia|eu|us)-k8s.gcr.io).

@thockin

This comment has been minimized.

Member

thockin commented Jan 19, 2018

/retest

@thockin

This comment has been minimized.

Member

thockin commented Jan 19, 2018

Un-holding this. I think it works now. Let's see what tests say. PTAL

@thockin

This comment has been minimized.

Member

thockin commented Jan 19, 2018

/retest

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 7, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 7, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@cblecker

This comment has been minimized.

Member

cblecker commented Feb 7, 2018

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm label Feb 7, 2018

@@ -221,7 +221,7 @@ function create-and-upload-hollow-node-image {
function create-and-upload-hollow-node-image-bazel {
RETRIES=3
for attempt in $(seq 1 ${RETRIES}); do
if ! bazel run //cluster/images/kubemark:push --define PROJECT="${PROJECT}"; then
if ! bazel run //cluster/images/kubemark:push --define REGISTRY="${FULL_REGISTRY}"; then

This comment has been minimized.

@dekkagaijin

dekkagaijin Feb 7, 2018

Contributor

This works with the current implementation of docker_build, since it's just concatenating the strings under the covers, but it would be good to be able to be able to assign $PROJECT/ as a base repository for kubemark in the BUILD target, in case that ever changes in the future.

This comment has been minimized.

@thockin

thockin Feb 7, 2018

Member

? PROJECT is a faux construct, AFAICT. gcr.io/google_containers is the registry. If you want something else, specify the whole thing.

This comment has been minimized.

@dekkagaijin

dekkagaijin Feb 7, 2018

Contributor

In the Docker sense, 'registry' == hostname[:port] and 'repository' == path of the URLesque image name: https://docs.docker.com/registry/spec/api/#overview
I'm worried that the a future implementation of docker_push might make use of that assumption.

Suggestions added in the relevant places...

registry = "gcr.io",
repository = "$(PROJECT)/kubemark",
registry = "$(REGISTRY)",
repository = "kubemark",

This comment has been minimized.

@cblecker

cblecker Feb 7, 2018

Member

@ixdy @BenTheElder Does this look okay from the bazel side?

This comment has been minimized.

@BenTheElder

BenTheElder Feb 7, 2018

Member

This looks like valid bazel, seems fine to me.
Edit: typically repository is foo/bar but it looks like this has already been discussed so.

@thockin

This comment has been minimized.

Member

thockin commented Feb 7, 2018

/retest

@BenTheElder

This comment has been minimized.

Member

BenTheElder commented Feb 7, 2018

/retest
this looks like a known flake in the go tests, @mikedanese did your timeout bump go in?

@cblecker

This comment has been minimized.

Member

cblecker commented Feb 7, 2018

@thockin If the test failure is a flake, you should be good to squash

@BenTheElder

This comment has been minimized.

Member

BenTheElder commented Feb 7, 2018

All green

registry = "gcr.io",
repository = "$(PROJECT)/kubemark",
registry = "$(REGISTRY)",
repository = "kubemark",

This comment has been minimized.

@dekkagaijin

dekkagaijin Feb 7, 2018

Contributor
registry = "$(REGISTRY)",
repository = "$(REPO_PATH_BASE)kubemark",
echo "Copying kubemark binary to ${MAKE_DIR}"
cp "${KUBEMARK_BIN}" "${MAKE_DIR}"
CURR_DIR=`pwd`
cd "${MAKE_DIR}"
RETRIES=3
for attempt in $(seq 1 ${RETRIES}); do
if ! REGISTRY="${CONTAINER_REGISTRY}" PROJECT="${PROJECT}" make "${KUBEMARK_IMAGE_MAKE_TARGET}"; then
if ! REGISTRY="${FULL_REGISTRY}" make "${KUBEMARK_IMAGE_MAKE_TARGET}"; then

This comment has been minimized.

@dekkagaijin

dekkagaijin Feb 7, 2018

Contributor

if ! REGISTRY="${REGISTRY}" REPO_PATH_BASE="${REPO_PATH_BASE}" make "${KUBEMARK_IMAGE_MAKE_TARGET}"; then

else
FULL_REGISTRY=staging-k8s.gcr.io
fi

This comment has been minimized.

@dekkagaijin

dekkagaijin Feb 7, 2018

Contributor
# TODO: Simplify. The caller should have to pass a full registry or nothing.
if [[ -n "${CONTAINER_REGISTRY}" ]]; then
  REGISTRY="${CONTAINER_REGISTRY}"
else
  REGISTRY=staging-k8s.gcr.io
fi
if [[ -n "${PROJECT}" ]]; then
 REPO_PATH_BASE="${PROJECT}/"
fi
@@ -221,7 +221,7 @@ function create-and-upload-hollow-node-image {
function create-and-upload-hollow-node-image-bazel {
RETRIES=3
for attempt in $(seq 1 ${RETRIES}); do
if ! bazel run //cluster/images/kubemark:push --define PROJECT="${PROJECT}"; then
if ! bazel run //cluster/images/kubemark:push --define REGISTRY="${FULL_REGISTRY}"; then

This comment has been minimized.

@dekkagaijin

dekkagaijin Feb 7, 2018

Contributor

if ! bazel run //cluster/images/kubemark:push --define REGISTRY="${REGISTRY}" --define REPO_PATH_BASE ="${REPO_PATH_BASE}"; then

This comment has been minimized.

@thockin

thockin Feb 8, 2018

Member

I prefer it to be simpler - one variable.

I'll leave refactoring of this to the team that owns it ( @shyamjvs and @wojtek-t can overrule if needed)

Switch to k8s.gcr.io vanity domain
This is the 2nd attempt.  The previous was reverted while we figured out
the regional mirrors (oops).

New plan: k8s.gcr.io is a read-only facade that auto-detects your source
region (us, eu, or asia for now) and pulls from the closest.  To publish
an image, push k8s-staging.gcr.io and it will be synced to the regionals
automatically (similar to today).  For now the staging is an alias to
gcr.io/google_containers (the legacy URL).

When we move off of google-owned projects (working on it), then we just
do a one-time sync, and change the google-internal config, and nobody
outside should notice.

We can, in parallel, change the auto-sync into a manual sync - send a PR
to "promote" something from staging, and a bot activates it.  Nice and
visible, easy to keep track of.
@thockin

This comment has been minimized.

Member

thockin commented Feb 8, 2018

squashed.

@cblecker

This comment has been minimized.

Member

cblecker commented Feb 8, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 8, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 8, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, thockin

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 OWNERS Files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 8, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 8, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 8, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 8, 2018

@thockin: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gke 3586986 link /test pull-kubernetes-e2e-gke
pull-kubernetes-unit 3586986 link /test pull-kubernetes-unit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 8, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit fb340a4 into kubernetes:master Feb 8, 2018

10 of 13 checks passed

pull-kubernetes-e2e-gke Job failed.
Details
pull-kubernetes-unit Job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-unit
Details
cla/linuxfoundation thockin 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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@shyamjvs

This comment has been minimized.

Member

shyamjvs commented Feb 8, 2018

Oops.. seems like I failed to stop it on time. IIUC this would create problems with our kubemark setup. See point 3 on #59567.

@shyamjvs

This comment has been minimized.

Member

shyamjvs commented Feb 8, 2018

Sorry, I think I misunderstood the change. You seem to define a FULL_REGISTRY that composes REGISTRY and PROJECT, and those value are taken from here which luckily also continues to take the same previous values here.

I'll however send a PR to unify these values into one single source of truth to avoid bugs in future.

@thockin

This comment has been minimized.

Member

thockin commented Feb 9, 2018

Thanks Shyam - Composing REGISTRY and PROJECT makes assumptions that no longer hold :)

@dekkagaijin

This comment has been minimized.

Contributor

dekkagaijin commented Feb 9, 2018

@shyamjvs would be good to split on the first '/' of the new single source of truth, when passing values to bazel run //cluster/images/kubemark:push in test/kubemark/start-kubemark.sh:
https://github.com/kubernetes/kubernetes/blob/master/cluster/images/kubemark/BUILD#L12
https://github.com/bazelbuild/rules_docker#container_push

deiwin added a commit to salemove/docker-fluentd that referenced this pull request May 8, 2018

Update base image to debian-base
The ubuntu-slim image has been deprecated and documentation suggests using
debian-base instead.

The new image contains a [`clean-install` script][0] which already takes care
of clearing the cache and running `apt-get clean` etc. So we don't have to do
that here.

`k8s.gcr.io` is an alias for `gcr.io/google_containers`, optimized for
different regions. According [to the PR][1] that introduced the alias, this the
suggested name to use.

[1]: kubernetes/kubernetes#57824
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment