Initial state transfer by jmontleon · Pull Request #2 · migtools/crane-lib

jmontleon · 2021-05-07T00:02:01Z

Not ready for merge. Publishing a PR for visibility and to allow feedback.

https://github.com/jmontleon/crane-test makes use of this library as a simple test.

It provides interfaces for a transfer method such as rsync (potentiall rclone and others), a transport like stunnel, and endpoint such as route and loadbalancer

I've run tests with:

rsync/stunnel/route
rsync/stunnel/loadbalancer

jmontleon · 2021-05-07T02:17:40Z

Added a null/socat transport to demonstrate swapping out. Could be useful in cases where the transfer utility provides encryption too. I did this with a load balancer. I believe using a rsync/socat/route combo wouldn't work due to route limitations like we hit when working with pvc migrate, but I'll confirm later... There might be need for a compatibility matrix...

I'd like to add rclone to the mix as well, but I need to sit down and learn some about rclone.

alaypatel07 · 2021-05-07T05:42:03Z

+)
+
+// Quiesce applications on source cluster
+func QuiesceApplications(cfg *rest.Config, ns string) error {


I wired up this example to use the scale subresource instead of patching the objects on each GVR separately , if it helps you in anyway to make this modular https://gist.github.com/alaypatel07/4c1494f70b3cebad1f9534c1c0fde639#file-scale-go-L151.

I would have strongly urged to do this way but I think it will not work with jobs and deploymentconfig is doubtful so it is kind of up to you.

Update 1: I verified, deploymentconfigs works with generic approach in the gist

@alaypatel07 thank you for investigating. I was talking to Shawn late yesterday and I think it would be nice if we had an interface for quiesce as well. We could have simple, scaler, and / or others. It may be beneficial to have multiple options like with the other interfaces; here I could even see potential for perhaps running multiple quiesce options on one cluster if we become able to handle different classes of applications better or worse.

For now I was looking to mimic what MTC does so I can do a quiesced data migration so the thought here is less well formed.

jmontleon · 2021-05-07T13:52:41Z

Added rclone transfer and a README.md with a compatibility matrix. ~~Without stunnel neither work over routes.~~

shawn-hurley

Looking good, the implementation looks right, I think we should take a second on the interfaces though WDYT?

shawn-hurley · 2021-05-10T22:24:26Z

+var labels = map[string]string{"app": "crane2"}
+
+type Transfer interface {
+	SetSource(*rest.Config)


I wonder if instead of this, should we create an initializer that creates a Transfer object, but the sources are set in stone? same with PVC/Endpoint/Transport...

I wonder if a builder pattern with an options struct might be an interesting UX here?

That might make sense. I'll admit to not being 100% certain on what the best/simplest way to build up an entire transfer would be.

alaypatel07 · 2021-06-22T20:05:42Z

+
+func createLoadBalancerService(c client.Client, r *LoadBalancerEndpoint, t Transfer) error {
+	serviceSelector := labels
+	serviceSelector["pvc"] = t.PVC().Name


since the label selector will only work when pods have this label. If the name of PVC is larger than 63 chars, this would be an invalid label. We would have to make sure we get the labels truncated or hashed.

alaypatel07 · 2021-06-22T20:15:43Z

+	}
+
+	//FIXME. Seems to take a moment, probably something better to do than wait 5 seconds
+	time.Sleep(5 * time.Second)


probably split this into two methods, CreateEndpoint simply tries to create the service. The other method, ValidateEndpoint checks if the valid status is present? Once the valid status is populated, we can call the r.SetHostname and r.SetPort

alaypatel07 · 2021-06-22T20:16:12Z

+		return err
+	}
+
+	//FIXME. Seems to take a moment, probably something better to do than wait 5 seconds


you can use a poller, it will be slightly better. https://github.com/kubernetes/kubernetes/blob/2453f07e933a6d2ac0be1eb35061a2154ccce5b8/test/e2e/framework/util.go#L269

alaypatel07 · 2021-06-22T20:44:51Z

+	rcloneConfigMap := &v1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: r.PVC().Namespace,
+			Name:      rcloneConfigPrefix + r.PVC().Name,


you probably want to be using something more narrow-scoped for not running into shared config with same named PVC accross two namespaces.

DVM uses md5hash(rcloneConfigPrefix + r.PVC().Name + r.PVC().Namespace), we got bugs in DVM where PVC name was the same across two namespaces and the config was shared and failed DVM

I don't fully understand, but sure, if this produces something unique, it works for me. That's all I was after.

alaypatel07 · 2021-06-22T20:46:16Z

+
+	err := createRcloneServerConfig(c, r)
+	if err != nil {
+		return err


might want to check if this error is IsAlreadyExists(err)

If the config exists the right things to do is to update/patch with the desired configuration

alaypatel07 · 2021-06-22T20:48:06Z

+
+func (r *RcloneTransfer) createTransferServer(c client.Client) error {
+	deploymentLabels := labels
+	deploymentLabels["pvc"] = r.PVC().Name


same as the previous comment about PVC names greater than 63 chars

alaypatel07 · 2021-06-22T20:56:11Z

+		return err
+	}
+
+	r.SetHostname(route.Spec.Host)


Probably wanna check if the route is admitted before using the hostname. https://github.com/konveyor/mig-controller/blob/219b97f79d92613dcb032cd5ccf54174b90955b8/pkg/controller/directvolumemigration/rsync.go#L777-L780

alaypatel07

I wonder if we could have subpackages for each major component. The pattern I have seen is something like this:

state_transfer/endpoint/*
state_transfer/transport/*
state_transfer/transfer/*

and then each package has an interface.go which would declare the interface, other files can be implementation or helpers for implementation.

alaypatel07 · 2021-06-24T15:56:23Z

@jmontleon I am sorry I clicked the "Ready for review" button by mistake.

jmontleon · 2021-06-25T16:40:07Z

@alaypatel07 @shawn-hurley i would vote merge from here so we can start working in earnest. I feel like the client scheme setup and stuff is creating a couple unnecessary functions but maybe one of you can lend some wizardry after it's merged.

alaypatel07 · 2021-06-25T16:47:58Z

Sounds good to me on the merge.

jmontleon · 2021-06-25T16:51:49Z

Sounds good to me on the merge.

I haven't fixed a lot of the stuff you mentioned here. I'll parse them out into Issues so we don't forget. They make sense. Just not sure it should hold up getting code in so we can start improving.

jmontleon · 2021-06-25T17:00:29Z

I think that's all of them.

shawn-hurley

I agree on merging and starting to work in parallel

jmontleon requested review from eriknelson, pranavgaikwad and shawn-hurley May 7, 2021 00:02

jmontleon force-pushed the initial-state-transfer branch from 930f39d to 72793fb Compare May 7, 2021 00:03

jmontleon requested review from alaypatel07 and jwmatthews May 7, 2021 00:04

eriknelson reviewed May 7, 2021

View reviewed changes

Comment thread state_transfer/route_endpoint.go

jmontleon force-pushed the initial-state-transfer branch from 6eacd67 to a503346 Compare May 7, 2021 02:21

alaypatel07 reviewed May 7, 2021

View reviewed changes

jmontleon force-pushed the initial-state-transfer branch 2 times, most recently from 0b85235 to 18cafdb Compare May 7, 2021 13:51

jmontleon force-pushed the initial-state-transfer branch 4 times, most recently from 1b37e06 to 21de238 Compare May 10, 2021 17:17

jmontleon requested a review from jortel May 10, 2021 18:28

shawn-hurley reviewed May 10, 2021

View reviewed changes

alaypatel07 reviewed Jun 22, 2021

View reviewed changes

Comment thread state_transfer/route_endpoint.go

alaypatel07 reviewed Jun 22, 2021

View reviewed changes

alaypatel07 reviewed Jun 24, 2021

View reviewed changes

alaypatel07 marked this pull request as ready for review June 24, 2021 15:55

alaypatel07 marked this pull request as draft June 24, 2021 15:56

jmontleon force-pushed the initial-state-transfer branch 2 times, most recently from 8973fbb to 0c72d75 Compare June 25, 2021 16:32

jmontleon marked this pull request as ready for review June 25, 2021 16:34

jmontleon added 7 commits June 25, 2021 12:35

Initial state_transfer implementation

432948a

add null transport

6179ce4

add rclone transfer and fix null transport

98d8838

Add Compatibility Matrix

4c82a6d

Fix Getter names and stutters

55dc6b6

move strings to constants for reuse

6ddc03d

fix null transport

032cd5a

jmontleon force-pushed the initial-state-transfer branch from 0c72d75 to 1240609 Compare June 25, 2021 16:36

interface improvements

5529834

jmontleon force-pushed the initial-state-transfer branch from 1240609 to 5529834 Compare June 25, 2021 16:36

jmontleon requested review from alaypatel07 and shawn-hurley June 25, 2021 16:48

shawn-hurley approved these changes Jun 25, 2021

View reviewed changes

jmontleon merged commit a5ead85 into migtools:main Jun 25, 2021

Uh oh!

Conversation

jmontleon commented May 7, 2021

Uh oh!

Uh oh!

jmontleon commented May 7, 2021

Uh oh!

alaypatel07 May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmontleon May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmontleon commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shawn-hurley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alaypatel07 Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alaypatel07 left a comment

Choose a reason for hiding this comment

Uh oh!

alaypatel07 commented Jun 24, 2021

Uh oh!

jmontleon commented Jun 25, 2021

Uh oh!

alaypatel07 commented Jun 25, 2021

Uh oh!

jmontleon commented Jun 25, 2021

Uh oh!

jmontleon commented Jun 25, 2021

Uh oh!

shawn-hurley left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alaypatel07 May 7, 2021 •

edited

Loading

jmontleon May 7, 2021 •

edited

Loading

jmontleon commented May 7, 2021 •

edited

Loading

alaypatel07 Jun 22, 2021 •

edited

Loading