Add migration logic for moving from in-tree to out-of-tree #484

prashanth26 · 2020-07-07T08:01:11Z

What this PR does / why we need it:
This PR adds migration logic for moving from in-tree to out-of-tree

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Added migration logic for moving from provider-specific machine class to generic machine classes in out of tree code path. On migration, the machine.sapcloud.io/migrated annotation set on the old machine class.

New flag `delete-migrated-machine-class` is introduced. When set to true (defaulted to false), deletes any provider-specific machine class (e.g. AWSMachineClass) that has the machine.sapcloud.io/migrated annotation set on it.

The machine controller adds finalizer only when machine reference is present, deletes it otherwise.

prashanth26 · 2020-07-07T08:02:57Z

/hold

prashanth26 · 2020-07-09T07:03:37Z

/hold until this is tested through the Gardener

prashanth26 · 2020-07-15T05:26:14Z

/unhold open for review

amshuman-kr

@prashanth26 Thanks for the PR! I have some minor comments. However, the main one is regarding passing machine class client in the MigrateMachineClass call.

amshuman-kr · 2020-07-20T10:52:47Z

docs/development/machine_error_codes.md

+This driver call will be called by the Machine Controller to try to perform a machineClass migration for an unknown machineClass Kind. This helps in migration of one kind of machineClass to another kind. For instance an machineClass custom resource of `AWSMachineClass` to `MachineClass`.
+
+- On successful migration of machine class the Provider MUST reply `0 OK` (or) `nil` error.
+- `MigrateMachineClassRequest` expects the `ClassSpec` containing the `type ClassSpec struct`. It also expects a `MachineClass` client that can be used to communicate with the cluster containing the machine objects. It also passes the `Namespace` where these objects can be found.


Why not pass the existing provider machine class as a parameter and expect a filled up generic machine class object in return? If type is a problem, it could be just bytes with the callee decoding it.

I have made these changes here - a646f1a#diff-40981fe61397ebaeddeec79632a3e1c4R141-R148

amshuman-kr · 2020-07-20T10:53:42Z

docs/development/machine_error_codes.md

+
+- On successful migration of machine class the Provider MUST reply `0 OK` (or) `nil` error.
+- `MigrateMachineClassRequest` expects the `ClassSpec` containing the `type ClassSpec struct`. It also expects a `MachineClass` client that can be used to communicate with the cluster containing the machine objects. It also passes the `Namespace` where these objects can be found.
+- `MigrateMachineClass` is responsible for creating the new `CR` of the new kind (MachinClass) with the same name as the previous one. It also responsible for annotating the old machineClass CR with an migrated annotation.


Why not just receive the filled up generic machine class object from the MigrateMachineClass and actually create the CR instance in the caller library?

This reason I have been avoiding this is because, then this code of annotating the old machine class would go into the current reconciliation loop. And this would mean multiple different calls might have to be made to different machineClass clients while annotation, however keeping it inside the provider spec it would be easier to determine which call to exactly make. However, now what I re-think about it. It seems fine to me.

Let me try making such changes with the new GCP provider and test it out. And see if I face any difficulties.

👍

The existing in-tree code already has the logic based on the old typed machine classes. So, this would be one more place. Also, this would be only for migration, we can phase this code out via deprecation over a couple of releases. But it if is there in the providers, then phasing it out also is then multiplied by the number of providers.

Yes makes sense.

I have moved the common logic into this pkg with this commit. Also the leftover at the provider can be seen here at the provider. I hope this is what you meant.

pkg/controller/machineset_util.go

pkg/util/provider/driver/driver.go

prashanth26 · 2020-08-25T18:52:44Z

@amshuman-kr & @hardikdr - I have made all the requested code changes to the best of my knowledge, let me know if I missed something. Also, I have added a few tests for verifying the migration code path.

hardikdr · 2020-08-26T10:50:02Z

@prashanth26 tests are failing, can you please check.
I'll take a final look soon.

prashanth26 · 2020-08-26T11:13:28Z

@hardikdr - Sorry, missed out some things while running the final tests. Seems to be working now.

hardikdr · 2020-08-27T11:51:27Z

@prashanth26 It'd be nice to squash & re-arrange a few of the commits in a meaningful order.

@amshuman-kr @ggaurav10 Can you please take a final look and approve if possible?

hardikdr · 2020-08-27T11:53:42Z

/lgtm

/hold till commits are squashed.

prashanth26 · 2020-08-27T11:54:08Z

@prashanth26 It'd be nice to squash & re-arrange a few of the commits in a meaningful order.

Yes Hardik. Shall do that once all comments are addressed.

- Any changes/updates to class.Kind propagates top-down without rolling updates - Ignore reconciling machines with unknown ClassKind at MCM - Updated docs to reflect these changes - Updated old machine class finalizers - Introduced a new flag to delete old machine classes on migration

- The machineClass controllers add finalizer only when reference is present, and removes it otherwise - There is also logic for setting the deletion timestamp if migration annotation and delete-migrated-machine-class flag is set

hardikdr · 2020-08-28T05:30:58Z

/unhold

ggaurav10

lgtm

amshuman-kr

LGTM

prashanth26 · 2020-08-28T06:47:03Z

Thank you, guys. Will merge it once the tests pass.

prashanth26 requested review from ggaurav10 and a team as code owners July 7, 2020 08:01

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 7, 2020

gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jul 7, 2020

prashanth26 removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jul 7, 2020

gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 7, 2020

prashanth26 force-pushed the migration branch from 2aa1ebc to 77ef915 Compare July 7, 2020 08:12

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 7, 2020

prashanth26 force-pushed the migration branch from 77ef915 to b45c4bb Compare July 7, 2020 08:26

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 7, 2020

prashanth26 mentioned this pull request Jul 7, 2020

Updated documentation to add new GenerateMachineClassForMigration interface gardener/machine-controller-manager-provider-sampleprovider#19

Merged

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 7, 2020

gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jul 9, 2020

prashanth26 force-pushed the migration branch from b45c4bb to 3b8a6cb Compare July 13, 2020 11:17

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 13, 2020

prashanth26 force-pushed the migration branch from 3b8a6cb to 4677b86 Compare July 15, 2020 05:09

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 15, 2020

gardener-robot removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jul 15, 2020

amshuman-kr reviewed Jul 20, 2020

View reviewed changes

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 4, 2020

prashanth26 force-pushed the migration branch from 3cbcc52 to d2e7f11 Compare August 4, 2020 16:06

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 4, 2020

hardikdr added this to the v0.34.0 milestone Aug 25, 2020

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 25, 2020

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 25, 2020

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 25, 2020

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 25, 2020

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 25, 2020

prashanth26 force-pushed the migration branch from 83050e1 to b264b66 Compare August 26, 2020 11:12

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 26, 2020

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 26, 2020

gardener-robot added reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies reviewed/lgtm Has approval for merging labels Aug 27, 2020

hardikdr approved these changes Aug 27, 2020

View reviewed changes

prashanth26 added 2 commits August 28, 2020 10:49

Updated finailizer deletion logic for MachineClasses

0a56891

- The machineClass controllers add finalizer only when reference is present, and removes it otherwise - There is also logic for setting the deletion timestamp if migration annotation and delete-migrated-machine-class flag is set

prashanth26 force-pushed the migration branch from b264b66 to 0a56891 Compare August 28, 2020 05:26

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 28, 2020

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 28, 2020

gardener-robot removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Aug 28, 2020

ggaurav10 approved these changes Aug 28, 2020

View reviewed changes

amshuman-kr approved these changes Aug 28, 2020

View reviewed changes

prashanth26 merged commit 90f8b67 into gardener:master Aug 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add migration logic for moving from in-tree to out-of-tree #484

Add migration logic for moving from in-tree to out-of-tree #484

prashanth26 commented Jul 7, 2020 •

edited

Loading

prashanth26 commented Jul 7, 2020

prashanth26 commented Jul 9, 2020

prashanth26 commented Jul 15, 2020

amshuman-kr left a comment

amshuman-kr Jul 20, 2020

prashanth26 Aug 23, 2020

amshuman-kr Jul 20, 2020

prashanth26 Jul 21, 2020

amshuman-kr Jul 21, 2020

prashanth26 Jul 22, 2020

prashanth26 Aug 4, 2020

prashanth26 commented Aug 25, 2020

hardikdr commented Aug 26, 2020

prashanth26 commented Aug 26, 2020

hardikdr commented Aug 27, 2020

hardikdr commented Aug 27, 2020

prashanth26 commented Aug 27, 2020

hardikdr commented Aug 28, 2020

ggaurav10 left a comment

amshuman-kr left a comment

prashanth26 commented Aug 28, 2020

Add migration logic for moving from in-tree to out-of-tree #484

Add migration logic for moving from in-tree to out-of-tree #484

Conversation

prashanth26 commented Jul 7, 2020 • edited Loading

prashanth26 commented Jul 7, 2020

prashanth26 commented Jul 9, 2020

prashanth26 commented Jul 15, 2020

amshuman-kr left a comment

Choose a reason for hiding this comment

amshuman-kr Jul 20, 2020

Choose a reason for hiding this comment

prashanth26 Aug 23, 2020

Choose a reason for hiding this comment

amshuman-kr Jul 20, 2020

Choose a reason for hiding this comment

prashanth26 Jul 21, 2020

Choose a reason for hiding this comment

amshuman-kr Jul 21, 2020

Choose a reason for hiding this comment

prashanth26 Jul 22, 2020

Choose a reason for hiding this comment

prashanth26 Aug 4, 2020

Choose a reason for hiding this comment

prashanth26 commented Aug 25, 2020

hardikdr commented Aug 26, 2020

prashanth26 commented Aug 26, 2020

hardikdr commented Aug 27, 2020

hardikdr commented Aug 27, 2020

prashanth26 commented Aug 27, 2020

hardikdr commented Aug 28, 2020

ggaurav10 left a comment

Choose a reason for hiding this comment

amshuman-kr left a comment

Choose a reason for hiding this comment

prashanth26 commented Aug 28, 2020

prashanth26 commented Jul 7, 2020 •

edited

Loading