Adding the core migrator that rewrites objects of a single resource type #3

caesarxuchao · 2018-09-14T22:34:10Z

The core migrator lists a chunk of objects, GET&PUT each of them, then continue with the next chunk.

The code more or less resembles the oc admin migrate storage code. A small improvement is that the migrator uses the inconsistent continue token sent by the apiserver in case the original list is compacted. So the migrator doesn't have to finish migrating a resource type in the etcd compaction window (5 mins by default).

caesarxuchao · 2018-09-21T00:06:26Z

/assign @yliaog for initial review

krzyzacy · 2018-09-21T16:38:56Z

/assign @yliaog

k8s-ci-robot · 2018-09-21T16:38:57Z

@krzyzacy: GitHub didn't allow me to assign the following users: yliaog.

Note that only kubernetes-sigs members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @yliaog

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.

yliaog · 2018-09-25T20:13:36Z

/assign @yliaog

deads2k · 2018-09-25T20:22:36Z

I see the oc migrate command switched to using unstructured client. It saved a GET request per object. But the unstructured client only supports json, which is less efficient than proto. I'm not sure if it's a net-win, so I didn't incorporate the change.

It wasn't so much about saving a GET as it was about gaining reliability, readability, and being fully schema agnostic. By using the dynamic client, we're certain that our conversion will work for any type regardless of whether it's known. I'll also note that if we had already split client-go into k8s.io/api specific and generic, we'd prefer to use the dynamic client.

This is an infrequently run command, proto versus json isn't hugely important.

Now after having written all this, I've realized that the other thing is that if you wire up something to accept proto, you won't be able to easily get namespaces and names and if you aren't carefully it will decode and strip unknown fields.

yliaog · 2018-09-25T21:07:27Z

some import issue:
https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kube-storage-version-migrator/3/pull-kube-storage-version-migrator-test/2

caesarxuchao · 2018-09-25T21:10:58Z

@yliaog I intentionally removed all the imports to make review easier.

yliaog · 2018-09-25T21:13:18Z

pkg/migrator/core.go

+	// creating a working serializer.
+	// We don't use dynamic client because it only supports json, which
+	// increases the memory usage of the apiserver.  When we support
+	// migrating CRD or aggregated apis, we need to plumb in a dynamic


s/plumb/plug/ ?

yliaog · 2018-09-25T21:16:21Z

pkg/migrator/core.go

+		namespaceScoped: namespaceScoped,
+		client:          client,
+		progress:        &dummyProgress{},
+		concurrency:     5,


why hard code it here? if no plan to change it, better make it a const

caesarxuchao · 2018-09-25T22:10:25Z

it was about gaining reliability, readability, and being fully schema agnostic

Agree. With the current typed clients I'll have to worry about version skews.

@deads2k, I'm a little bit concerned if the roundtrip of data->unstructured->data is loss less. Are we 100% confident? If the dynamic client supports GetRaw() it's more reassuring.

yliaog · 2018-09-25T21:33:55Z

pkg/migrator/errors.go

+		return ErrNotRetriable{err}
+	case errors.IsConflict(err):
+		return ErrRetriable{err}
+	case errors.IsServerTimeout(err):


what about TooManyRequests? seems to be a retriable

yliaog · 2018-09-25T21:48:37Z

pkg/migrator/core.go

+			if err == nil {
+				break
+			}
+			if canRetry(err) {


no backoff?

Added backoff if the error suggests "retry after" seconds. General backoff is handled by the RESTClient's built-in rate limiter.

yliaog · 2018-09-25T21:58:39Z

pkg/migrator/core.go

+	for {
+		list, listError := m.list(
+			&metav1.ListOptions{
+				Limit:    500,


all const better defined at the beginning of the package

deads2k · 2018-09-26T12:13:03Z

@deads2k, I'm a little bit concerned if the roundtrip of data->unstructured->data is loss less. Are we 100% confident?

Yes. We are very sure. It decodes to map[string]interface{} and goes out the same way.

yliaog · 2018-09-28T23:12:48Z

pkg/migrator/core.go

-	// TODO: The oc migrator tried to count if the update causes changes.
-	// To do so it needs to decode the PUT response and extract the
-	// resourceVersion.
+	return m.get(namespace, name)


so if there is a conflict, this will return what's stored without retry, but it is possible what's stored there is still the old version

This is a bug. I fixed the logic. I'll add unit test when the dynamic client is fixed.

yliaog · 2018-09-28T23:37:38Z

pkg/migrator/core_test.go

-func newPods(num int) []v1.Pod {
-	var list []v1.Pod
+func newPodList(num int) v1.PodList {
+	var pods []v1.Pod
 	for i := 0; i < num; i++ {
 		pod := newPod(fmt.Sprintf("pod%d", i), fmt.Sprintf("namespace%d", i))


is it worthwhile to test the nil or empty namespace case?

caesarxuchao · 2018-10-01T18:08:11Z

PTAL @yliaog. Thank you!

yliaog · 2018-10-01T21:22:40Z

pkg/migrator/core.go

+
+// try tries to migrate the single object by PUT. If the PUT fails due to
+// conflicts, the function requests next try to GET the new object. If the GET
+// failed, the function requests the next try to GET the new object.


If the GET failed, and the error is retriable, then it requests next try to GET the new object

Discussed offline. Updated.

yliaog · 2018-10-03T00:10:19Z

/lgtm

caesarxuchao · 2018-10-03T00:14:37Z

Thanks @yliaog.

@deads2k @lavalamp for this repository, do you want to review every changes or do you want to checkpoint from time to time?

…urce type

dep doesn't parse the Godeps.json in the dependencies, so this has potential problems. I tried glide as well, it doesn't work out-of-box, and it hasn't been updated since the end of 2017. I'll use dep as long as it works

caesarxuchao · 2018-10-11T17:41:48Z

Let's get this one merged. We can fix issues in later iterations.

@yliaog could you re-apply the lgtm? I only added the dependencies after you lgtm'ed it last time.

yliaog · 2018-10-11T17:42:54Z

/lgtm

caesarxuchao · 2018-10-11T17:48:30Z

@lavalamp @deads2k could you approve the change?

I corrected the name in the OWNER file in the last commit, so myself becomes an approver.

lavalamp · 2018-10-11T20:25:09Z

/approve

k8s-ci-robot · 2018-10-11T20:25:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, lavalamp

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~~ [lavalamp]

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

yliaog · 2018-10-11T21:06:42Z

/lgtm

k8s-ci-robot requested review from deads2k and lavalamp September 14, 2018 22:34

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 14, 2018

caesarxuchao force-pushed the init branch 2 times, most recently from 48f6c3e to bfff573 Compare September 18, 2018 00:38

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 18, 2018

caesarxuchao closed this Sep 19, 2018

caesarxuchao reopened this Sep 19, 2018

caesarxuchao force-pushed the init branch from bfff573 to e103128 Compare September 20, 2018 21:50

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 20, 2018

caesarxuchao changed the title ~~[WIP] add the core migrator that rewrites objects of a single resource type~~ Adding the core migrator that rewrites objects of a single resource type Sep 21, 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 Sep 21, 2018

k8s-ci-robot assigned yliaog Sep 25, 2018

yliaog reviewed Sep 25, 2018

View reviewed changes

nikhita mentioned this pull request Sep 25, 2018

Add template files #1

Merged

yliaog reviewed Sep 25, 2018

View reviewed changes

yliaog reviewed Sep 28, 2018

View reviewed changes

caesarxuchao force-pushed the init branch 4 times, most recently from 8a0db3c to 7c075cd Compare October 1, 2018 18:07

yliaog reviewed Oct 1, 2018

View reviewed changes

caesarxuchao force-pushed the init branch from 7c075cd to 8117599 Compare October 3, 2018 00:03

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

Chao Xu added 3 commits October 3, 2018 17:15

Add Makefile for setting up presubmits

369ff9b

adding the core migrator that is responsible to migrate a single reso…

ec53107

…urce type

Use dep to install dependencies.

07a63e3

dep doesn't parse the Godeps.json in the dependencies, so this has potential problems. I tried glide as well, it doesn't work out-of-box, and it hasn't been updated since the end of 2017. I'll use dep as long as it works

caesarxuchao force-pushed the init branch from 8117599 to 07a63e3 Compare October 4, 2018 00:16

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 4, 2018

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

fix name in OWNER file

0cd9134

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2018

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

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

k8s-ci-robot merged commit 8166c84 into kubernetes-sigs:master Oct 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the core migrator that rewrites objects of a single resource type #3

Adding the core migrator that rewrites objects of a single resource type #3

caesarxuchao commented Sep 14, 2018 •

edited

Loading

caesarxuchao commented Sep 21, 2018

krzyzacy commented Sep 21, 2018

k8s-ci-robot commented Sep 21, 2018

yliaog commented Sep 25, 2018

deads2k commented Sep 25, 2018

yliaog commented Sep 25, 2018

caesarxuchao commented Sep 25, 2018

yliaog Sep 25, 2018

yliaog Sep 25, 2018

caesarxuchao commented Sep 25, 2018

yliaog Sep 25, 2018

caesarxuchao Sep 26, 2018

yliaog Sep 25, 2018

caesarxuchao Sep 26, 2018

yliaog Sep 25, 2018

caesarxuchao Sep 26, 2018

deads2k commented Sep 26, 2018

yliaog Sep 28, 2018

caesarxuchao Oct 1, 2018

yliaog Sep 28, 2018

caesarxuchao Oct 1, 2018

caesarxuchao commented Oct 1, 2018

yliaog Oct 1, 2018

caesarxuchao Oct 3, 2018

yliaog commented Oct 3, 2018

caesarxuchao commented Oct 3, 2018

caesarxuchao commented Oct 11, 2018

yliaog commented Oct 11, 2018

caesarxuchao commented Oct 11, 2018

lavalamp commented Oct 11, 2018

k8s-ci-robot commented Oct 11, 2018

yliaog commented Oct 11, 2018

Adding the core migrator that rewrites objects of a single resource type #3

Adding the core migrator that rewrites objects of a single resource type #3

Conversation

caesarxuchao commented Sep 14, 2018 • edited Loading

caesarxuchao commented Sep 21, 2018

krzyzacy commented Sep 21, 2018

k8s-ci-robot commented Sep 21, 2018

yliaog commented Sep 25, 2018

deads2k commented Sep 25, 2018

yliaog commented Sep 25, 2018

caesarxuchao commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Sep 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 26, 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

caesarxuchao commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yliaog commented Oct 3, 2018

caesarxuchao commented Oct 3, 2018

caesarxuchao commented Oct 11, 2018

yliaog commented Oct 11, 2018

caesarxuchao commented Oct 11, 2018

lavalamp commented Oct 11, 2018

k8s-ci-robot commented Oct 11, 2018

yliaog commented Oct 11, 2018

caesarxuchao commented Sep 14, 2018 •

edited

Loading