-
Notifications
You must be signed in to change notification settings - Fork 19
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
refact(migration): update volume migration to run in parallel #52
Conversation
This PR updates the volume migration code to update the SC by only one job when multiple jobs running in parallel. Signed-off-by: shubham <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided few comments
handles error case in isSCMigrationRequired func Signed-off-by: shubham <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes are good
pkg/migrate/cstor/volume.go
Outdated
} | ||
return false, err | ||
} | ||
if err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: No need of this if block
Signed-off-by: shubham <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Signed-off-by: shubham [email protected]
This PR updates the volume migration code to update the SC by only one job when multiple jobs running in parallel.
Local test performed:
Tested by launching 3 migration jobs in parallel and the job failure did not occur. Tried the same test 5 times and did not face the issue.