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.

Sign up for GitHub

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

Jump to bottom

[GEP-7] Adds a DeployMigrateWaiter component to manage containerruntimes #2762

Merged
merged 1 commit into from Aug 31, 2020
Merged

[GEP-7] Adds a DeployMigrateWaiter component to manage containerruntimes #2762

merged 1 commit into from Aug 31, 2020

Conversation

plkokanov
Copy link
Contributor

How to categorize this PR?

/area control-plane
/area control-plane-migration
/kind enhancement
/priority normal

What this PR does / why we need it:
This PR Refactors the deployment of ContainerRuntime resources and adopts the DeployMigrateWaiter functionality so that ContainerRuntime CRs can be deployed, destroyed, migrated and restored.

Previously the state of ContainerRuntime CRs weren't annotated with the gardenr.cloud/operation=restore annotation and their state wasn't properly added.

Which issue(s) this PR fixes:
part of #1631

Special notes for your reviewer:
Executed the following tests with the gvisor extension:

  1. Created a shoot with two worker definitions so that two gvisor containerruntime resources are created
  2. Hibernated the shoot cluster
  3. Woke up the shoot cluster
  4. Migrated the shoot cluster to a new seed
  5. Deleted one of the worker definitions and verified that the stale containerruntime resource also gets deleted

Release note:

ContainerRuntimes are now annotated with `gardener.cloud/operation=restore` during the `restore` phase of Control Plane Migration and their state (if any) is copied from the `ShootState` to the CRs' `status.state` field.

@plkokanov plkokanov requested a review from a team as a code owner August 20, 2020 11:05
@gardener-robot gardener-robot added area/control-plane Control plane related area/control-plane-migration Control plane migration related kind/enhancement Enhancement, improvement, extension priority/normal labels Aug 20, 2020
@plkokanov plkokanov changed the title Adds a DeployMigrateWaiter component to manage containerruntimes [GEP-7] Adds a DeployMigrateWaiter component to manage containerruntimes Aug 20, 2020
swilen-iwanow
swilen-iwanow reviewed Aug 24, 2020
View reviewed changes
Copy link
Contributor

@swilen-iwanow swilen-iwanow left a comment

Choose a reason for hiding this comment

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

A couple of suggestions from me.

pkg/operation/botanist/migration.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/shoot/shoot_control_reconcile.go Outdated Show resolved Hide resolved
pkg/operation/botanist/extensions/containerruntime/containerruntime.go Outdated Show resolved Hide resolved
pkg/operation/botanist/migration_test.go Show resolved Hide resolved
rfranzke
rfranzke reviewed Aug 25, 2020
View reviewed changes
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

I just had a rough, high-level view, @danielfoehrKn please have a (more detailed) look as well

/invite @danielfoehrKn
/assign @danielfoehrKn @swilen-iwanow

pkg/operation/botanist/extensions/containerruntime/containerruntime.go Outdated Show resolved Hide resolved
pkg/operation/botanist/extensions/containerruntime/containerruntime.go Outdated Show resolved Hide resolved
pkg/operation/botanist/extensions/containerruntime/containerruntime.go Outdated Show resolved Hide resolved
pkg/operation/botanist/extensions/containerruntime/containerruntime.go Outdated Show resolved Hide resolved
pkg/operation/botanist/migration_test.go Outdated Show resolved Hide resolved
pkg/operation/shoot/types.go Outdated Show resolved Hide resolved
danielfoehrKn
danielfoehrKn reviewed Aug 25, 2020
View reviewed changes
Copy link
Contributor

@danielfoehrKn danielfoehrKn left a comment

Choose a reason for hiding this comment

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

seems to be working fine, though did not test the actual migration.
Thanks also for refactoring the common library.

pkg/operation/botanist/extensions/containerruntime/containerruntime.go Outdated Show resolved Hide resolved
@plkokanov plkokanov force-pushed the cp-migration/containerruntime branch from bc9acc7 to b8f2510 Compare August 26, 2020 12:26
@plkokanov
Copy link
Contributor Author

Added a commit fixing the review comments @rfranzke @swilen-iwanow @danielfoehrKn could you have another look. If everything is ok I will squash the commits.

rfranzke
rfranzke previously approved these changes Aug 28, 2020
View reviewed changes
Copy link
Member

@rfranzke rfranzke left a comment •
edited

Choose a reason for hiding this comment

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

/lgtm from my side
/needs second-opinion

swilen-iwanow
swilen-iwanow previously approved these changes Aug 28, 2020
View reviewed changes
Copy link
Contributor

@swilen-iwanow swilen-iwanow left a comment

Choose a reason for hiding this comment

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

lgtm

@plkokanov plkokanov dismissed stale reviews from swilen-iwanow and rfranzke via e1a8b73 August 28, 2020 07:03
@plkokanov plkokanov force-pushed the cp-migration/containerruntime branch from b8f2510 to e1a8b73 Compare August 28, 2020 07:03
@rfranzke rfranzke merged commit 50f775b into gardener:master Aug 31, 2020
@timebertt timebertt mentioned this pull request Sep 2, 2020
Merged
@gardener-robot gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021
@plkokanov plkokanov deleted the cp-migration/containerruntime branch December 9, 2022 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfranzke rfranzke rfranzke approved these changes

@swilen-iwanow swilen-iwanow swilen-iwanow left review comments

@danielfoehrKn danielfoehrKn Awaiting requested review from danielfoehrKn

Assignees

@danielfoehrKn danielfoehrKn

@swilen-iwanow swilen-iwanow

Labels
area/control-plane Control plane related area/control-plane-migration Control plane migration related kind/enhancement Enhancement, improvement, extension
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@plkokanov @rfranzke @danielfoehrKn @swilen-iwanow @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot