-
Notifications
You must be signed in to change notification settings - Fork 14
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 support for syncing block volumes #121
Conversation
4bdb6f6
to
0b13e2d
Compare
20bf851
to
a690774
Compare
I fixed the unit test build failure (and also updated go mod tidy like you did) |
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.
Thank you for amazing work, great PR! Just want to understand some changes better
) | ||
|
||
const ( | ||
blockrsyncImage = "quay.io/awels/blockrsync:latest" |
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.
do you intend to keep the image in your account/
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.
No, I would love for this to be part of the konveyor organization. I just started the development in my local repo so I could make progress.
@@ -39,8 +39,8 @@ type Transport interface { | |||
// ServerVolumes returns a list of volumes transfers can add to their server Pods | |||
ServerVolumes() []v1.Volume | |||
Direct() bool | |||
CreateServer(client.Client, endpoint.Endpoint) error | |||
CreateClient(client.Client, endpoint.Endpoint) error | |||
CreateServer(client.Client, string, endpoint.Endpoint) error |
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.
what is the need for changing interfaces for prefix?
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.
So I needed to create a new stunnel for block syncing since I have a separate server pod. This required a new stunnel/service/route. To identify the difference I added a prefix. This way it is easy to tell which stunnel is for filesystem sync and which one is for block sync.
a690774
to
66cb3df
Compare
Right now we only support syncing file volumes. This adds support for block volumes using the blockrsync utility code here: https://github.com/awels/blockrsync This also modifies the transport code to allow a second transport to be used. This is needed because we start a second blockrsync server that needs a different transport to get the routing of data correct. Added unit tests for the transport and rsync code Signed-off-by: Alexander Wels <awels@redhat.com>
66cb3df
to
8303381
Compare
Also note there will be a matching PR in mig-controller that uses the modified interface (with the extra prefix parameter). But I can't create that PR until I have a crane-lib that supports it. |
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, thank you again for the awesome work!
Right now we only support syncing file volumes.
This adds support for block volumes using the
blockrsync utility code here:
https://github.com/awels/blockrsync
This also modifies the transport code to allow a
second transport to be used. This is needed
because we start a second blockrsync server that
needs a different transport to get the routing of
data correct.
Added unit tests for the transport and rsync code