Skip to content
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

Add a migration framework for mutable resources #9240

Merged
merged 2 commits into from
Jul 28, 2016

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Jun 9, 2016

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
Copy link
Contributor Author

[test]

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

This is done barring tests.

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

@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 smarterclayton force-pushed the migrate_api branch 2 times, most recently from 7bf2697 to c6c0e20 Compare June 14, 2016 02:37
@smarterclayton smarterclayton changed the title WIP - Add a migration framework for mutable resources Add a migration framework for mutable resources Jun 14, 2016
@smarterclayton
Copy link
Contributor Author

smarterclayton commented Jun 14, 2016

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.

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

soltysh commented Jun 15, 2016

/cc

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@deads2k
Copy link
Contributor

deads2k commented Jun 15, 2016

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.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2016
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2016
kcmdutil.CheckErr(options.Complete(f, cmd, args))
kcmdutil.CheckErr(options.Validate())
if err := options.Run(); err != nil {
// TODO: move met to kcmdutil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@smarterclayton
Copy link
Contributor Author

Aaaany other comments?

return nil
}

func (o *MigrateImageReferenceOptions) Validate() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return changed
}

func updatePodTemplate(template *kapi.PodTemplateSpec, fn TransformImageFunc) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method looks like it's not used anywhere.

@soltysh
Copy link
Contributor

soltysh commented Jul 27, 2016

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
Copy link
Contributor Author

nits fixed [merge]

@smarterclayton
Copy link
Contributor Author

[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
Copy link
Contributor Author

[test] #9959

@soltysh
Copy link
Contributor

soltysh commented Jul 28, 2016

Flake from #9203.

re-[merge]

@soltysh
Copy link
Contributor

soltysh commented Jul 28, 2016

Flake from #10076.

re-[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 28, 2016

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

@soltysh
Copy link
Contributor

soltysh commented Jul 28, 2016

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

re-[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to f8aa83f

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Jul 28, 2016 via email

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Jul 28, 2016 via email

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to f8aa83f

@openshift-bot
Copy link
Contributor

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

@soltysh
Copy link
Contributor

soltysh commented Jul 28, 2016

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

re-[merge]

@smarterclayton
Copy link
Contributor Author

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
.

@openshift-bot openshift-bot merged commit f822de7 into openshift:master Jul 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants