kubeadm: Implement support for using images from CI builds #49119

kad · 2017-07-18T14:55:59Z

What this PR does / why we need it: Implements support for CI images in kubeadm

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

Special notes for your reviewer:

Release note:

- kubeadm now supports "ci/latest-1.8" or "ci-cross/latest-1.8" and similar labels.

k8s-ci-robot · 2017-07-18T14:56:06Z

Hi @kad. Thanks for your PR.

I'm waiting for a kubernetes 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.

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

kad · 2017-07-18T22:16:37Z

@luxas if you can add "ok-to-test" please

luxas · 2017-07-19T14:15:06Z

/ok-to-test

kad · 2017-07-19T16:41:53Z

those failures seems to be now on infra side (gcs push failures)

luxas

I think this can be done in way less code, without touching the API struct. In the special case of a ci/* version, we'll just use (the constant) gcr.io/kubernetes-ci-images repo in GetCoreImage()

kad · 2017-07-19T20:14:10Z

We can't. gcr.io/kubernetes-ci-images contains only subset of images (the core ones).

luxas · 2017-07-19T20:45:16Z

@kad I didn't propose to touch cfg.ImageRepository. Instead, just pass useCIRepository or the like to GetCoreImage(). You can add a new field (e.g. cfg.UseCIRepository) on the internal version of the API for this purpose

kad · 2017-07-19T21:01:47Z

Can you a bit elaborate about "internal version of the API" ? I'll look at this possibilities.
Second thing, that way location of images would be spread across whole source code and strictly hardcoded to gcr.io, which might be not the best in long run if people are interested to use images from local registries in isolated networks.
This current state of the code, it is possible to easily redefine via config file location of core/non-core images separately and have a custom registry for CI images as well.
Actually it can even enable use case of automatic testing via kubeadm support images like etcd or kube-dns, where tests can be deploying latest stable k8s core + via custom NonCoreImageRepository rest of images.

luxas · 2017-07-20T09:58:16Z

I'm not planning to expand the ImageRepository functionality more than we do now, instead it will eventually be possible to set all images separately via the config file. We should not over-engineer this, but keep it as simple as we can.

Hardcoding the ci/ helper to gcr.io/kubernetes-ci-images is very fine, since it's a helper. You can indeed prepull these images if you want. If you want to use a local registry, just point ImageRepository to that and set the KubernetesVersion yourself and voila!

There are two sub-API groups for the kubeadm API group, an internal version and an external
Internal: https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/apis/kubeadm/types.go
External: https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/apis/kubeadm/v1alpha1/types.go

The external one follows a more strict, versioned scheme, but the internal version may change and have more parameters used internally if applicable. I think having an UseCIImages bool or something in the internal version would make sense for this task. Then GetCoreImage would know whether to use the special CI images repo or not.

kad · 2017-07-20T10:19:12Z

thing is, that this hardcoding in case of bool will be at least in two places (maybe more). e.g. accroding to your request kube-dns images are handled not via GetCoreImage().
Regarding internal APIs, thx, I'll check what can be done better.

kad · 2017-07-20T12:57:40Z

@luxas updated to use only internal APIs and removed ability to configure additional image repositories.

luxas

Looks much better 👍, but still needs to be modified a little to work properly. The biggest point here is that if the user specifies the ci/ prefix on the CLI, we should lookup the tag from dl.k8s.io/ci-cross.

luxas · 2017-07-20T13:04:30Z

cmd/kubeadm/app/util/config/masterconfig.go

 	if err != nil {
 		return err
 	}
 	cfg.KubernetesVersion = ver

+	// Requested version is automatic CI build, thus use KubernetesCI Image Repository for core images
+	if useCI {


Why not just move this up to check strings.HasPrefix(cfg.KubernetesVersion, "ci/") and set the CI repo on that condition? KubernetesValidateVersion will be much simpler then...

luxas · 2017-07-20T13:04:41Z

cmd/kubeadm/app/util/config/masterconfig.go

 	if err != nil {
 		return err
 	}
 	cfg.KubernetesVersion = ver

+	// Requested version is automatic CI build, thus use KubernetesCI Image Repository for core images
+	if useCI {
+		cfg.CIImageRepository = "gcr.io/kubernetes-ci-images"


probably want a constant for this

luxas · 2017-07-20T13:08:19Z

cmd/kubeadm/app/util/version.go

 	kubeReleaseRegex      = regexp.MustCompile(`^v?(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$`)
 	kubeReleaseLabelRegex = regexp.MustCompile(`^[[:lower:]]+(-[-\w_\.]+)?$`)
+	kubeBucketPrefixes    = regexp.MustCompile(`^((release|ci|ci-cross)/)?([-\w_\.+]+)$`)


We will only allow release and ci. What is indeed a little bit confusing is that we're gonna use artifacts from ci-cross under the hood (kubeadm user uses ci/latest => will actually fetch ci-cross/latest.txt for the version to use)

This is due to that the normal ci job does not push images

luxas · 2017-07-20T13:09:35Z

cmd/kubeadm/app/util/version.go

+		if url == "" {
+			url = kubeReleaseBucketURL + "/release"
+		}
+		vurl := fmt.Sprintf("%s/%s.txt", url, version)


vurl doesn't say much to me

luxas · 2017-07-20T13:10:57Z

cmd/kubeadm/app/util/version.go

+// release/v1.7.1 -- same as above.
+// stable-1.7 or release/stable-1.7 -- labeled official releases
+// ci/v1.8.0-alpha.1.123+012345678 -- CI explicit build version
+// ci/latest-1.8 or ci-cross/latest-1.8 -- CI labeled builds


I don't think we should confuse the user with ci-cross although we fetch the images from there under the hood.
Let's not support for ci-cross/ at the moment.

luxas · 2017-07-20T13:26:47Z

cmd/kubeadm/app/util/version.go

+// ci/latest-1.8 or ci-cross/latest-1.8 -- CI labeled builds
+//
+// Returns: version, build is release (false) or CI (true), error.
+func KubernetesValidateVersion(version string) (string, bool, error) {


I find the relationship between KubernetesValidateVersion and KubernetesReleaseVersion kind of strange.
What I think we want is this:

Have a function that takes cfg.KubernetesVersion (may be a label) and just return the resulting "real" version we can use. Basically what KubernetesReleaseVersion is today.

This would do the following:
If it already is a valid version, return
If it matches (release/)?(stable|latest)(-1.[0-9]{1})?, unconditionally trim the release/ prefix and fetch real version from dl.k8s.io/release/%s.txt and return
If it matches ci/latest(-1.[0-9]{1})?, unconditionally trim the ci/ prefix and fetch the real version from dl.k8s.io/ci-cross/%s.txt and return

That will do what we want. Feel free to create private helper functions for that main function.

kad · 2017-07-20T14:04:43Z

Regarding ci vs ci-cross prefix: there are two different latest.txt files in those buckets. It supports both. Having those tricks to substitute one with another invisible for user sounds like over
engineering to me. Check get-kube.sh, based on what user asked, it will fetch corresponding version file.

kad · 2017-07-20T14:16:01Z

@luxas any hint on how to fix those unit test failures ? they are both related to declare only internally used variable in MasterConfiguration that supposed to be not exposed to versioned external API and not to config ?

luxas · 2017-07-20T14:17:36Z

Regarding ci vs ci-cross prefix: there are two different latest.txt files in those buckets. It supports both. Having those tricks to substitute one with another invisible for user sounds like over
engineering to me. Check get-kube.sh, based on what user asked, it will fetch corresponding version file.

That's not how this works. No images are pushed from the job that uploads to ci/latest.txt. Only from the ci-cross job that uploads to ci-cross/latest.txt.

kad · 2017-08-18T14:08:26Z

Updated.

luxas · 2017-08-18T15:01:19Z

/lgtm
/retest

Thanks!

k8s-github-robot · 2017-08-18T15:02:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kad, luxas

Associated issue: 337

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

luxas · 2017-08-18T16:01:28Z

/retest

…

On 18 Aug 2017, at 18:53, k8s-ci-robot ***@***.***> wrote: @kad: The following test failed, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-gce-etcd3 9f3ac32 link /test pull-kubernetes-e2e-gce-etcd3 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. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

luxas · 2017-08-18T17:57:05Z

/retest

…

On 18 Aug 2017, at 20:18, k8s-ci-robot ***@***.***> wrote: @kad: The following test failed, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-gce-etcd3 9f3ac32 link /test pull-kubernetes-e2e-gce-etcd3 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. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

fejta-bot · 2017-08-19T03:05:29Z

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

Review the full test history for this PR.

luxas · 2017-08-19T06:05:03Z

/retest

…

On 19 Aug 2017, at 06:26, k8s-ci-robot ***@***.***> wrote: @kad: The following test failed, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-gce-etcd3 9f3ac32 link /test pull-kubernetes-e2e-gce-etcd3 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. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

fejta-bot · 2017-08-19T09:02:28Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-19T12:32:28Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-19T15:41:29Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-19T18:50:28Z

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

Review the full test history for this PR.

luxas · 2017-08-19T21:27:12Z

/retest

fejta-bot · 2017-08-20T00:26:27Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-20T02:53:27Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-20T06:02:27Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-20T09:32:27Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-20T13:44:27Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-20T17:35:27Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-20T22:08:27Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-21T02:41:27Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-21T06:53:28Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-21T11:47:27Z

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

Review the full test history for this PR.

luxas · 2017-08-21T15:02:49Z

/retest

k8s-github-robot · 2017-08-21T21:30:02Z

Automatic merge from submit-queue (batch tested with PRs 50693, 50831, 47506, 49119, 50871)

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 18, 2017

k8s-github-robot assigned mikedanese and dmmcquay Jul 18, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 18, 2017

kad force-pushed the n-addons-repo branch from 6a0afd5 to 17f3234 Compare July 18, 2017 22:15

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 18, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 19, 2017

kad force-pushed the n-addons-repo branch 2 times, most recently from 2152734 to 40b7fe6 Compare July 19, 2017 15:50

luxas suggested changes Jul 19, 2017

View reviewed changes

kad force-pushed the n-addons-repo branch from 40b7fe6 to 2d61f79 Compare July 20, 2017 12:55

kad force-pushed the n-addons-repo branch from 2d61f79 to e854081 Compare July 20, 2017 13:00

luxas reviewed Jul 20, 2017

View reviewed changes

luxas self-assigned this Aug 2, 2017

kad force-pushed the n-addons-repo branch from 6ce42d9 to 9f3ac32 Compare August 18, 2017 14:03

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2017

k8s-github-robot merged commit f40d074 into kubernetes:master Aug 21, 2017

luxas mentioned this pull request Aug 24, 2017

Use ci/latest instead of latest in the kubeadm e2e job kubernetes/test-infra#4176

Merged

jessicaochen mentioned this pull request Oct 18, 2017

[WIP] Automated cherry pick of #48053 #49119 #54173

Closed

kubeadm: Implement support for using images from CI builds #49119

kubeadm: Implement support for using images from CI builds #49119

Conversation

kad commented Jul 18, 2017 • edited Loading

k8s-ci-robot commented Jul 18, 2017

kad commented Jul 18, 2017

luxas commented Jul 19, 2017

kad commented Jul 19, 2017

luxas left a comment

Choose a reason for hiding this comment

kad commented Jul 19, 2017

luxas commented Jul 19, 2017

kad commented Jul 19, 2017 • edited Loading

luxas commented Jul 20, 2017

kad commented Jul 20, 2017

kad commented Jul 20, 2017

luxas left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kad commented Jul 20, 2017

kad commented Jul 20, 2017

luxas commented Jul 20, 2017

kad commented Aug 18, 2017

luxas commented Aug 18, 2017

k8s-github-robot commented Aug 18, 2017

luxas commented Aug 18, 2017 via email

luxas commented Aug 18, 2017 via email

fejta-bot commented Aug 19, 2017

luxas commented Aug 19, 2017 via email

fejta-bot commented Aug 19, 2017

fejta-bot commented Aug 19, 2017

fejta-bot commented Aug 19, 2017

fejta-bot commented Aug 19, 2017

luxas commented Aug 19, 2017

fejta-bot commented Aug 20, 2017

fejta-bot commented Aug 20, 2017

fejta-bot commented Aug 20, 2017

fejta-bot commented Aug 20, 2017

fejta-bot commented Aug 20, 2017

fejta-bot commented Aug 20, 2017

fejta-bot commented Aug 20, 2017

fejta-bot commented Aug 21, 2017

fejta-bot commented Aug 21, 2017

fejta-bot commented Aug 21, 2017

luxas commented Aug 21, 2017

k8s-github-robot commented Aug 21, 2017

kad commented Jul 18, 2017 •

edited

Loading

kad commented Jul 19, 2017 •

edited

Loading

luxas left a comment •

edited

Loading