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.

Sign up for GitHub

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

Jump to bottom

Parallelize storage migration #19691

Merged
merged 2 commits into from
May 14, 2018
Merged

Parallelize storage migration #19691

merged 2 commits into from
May 14, 2018

Conversation

enj
Copy link
Contributor

@enj enj commented May 11, 2018 •
edited
Loading

Changes made to "oc adm migrate storage":

  1. Use a channel based producer / consumer pattern to parallelize the underlying migrator logic.
  2. Unit test new parallel code with the race detector.
  3. Chunk list requests to 500.
  4. Add a bandwidth flag to control the rate of network IO in Mbps. This defaults to 10 Mbps and can be set to zero to mean "no limit."
  5. Deprecate and hide the confirm and output flags. This removes the "dry run" mode from this command which never made any sense and was almost certainly always invoked as a mistake. Old scripts can continue to set these flags; the value will simply be ignored and a deprecation warning will be printed (but the command will not fail).

Signed-off-by: Monis Khan [email protected]

/assign @simo5 @smarterclayton
/kind bug
@sdodson @jupierce
@openshift/sig-security

@openshift-ci-robot openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/security labels May 11, 2018
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 11, 2018
simo5
simo5 reviewed May 11, 2018
View reviewed changes
pkg/oc/admin/migrate/migrator.go Outdated
// This should not be exposed as a CLI flag. Instead it
// should have a fixed value that is high enough to saturate
// the desired IOPS when parallel processing is desired.
Parallel int
Copy link
Contributor

Choose a reason for hiding this comment

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

Parallel -> Workers

simo5
simo5 reviewed May 11, 2018
View reviewed changes
pkg/oc/admin/migrate/storage/storage.go Outdated

// Total IO in megabytes per second across all workers.
// Zero means "no rate limit."
iops int
Copy link
Contributor

Choose a reason for hiding this comment

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

iops -> bandwidth

@simo5
Copy link
Contributor

simo5 commented May 11, 2018

aside for the bikeshedding on names
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2018
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

9 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@enj enj removed the lgtm Indicates that a PR is ready to be merged. label May 12, 2018
enj added 2 commits May 14, 2018 00:10
@enj
Parallelize storage migration
0960f60 
Changes made to "oc adm migrate storage":

1. Use a channel based producer / consumer pattern to parallelize
the underlying migrator logic.
2. Unit test new parallel code with the race detector.
3. Chunk list requests to 500.
4. Add a bandwidth flag to control the rate of network IO in Mbps.
This defaults to 10 Mbps and can be set to zero to mean "no limit."
5. Deprecate and hide the confirm and output flags.  This removes
the "dry run" mode from this command which never made any sense and
was almost certainly always invoked as a mistake.  Old scripts can
continue to set these flags; the value will simply be ignored and a
deprecation warning will be printed (but the command will not fail).

Signed-off-by: Monis Khan <[email protected]>
@enj
Generated completions
49a753e 
Signed-off-by: Monis Khan <[email protected]>
@enj enj force-pushed the enj/f/fast_migrate branch from 0582a58 to 49a753e Compare May 14, 2018 04:14
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj

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:

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

@enj enj added the lgtm Indicates that a PR is ready to be merged. label May 14, 2018
@enj
Copy link
Contributor Author

enj commented May 14, 2018

Comments addressed, tagging.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit c5fbe95 into openshift:master May 14, 2018
soltysh
soltysh reviewed May 14, 2018
View reviewed changes
pkg/oc/admin/migrate/storage/storage.go
"Average network bandwidth measured in megabits per second (Mbps) to use during storage migration. Zero means no limit. This flag is alpha and may change in the future.")

// remove flags that do not make sense
flags.MarkDeprecated("confirm", "storage migration does not support dry run, this flag is ignored")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since when confirm does not make sense?

@soltysh soltysh mentioned this pull request Jun 15, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh left review comments

@simo5 simo5 simo5 left review comments

@juanvallejo juanvallejo Awaiting requested review from juanvallejo

Assignees

@smarterclayton smarterclayton

@simo5 simo5

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. sig/security size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@enj @simo5 @openshift-bot @openshift-ci-robot @soltysh @smarterclayton @openshift-merge-robot