Add manifest support for release images #516

mkumatag · 2018-02-10T02:31:20Z

This will enable pushing manifest image for release images

Refer #248 for more information.

mkumatag · 2018-02-10T02:31:35Z

/cc @luxas @ixdy

luxas

LGTM I guess.
@ixdy @jdumars @david-mcmahon @calebamiles

luxas · 2018-02-11T12:22:06Z

anago

+  # Check for manifest-tool, if not install it
+  if [[ ! -f $(which manifest-tool) ]]; then
+    MANIFEST_TOOL_PATH=${MANIFEST_TOOL_PATH:-$(mktemp -d -t manifest)}
+    PATH=$MANIFEST_TOOL_PATH:$PATH


@ixdy @david-mcmahon Where do you want to put this? Globally in e.g. /usr/local/bin if not present or in temp or whatever?

An alternative would be to use download the latest docker 18.02 client binary and use that for "official" pushing

I'd prefer this to instruct the user to Do The Right Thing and install it somewhere permanent.

+1 to David's comment

I've already included this tool in Dockerfile.k8s-cloud-builder file and will be used if we use cloud-builder. Above flow is only for people who are trying it out locally. Having said that I'll add an instruction to install the tool and rerun the script if we don't find a tool in the path.

@david-mcmahon @jdumars @luxas @ixdy Does is it make sense?

Yes, just look at the way the other prerequisites check and notify on dependencies and follow suit. There is still local usage of the tooling so we want to get that right.

@david-mcmahon @jdumars fixed it, PTAL latest commit. -Thanks

luxas · 2018-02-11T12:22:52Z

lib/releaselib.sh

@@ -927,8 +929,8 @@ release::docker::release () {

    # 'gcloud docker' gives lots of internal_failure's so add retries to
    # all of the invocations
-    for arch in "${KUBE_SERVER_PLATFORMS[@]##*/}"; do
-      for binary in "${binaries[@]}"; do
+    for binary in "${binaries[@]}"; do


Why reverse the loop flow here?

This reverse will help me running manifest in outer loop for every binary.

This branch of the if/else is defunct starting with kubernetes 1.8 (kubernetes/kubernetes#47939). You should probably add this new logic to the release::docker::release_from_tarfiles function instead.

mkumatag · 2018-02-11T15:05:52Z

I tried running the build anago but faced permission issues, can someone please help me mock the release and make sure it is creating manifest images!

jdumars · 2018-02-13T01:30:53Z

I am not sure this is possible without certain Google privs. @calebamiles might be able to verify that.

ixdy · 2018-02-14T00:25:40Z

lib/releaselib.sh

@@ -910,6 +910,8 @@ release::docker::release () {
    "kube-proxy"
    "hyperkube"
  )
+  # ml_platforms should be in the os1/arch1,os2/arch2,os3/arch3 form


what does ml stand for? I keep reading "machine learning", which I'm sure is wrong.

ixdy · 2018-02-14T00:31:08Z

lib/releaselib.sh

@@ -910,6 +910,8 @@ release::docker::release () {
    "kube-proxy"
    "hyperkube"
  )
+  # ml_platforms should be in the os1/arch1,os2/arch2,os3/arch3 form
+  local -r ml_platforms= $(echo "${KUBE_SERVER_PLATFORMS[@]}" | sed "s/ /,/g")


instead of using sed, you could do something like

local -r platforms=$(IFS=, ; echo "${KUBE_SERVER_PLATFORMS[*]}")

(I think)

mkumatag · 2018-02-16T12:09:39Z

@ixdy fixed all your review comments, can you PTAL?

@calebamiles can you please invoke mock release see how are things?

mkumatag · 2018-02-21T18:02:02Z

ping @ixdy @calebamiles

david-mcmahon · 2018-02-23T20:46:03Z

anago

+  if [[ ! -f $(which manifest-tool) ]]; then
+    logecho -r "$FAILED"
+    logecho "No manifest-tool found in the path. Please install it from" \
+            "https://github.com/estesp/manifest-tool/releases and set the path variable"


...and ensure it is on the PATH.

@david-mcmahon fixed it, PTAL.

BenTheElder · 2018-03-02T05:07:52Z

/cc

david-mcmahon · 2018-03-06T21:00:27Z

Based on the *UPDATE (Feb 2018)" item in the https://github.com/estesp/manifest-tool README, should we shift here to a pure docker solution to this?

mkumatag · 2018-03-07T04:03:21Z

docker solution got merged via docker/cli#138 which will be in v18.02.0-ce release, so that is the reason I was using this tool.

BenTheElder · 2018-03-07T06:31:43Z

Kubernetes isn't even on 17.x last I checked. Might be worth using something else for a while until people are ready to use such a recent docker version?

david-mcmahon · 2018-03-07T22:48:15Z

Giving this a try and getting:

$ manifest-tool push from-args --platforms linux/amd64,linux/arm,linux/arm64,linux/ppc64le,linux/s390x --template gcr.io/kubernetes-release-test/kube-aggregator-ARCH:v1.11.0-alpha.1 --target gcr.io/kubernetes-release-test/kube-aggregator:v1.11.0-alpha.1
FATA[0004] Inspect of image "gcr.io/kubernetes-release-test/kube-aggregator-ppc64le:v1.11.0-alpha.1" failed with error: denied: Permission denied for "v1.11.0-alpha.1" from request "/v2/kubernetes-release-test/kube-aggregator-ppc64le/manifests/v1.11.0-alpha.1".

dims · 2018-08-24T20:07:09Z

@deitch can you please log issues for what's broken for multi-arch clusters separately from this one? i'd like to track it

deitch · 2018-08-24T20:10:12Z

I would be happy to, but I need to do it methodically. Forgive me if it takes a week or two for me to find a day to sit down and document. :-)

dims · 2018-08-24T20:12:48Z

@deitch Ack no worries! :)

dims · 2018-08-27T20:04:35Z

@mkumatag Don't we have to enable the experimental support on the CLI and docker daemon in this script?

jdumars · 2018-08-27T21:59:52Z

Is this still "priority/critical-urgent"?

dims · 2018-08-28T01:12:39Z

@jdumars we'd like to sort this out for 1.12 release. the earlier we can try this out the better our chances of ensuring we have the right images when we cut the release.

mkumatag · 2018-08-28T14:47:02Z

@mkumatag Don't we have to enable the experimental support on the CLI and docker daemon in this script?

Yes, experimental support should be enabled only in the cli and not in the daemon but not sure whether we should enable part of script!

BenTheElder · 2018-08-28T16:20:35Z

Yes, experimental support should be enabled only in the cli and not in the daemon but not sure whether we should enable part of script!

Can we detect that it is not enabled then print out instructions to enable it? We probably shouldn't muck with docker config in the scripts.

ixdy · 2018-08-28T18:38:28Z

Maybe check that docker version -f '{{.Client.Experimental}}' equals true?

$ docker version -f '{{.Client.Experimental}}'
true

mkumatag · 2018-08-29T08:30:15Z

Maybe check that docker version -f '{{.Client.Experimental}}' equals true?

$ docker version -f '{{.Client.Experimental}}'
true

added the check, ptal to the latest commit.

dims · 2018-08-29T11:01:36Z

anago

+  fi
+  logecho -r "$OK"
+
+  # TODO: Remove this section once docker manifest command promted


promted -> promoted.

will do that..

dims · 2018-08-29T11:01:49Z

anago

+  logecho -n "Checking Docker CLI Experimental status: "
+  cli_experimental=$(docker version --format '{{.Client.Experimental}}' | cut -d"-" -f1)
+  if [[ "${cli_experimental}" == "false" ]]; then
+    echo "dsdmsdsmdsdsd :>>>>${cli_experimental}<<<<<"


remove debug statement please

oops.. :P my bad...

dims · 2018-08-29T11:04:39Z

Tiny nit @mkumatag

mkumatag · 2018-08-29T13:03:53Z

Tiny nit @mkumatag

fixed them..

dims · 2018-08-29T13:12:17Z

/lgtm

BenTheElder · 2018-08-29T20:30:22Z

Generally LGTM, we should get this tested out. 😬 Thanks for keeping after this!

jdumars · 2018-09-02T20:05:50Z

I'm basically LGTM as well, but would like @tpepper to be involved as well.

tpepper · 2018-09-04T17:21:03Z

/milestone v1.12

tpepper · 2018-09-04T18:01:36Z

I'm good with this commit and LGTM, but would like a @calebamiles review here also.

calebamiles · 2018-09-04T18:53:31Z

/approve

k8s-ci-robot · 2018-09-04T18:53:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: calebamiles, dims, mkumatag

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:

~~OWNERS~~ [calebamiles]

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 10, 2018

k8s-ci-robot requested review from ixdy and luxas February 10, 2018 02:31

This was referenced Feb 10, 2018

Push manifest lists when releasing docker images #248

Closed

Manifest list for kubernetes images kubernetes/kubernetes#57869

Closed

luxas reviewed Feb 11, 2018

View reviewed changes

mkumatag changed the title ~~[WIP] Add manifest support for release images~~ Add manifest support for release images Feb 11, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2018

ixdy reviewed Feb 14, 2018

View reviewed changes

mkumatag force-pushed the manifest branch from f2abca7 to c9f99aa Compare February 16, 2018 12:07

mkumatag force-pushed the manifest branch 3 times, most recently from 00dfcc0 to ca87d64 Compare February 23, 2018 05:18

david-mcmahon reviewed Feb 23, 2018

View reviewed changes

mkumatag force-pushed the manifest branch from ca87d64 to d903ce6 Compare February 24, 2018 01:32

david-mcmahon self-assigned this Mar 1, 2018

k8s-ci-robot requested a review from BenTheElder March 2, 2018 05:07

Add manifest support for release images

a70044a

mkumatag force-pushed the manifest branch from 2042fd4 to a70044a Compare August 29, 2018 00:54

dims reviewed Aug 29, 2018

View reviewed changes

Add docker checks

59dc8ed

mkumatag force-pushed the manifest branch from c154719 to 59dc8ed Compare August 29, 2018 13:03

k8s-ci-robot assigned dims Aug 29, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2018

k8s-ci-robot merged commit ff17b75 into kubernetes:master Sep 4, 2018

dims mentioned this pull request Sep 5, 2018

Support for manifests using docker experimental cli commands #622

Merged

Add manifest support for release images #516

Add manifest support for release images #516

Conversation

mkumatag commented Feb 10, 2018 • edited Loading

mkumatag commented Feb 10, 2018

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkumatag commented Feb 11, 2018

jdumars commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkumatag commented Feb 16, 2018

mkumatag commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Mar 2, 2018

david-mcmahon commented Mar 6, 2018

mkumatag commented Mar 7, 2018

BenTheElder commented Mar 7, 2018

david-mcmahon commented Mar 7, 2018

dims commented Aug 24, 2018

deitch commented Aug 24, 2018

dims commented Aug 24, 2018

dims commented Aug 27, 2018

jdumars commented Aug 27, 2018

dims commented Aug 28, 2018

mkumatag commented Aug 28, 2018

BenTheElder commented Aug 28, 2018

ixdy commented Aug 28, 2018

mkumatag commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Aug 29, 2018

mkumatag commented Aug 29, 2018

dims commented Aug 29, 2018

BenTheElder commented Aug 29, 2018

jdumars commented Sep 2, 2018

tpepper commented Sep 4, 2018

tpepper commented Sep 4, 2018

calebamiles commented Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

mkumatag commented Feb 10, 2018 •

edited

Loading