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.

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

Improve accuracy of local control plane migration e2e tests #7981

Merged
merged 5 commits into from
May 26, 2023

Conversation

rfranzke
Copy link
Member

How to categorize this PR?

/area dev-productivity
/kind enhancement

What this PR does / why we need it:
When performing control plane migration with provider-local, the full migration and restoration logic implemented in the extensions library (generic Worker actuator) is now executed (previously, it was skipped). This improves the accuracy of the e2e tests for control plane migration.

Which issue(s) this PR fixes:
Introduced with #6059

Special notes for your reviewer:
/cc @plkokanov

Release note:

When performing control plane migration with `provider-local`, the full migration and restoration logic implemented in the extensions library (generic `Worker` actuator) is now executed (previously, it was skipped). This improves the accuracy of the e2e tests for control plane migration.

@gardener-prow gardener-prow bot requested a review from plkokanov May 24, 2023 19:33
@gardener-prow gardener-prow bot added area/dev-productivity Developer productivity related (how to improve development) kind/enhancement Enhancement, improvement, extension cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 24, 2023
@rfranzke
Copy link
Member Author

FYI @JensAc @mreiger

@plkokanov
Copy link
Contributor

/assign

Copy link
Contributor

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

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

Just one nit, otherwise I think it looks good.

pkg/provider-local/controller/worker/actuator.go Outdated Show resolved Hide resolved
rfranzke and others added 5 commits May 25, 2023 20:44
This way, they become reusable even when no `genericActuator` object is available

Co-Authored-By: Jens Schneider <schneider@23technologies.cloud>
Co-Authored-By: mreiger <michael@rauschpfeife.net>
…nction

Earlier, the `genericactuator.Restore()` was restoring and always calling `Reconcile()` at the end. It is still doing this, however now the restoration logic has been separated into `RestoreWithoutReconcile`. This allows extensions to optionally perform custom operations before they call the `Reconcile()` function.

Co-Authored-By: Jens Schneider <schneider@23technologies.cloud>
Co-Authored-By: mreiger <michael@rauschpfeife.net>
Earlier, the `Worker` controller of `provider-local` has overwritten the `Migrate` function which caused the essential migration logic in the generic `Worker` actuator to not get executed. This limited the accuracy of the CPM e2e test since the essential logic was not executed.
Similarly, `Restore` was not executing the essential logic of the generic `Worker` actuator. Now it is calling the the `RestoreWithoutReconcile()` and `Reconcile()` afterwards.

In the next commit, it will perform some custom logic to allow making use of the essential CPM logic in generic `Worker` actuator.

Co-Authored-By: Jens Schneider <schneider@23technologies.cloud>
Co-Authored-By: mreiger <michael@rauschpfeife.net>
Co-Authored-By: Jens Schneider <schneider@23technologies.cloud>
Co-Authored-By: mreiger <michael@rauschpfeife.net>
@plkokanov
Copy link
Contributor

/lgtm
/approve

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 26, 2023
@gardener-prow
Copy link
Contributor

gardener-prow bot commented May 26, 2023

LGTM label has been added.

Git tree hash: a2dec581a0b2f891f1a927652f9f06b7775b7ec6

@gardener-prow
Copy link
Contributor

gardener-prow bot commented May 26, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: plkokanov

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

@gardener-prow gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2023
@acumino
Copy link
Member

acumino commented May 26, 2023

/test pull-gardener-integration

@gardener-prow gardener-prow bot merged commit 64b1b85 into gardener:master May 26, 2023
16 checks passed
@rfranzke rfranzke deleted the hackathon/local-cpm branch May 26, 2023 13:47
andrerun pushed a commit to andrerun/gardener that referenced this pull request Jul 6, 2023
…#7981)

* Decouple certain functions from generic `Worker` actuator

This way, they become reusable even when no `genericActuator` object is available

Co-Authored-By: Jens Schneider <schneider@23technologies.cloud>
Co-Authored-By: mreiger <michael@rauschpfeife.net>

* Move restoration logic in generic `Worker` actuator into dedicated function

Earlier, the `genericactuator.Restore()` was restoring and always calling `Reconcile()` at the end. It is still doing this, however now the restoration logic has been separated into `RestoreWithoutReconcile`. This allows extensions to optionally perform custom operations before they call the `Reconcile()` function.

Co-Authored-By: Jens Schneider <schneider@23technologies.cloud>
Co-Authored-By: mreiger <michael@rauschpfeife.net>

* [provider-local] Drop `Migrate` overwrite in `Worker` controller

Earlier, the `Worker` controller of `provider-local` has overwritten the `Migrate` function which caused the essential migration logic in the generic `Worker` actuator to not get executed. This limited the accuracy of the CPM e2e test since the essential logic was not executed.
Similarly, `Restore` was not executing the essential logic of the generic `Worker` actuator. Now it is calling the the `RestoreWithoutReconcile()` and `Reconcile()` afterwards.

In the next commit, it will perform some custom logic to allow making use of the essential CPM logic in generic `Worker` actuator.

Co-Authored-By: Jens Schneider <schneider@23technologies.cloud>
Co-Authored-By: mreiger <michael@rauschpfeife.net>

* [provider-local] Delete stale `Machine`s after CPM

Co-Authored-By: Jens Schneider <schneider@23technologies.cloud>
Co-Authored-By: mreiger <michael@rauschpfeife.net>

* Address PR review feedback

---------

Co-authored-by: Jens Schneider <schneider@23technologies.cloud>
Co-authored-by: mreiger <michael@rauschpfeife.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dev-productivity Developer productivity related (how to improve development) cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants