Add a migration framework for mutable resources #9240

smarterclayton · 2016-06-09T04:12:45Z

Implement a migration framework and an image reference migrator:

oadm migrate image-references registry1.com/*=registry2.com/* --confirm
oadm migrate storage --confirm

Reuse resource builder to manage output. Handles images, image streams, secrets, and all the pod template resources.

@liggitt

smarterclayton · 2016-06-09T04:59:30Z

[test]

smarterclayton · 2016-06-10T04:38:23Z

This is done barring tests.

smarterclayton · 2016-06-13T13:46:02Z

@deads2k take a look at this

On Sat, Jun 11, 2016 at 11:35 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test ABORTED (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4750/)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9240 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p3maOlH0aOCPzvYUIQaEVns_5aHlks5qK35ygaJpZM4IxnZA
.

smarterclayton · 2016-06-14T02:40:48Z

This is now effectively complete for review. Tests for migration of storage need more work - still debating what the best approach is, but probably going to be taking an old db data dump from v1.0 and running migration twice on it and verifying it boots.

soltysh · 2016-06-15T11:11:18Z

/cc

deads2k · 2016-06-15T18:03:32Z

pkg/client/client.go

@@ -283,6 +284,13 @@ func New(c *restclient.Config) (*Client, error) {
 	return &Client{client}, nil
 }

+// DiscoveryClient returns a discovery client.
+func (c *Client) Discovery() discovery.DiscoveryInterface {
+	d := discovery.NewDiscoveryClient(c.RESTClient)


If we're using this, want to remove the cruft from pkg/client/discovery.go where we combine results from /api and /oapi "for the short-term/forever"?

Actually... maybe I just want to use that.

deads2k · 2016-06-15T18:21:50Z

Can you break out some of the prerequisite pulls to make this a little smaller to look at, I'm going cross eyed.

Assuming this the approach we want instead of one that streams through, it looks plausible.

soltysh · 2016-06-16T09:14:09Z

pkg/cmd/admin/migrate/images/images.go

+			kcmdutil.CheckErr(options.Complete(f, cmd, args))
+			kcmdutil.CheckErr(options.Validate())
+			if err := options.Run(); err != nil {
+				// TODO: move met to kcmdutil


I don't think it's needed. With the current implementation kcmutil.CheckErr should os.Exit(1) with that error, printing nothing (from looking into StandardErrorMessage from kcmdutil package).

Yeah, the rebase got my upstream fix.

On Thu, Jun 16, 2016 at 5:14 AM, Maciej Szulik notifications@github.com
wrote:

In pkg/cmd/admin/migrate/images/images.go
#9240 (comment):

In: in,

Out: out,

ErrOut: errout,

Include: []string{"imagestream", "image", "secrets"},

},

}

cmd := &cobra.Command{

Use: fmt.Sprintf("%s REGISTRY/NAME=REGISTRY/NAME [...]", name),

Short: "Update embedded Docker image references",

Long: internalMigrateImagesLong,

Example: fmt.Sprintf(internalMigrateImagesExample, fullName),

Run: func(cmd *cobra.Command, args []string) {

kcmdutil.CheckErr(options.Complete(f, cmd, args))

kcmdutil.CheckErr(options.Validate())

if err := options.Run(); err != nil {

// TODO: move met to kcmdutil

I don't think it's needed. With the current implementation
kcmutil.CheckErr should os.Exit(1) with that error, printing nothing
(from looking into StandardErrorMessage from kcmdutil package).

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9240/files/eb1eb8a1f37fe62714438a3a805545a98b82daec..053e3598622ec47ce57195dacc03aedc08e4489f#r67312076,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p3zxxuIxre02vI8IzwzfJNmdGNccks5qMRPkgaJpZM4IxnZA
.

smarterclayton · 2016-07-26T21:09:09Z

The flake in tests has been fixed (there was a bug in calling resource.Builder).

smarterclayton · 2016-07-27T18:40:43Z

Aaaany other comments?

soltysh · 2016-07-27T20:01:47Z

pkg/cmd/admin/migrate/images/imagerefs.go

+	return nil
+}
+
+func (o *MigrateImageReferenceOptions) Validate() error {


According to our conventions both Validate and Run should non-pointer methods.

That's strange, because it's not sufficient to defend against any problem
(mutation of sub resources or interfaces), so it's kind of arbitrary, and
goes against our general conventions of always passing pointers unless we
have a real reason not to.

On Wed, Jul 27, 2016 at 4:01 PM, Maciej Szulik notifications@github.com
wrote:

In pkg/cmd/admin/migrate/images/imagerefs.go
#9240 (comment):

o.ResourceOptions.SaveFn = o.save

if err := o.ResourceOptions.Complete(f, c); err != nil {

return err

}

osclient, _, err := f.Clients()

if err != nil {

return err

}

o.Client = osclient

return nil
+}

+func (o *MigrateImageReferenceOptions) Validate() error {

According to our conventions
https://github.com/kubernetes/kubernetes/blob/master/docs/devel/kubectl-conventions.md#command-implementation-conventions
both Validate and Run should non-pointer methods.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9240/files/1ddd7d050a21f14d07b8d4198df6e7eec9cf8475#r72512049,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p2L_GGWc9FhgS5T9vXhxerrvLGOSks5qZ7ktgaJpZM4IxnZA
.

The reasoning was, only Complete should modify the necessary options struct. The other two should only act upon it, without mutating it in any way. I've spoke with @Kargakis about it.

soltysh · 2016-07-27T20:16:38Z

pkg/cmd/admin/migrate/images/imagerefs.go

+	return changed
+}
+
+func updatePodTemplate(template *kapi.PodTemplateSpec, fn TransformImageFunc) bool {


This method looks like it's not used anywhere.

soltysh · 2016-07-27T20:21:42Z

Nits, mostly LGTM.

Implement a migration framework and an image reference migrator: oadm migrate image-references registry1.com/*=registry2.com/* --confirm Reuse resource builder to manage output

smarterclayton · 2016-07-27T20:37:03Z

nits fixed [merge]

smarterclayton · 2016-07-27T21:42:12Z

[test] #9959

On Wed, Jul 27, 2016 at 5:35 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7011/)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9240 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p5UJ0yGl33Fh7nK1HJgQF-Aqqx59ks5qZ88lgaJpZM4IxnZA
.

smarterclayton · 2016-07-28T02:21:16Z

[test] #9959

soltysh · 2016-07-28T08:27:08Z

Flake from #9203.

re-[merge]

soltysh · 2016-07-28T10:26:33Z

Flake from #10076.

re-[merge]

openshift-bot · 2016-07-28T11:50:32Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7056/) (Image: devenv-rhel7_4695)

soltysh · 2016-07-28T11:55:03Z

Flake #10080, what on earth is wrong with this PR.

re-[merge]

openshift-bot · 2016-07-28T11:55:43Z

Evaluated for origin merge up to f8aa83f

smarterclayton · 2016-07-28T12:23:13Z

#9203

smarterclayton · 2016-07-28T12:23:23Z

[test]

openshift-bot · 2016-07-28T12:25:38Z

Evaluated for origin test up to f8aa83f

openshift-bot · 2016-07-28T13:45:35Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7041/)

soltysh · 2016-07-28T14:15:33Z

@smarterclayton I must admit you're the "luckiest" person when it comes to flakes with this PR, yet another: #9548

re-[merge]

smarterclayton · 2016-07-28T14:29:12Z

It's the flake detector.

On Thu, Jul 28, 2016 at 10:15 AM, Maciej Szulik notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton I must admit you're
the "luckiest" person when it comes to flakes with this PR, yet another:
#9548 #9548

re-[merge]

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9240 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p4IkNRZtrYaroKPFam9PTQHbRJtaks5qaLmHgaJpZM4IxnZA
.

smarterclayton force-pushed the migrate_api branch from d8358f6 to a803f03 Compare June 9, 2016 04:59

smarterclayton force-pushed the migrate_api branch 3 times, most recently from 7e16e64 to b341d39 Compare June 10, 2016 04:37

smarterclayton force-pushed the migrate_api branch 5 times, most recently from 5e70e7e to cc5c962 Compare June 11, 2016 23:24

smarterclayton force-pushed the migrate_api branch 2 times, most recently from 7bf2697 to c6c0e20 Compare June 14, 2016 02:37

smarterclayton changed the title ~~WIP - Add a migration framework for mutable resources~~ Add a migration framework for mutable resources Jun 14, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2016

smarterclayton force-pushed the migrate_api branch from 9ca93e6 to 326a588 Compare June 14, 2016 14:36

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2016

deads2k reviewed Jun 15, 2016
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2016

smarterclayton force-pushed the migrate_api branch from 326a588 to 9a06747 Compare June 16, 2016 03:56

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2016

soltysh reviewed Jun 16, 2016
View reviewed changes

soltysh reviewed Jul 27, 2016
View reviewed changes

smarterclayton force-pushed the migrate_api branch from 530d160 to 3278289 Compare July 27, 2016 20:14

soltysh reviewed Jul 27, 2016
View reviewed changes

smarterclayton added 2 commits July 27, 2016 16:35

Add a migration framework for mutable resources

d04b1b5

Implement a migration framework and an image reference migrator: oadm migrate image-references registry1.com/*=registry2.com/* --confirm Reuse resource builder to manage output

generated: cmd changes

f8aa83f

smarterclayton force-pushed the migrate_api branch from 3278289 to f8aa83f Compare July 27, 2016 20:35

soltysh mentioned this pull request Jul 28, 2016

yum flakes: operation too slow #9203

Closed

soltysh mentioned this pull request Jul 28, 2016

flake: test/cmd/basicresources.sh oc new-project test4 #10076

Closed

soltysh mentioned this pull request Jul 28, 2016

flake: oc import-image php, image stream was updated #10080

Closed

openshift-bot merged commit f822de7 into openshift:master Jul 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a migration framework for mutable resources #9240

Add a migration framework for mutable resources #9240

smarterclayton commented Jun 9, 2016 •

edited

Loading

smarterclayton commented Jun 9, 2016

smarterclayton commented Jun 10, 2016

smarterclayton commented Jun 13, 2016

smarterclayton commented Jun 14, 2016 •

edited

Loading

soltysh commented Jun 15, 2016

deads2k Jun 15, 2016

smarterclayton Jun 15, 2016

deads2k commented Jun 15, 2016

soltysh Jun 16, 2016

smarterclayton Jun 16, 2016

smarterclayton commented Jul 26, 2016

smarterclayton commented Jul 27, 2016

soltysh Jul 27, 2016

smarterclayton Jul 27, 2016

soltysh Jul 27, 2016

soltysh Jul 27, 2016

soltysh commented Jul 27, 2016

smarterclayton commented Jul 27, 2016

smarterclayton commented Jul 27, 2016

smarterclayton commented Jul 28, 2016

soltysh commented Jul 28, 2016

soltysh commented Jul 28, 2016

openshift-bot commented Jul 28, 2016 •

edited

Loading

soltysh commented Jul 28, 2016

openshift-bot commented Jul 28, 2016

smarterclayton commented Jul 28, 2016 via email

smarterclayton commented Jul 28, 2016 via email

openshift-bot commented Jul 28, 2016

openshift-bot commented Jul 28, 2016

soltysh commented Jul 28, 2016

smarterclayton commented Jul 28, 2016

Add a migration framework for mutable resources #9240

Add a migration framework for mutable resources #9240

Conversation

smarterclayton commented Jun 9, 2016 • edited Loading

smarterclayton commented Jun 9, 2016

smarterclayton commented Jun 10, 2016

smarterclayton commented Jun 13, 2016

smarterclayton commented Jun 14, 2016 • edited Loading

soltysh commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jul 26, 2016

smarterclayton commented Jul 27, 2016

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

soltysh commented Jul 27, 2016

smarterclayton commented Jul 27, 2016

smarterclayton commented Jul 27, 2016

smarterclayton commented Jul 28, 2016

soltysh commented Jul 28, 2016

soltysh commented Jul 28, 2016

openshift-bot commented Jul 28, 2016 • edited Loading

soltysh commented Jul 28, 2016

openshift-bot commented Jul 28, 2016

smarterclayton commented Jul 28, 2016 via email

smarterclayton commented Jul 28, 2016 via email

openshift-bot commented Jul 28, 2016

openshift-bot commented Jul 28, 2016

soltysh commented Jul 28, 2016

smarterclayton commented Jul 28, 2016

smarterclayton commented Jun 9, 2016 •

edited

Loading

smarterclayton commented Jun 14, 2016 •

edited

Loading

openshift-bot commented Jul 28, 2016 •

edited

Loading