🐛 topology controller should avoid unnecessary rollouts during upgrades #8628

ykakarap · 2023-05-10T00:40:10Z

What this PR does / why we need it:

This PR modified the topology controller so that it modified pushing any changes to ControlPlane/MachineDeployment if it is pending an upgrade.

If a ControlPlane or MD is pending an upgrade it implies that it will be rollouts after it picks up the upgrade. Therefore it is better to hold out other changes, potentially causing rollouts, to these objects as well in the mean time to avoid unnecessary machine churn.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #8656
Fixes #8695

ykakarap · 2023-05-15T04:38:09Z

/test pull-cluster-api-e2e-informing-dualstack-ipv6-main

internal/controllers/topology/cluster/reconcile_state.go

killianmuldoon

/area clusterclass

sbueringer

Hm. I wonder if it would be better to define "upgrade is held back" independent of the pending stuff.
Or we have to re-evaluate what pending exactly means and align to what we need.

In my opinion holding back an upgrade simply means:

For control plane: Cluster.topology.version != desiredCP.version
For MD: Cluster.topology.version != desiredMD.version

Maybe it's as easy as that?

internal/controllers/topology/cluster/conditions.go

internal/controllers/topology/cluster/desired_state.go

internal/controllers/topology/cluster/reconcile_state.go

sbueringer

Good progress :)

Let's pair a bit on it. I think we have some good ideas on how we can optimize this further

internal/controllers/topology/cluster/conditions.go

internal/controllers/topology/cluster/scope/upgradetracker.go

internal/controllers/topology/cluster/desired_state.go

sbueringer

Good progress :)

Let's pair a bit on it. I think we have some good ideas on how we can optimize this further

Let's rebase on top of #8658

fabriziopandini

made a first pass while the PR is still WIP, and it looks already good
just a couple of nits from my side

internal/controllers/topology/cluster/desired_state.go

internal/controllers/topology/cluster/scope/upgradetracker.go

ykakarap · 2023-05-30T18:42:05Z

/test ?

k8s-ci-robot · 2023-05-30T18:42:07Z

@ykakarap: The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-dualstack-ipv6-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-scale-main-experimental
/test pull-cluster-api-e2e-workload-upgrade-1-27-latest-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-informing-ipv6-main
pull-cluster-api-e2e-informing-main
pull-cluster-api-e2e-main
pull-cluster-api-test-main
pull-cluster-api-test-mink8s-main
pull-cluster-api-verify-main

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ykakarap · 2023-05-30T18:42:27Z

/test pull-cluster-api-e2e-full-main

ykakarap · 2023-05-31T03:30:57Z

/retest

sbueringer

Only nits, looks great!

internal/controllers/topology/cluster/scope/upgradetracker.go

internal/controllers/topology/cluster/conditions.go

internal/controllers/topology/cluster/desired_state.go

internal/controllers/topology/cluster/desired_state_test.go

internal/controllers/topology/cluster/reconcile_state.go

internal/controllers/topology/cluster/reconcile_state_test.go

ykakarap · 2023-06-02T01:13:04Z

/test pull-cluster-api-e2e-full-main

ykakarap · 2023-06-02T08:11:01Z

/retest

sbueringer · 2023-06-02T08:27:42Z

Thx, last nit from my side: #8628 (comment)

fabriziopandini · 2023-06-05T10:38:03Z

pending latest comment from @sbueringer, lgtm from my side

k8s-ci-robot · 2023-06-05T16:14:23Z

@ykakarap: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-e2e-informing-dualstack-ipv6-main	`14ee7f0`	link	false	`/test pull-cluster-api-e2e-informing-dualstack-ipv6-main`
pull-cluster-api-e2e-full-dualstack-ipv6-main	`6b2f3a1`	link	false	`/test pull-cluster-api-e2e-full-dualstack-ipv6-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

sbueringer · 2023-06-05T16:30:38Z

Thank you!

/lgtm
/approve

Let's hold until we have some idea what's going on regarding #8786 (just to keep debugging as easy as possible. This PR could introduce another potential error source)

sbueringer · 2023-06-05T16:30:43Z

/hold

k8s-ci-robot · 2023-06-05T16:30:45Z

LGTM label has been added.

Git tree hash: a6c0f5bb594609962f3252e4e33d09f8d5f40933

k8s-ci-robot · 2023-06-05T16:30:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

~~OWNERS~~ [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ykakarap · 2023-06-05T16:32:24Z

/retest

sbueringer · 2023-06-09T10:18:43Z

Updated the status of #8786 (comment). I think we're good with merging this PR. The issue is something in KCP. So unrelated to this PR

/hold cancel

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 10, 2023

k8s-ci-robot requested review from JoelSpeed and stmcginnis May 10, 2023 00:40

ykakarap changed the title ~~⚠️ [DO_NOT_REVIEW][EXPIREMENT] topology controller should avoid double rollouts during upgrades~~ ⚠️ [DO_NOT_REVIEW][EXPERIMENT] topology controller should avoid double rollouts during upgrades May 10, 2023

ykakarap changed the title ~~⚠️ [DO_NOT_REVIEW][EXPERIMENT] topology controller should avoid double rollouts during upgrades~~ ⚠️ [DO_NOT_REVIEW][EXPERIMENT] topology controller should avoid unnecessary rollouts during upgrades May 14, 2023

ykakarap force-pushed the pr-double-rollout_topology branch from 14ee7f0 to 6b2f3a1 Compare May 15, 2023 17:54

ykakarap changed the title ~~⚠️ [DO_NOT_REVIEW][EXPERIMENT] topology controller should avoid unnecessary rollouts during upgrades~~ 🐛 topology controller should avoid unnecessary rollouts during upgrades May 15, 2023

ykakarap commented May 15, 2023

View reviewed changes

internal/controllers/topology/cluster/reconcile_state.go Outdated Show resolved Hide resolved

ykakarap force-pushed the pr-double-rollout_topology branch from b15064d to 2b9f1f7 Compare May 16, 2023 01:51

killianmuldoon reviewed May 17, 2023

View reviewed changes

k8s-ci-robot added the area/clusterclass Issues or PRs related to clusterclass label May 17, 2023

sbueringer reviewed May 22, 2023

View reviewed changes

internal/controllers/topology/cluster/conditions.go Outdated Show resolved Hide resolved

internal/controllers/topology/cluster/desired_state.go Outdated Show resolved Hide resolved

internal/controllers/topology/cluster/reconcile_state.go Outdated Show resolved Hide resolved

ykakarap changed the title ~~🐛 topology controller should avoid unnecessary rollouts during upgrades~~ 🐛 [WIP] topology controller should avoid unnecessary rollouts during upgrades May 23, 2023

ykakarap force-pushed the pr-double-rollout_topology branch from 37d855a to a53b2c5 Compare May 24, 2023 07:22

sbueringer reviewed May 24, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2023

fabriziopandini reviewed May 29, 2023

View reviewed changes

internal/controllers/topology/cluster/desired_state.go Outdated Show resolved Hide resolved

internal/controllers/topology/cluster/scope/upgradetracker.go Outdated Show resolved Hide resolved

ykakarap force-pushed the pr-double-rollout_topology branch from a53b2c5 to db2140a Compare May 30, 2023 04:25

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 30, 2023

sbueringer reviewed Jun 1, 2023

View reviewed changes

k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 2, 2023

ykakarap changed the title ~~🐛 [WIP] topology controller should avoid unnecessary rollouts during upgrades~~ 🐛 topology controller should avoid unnecessary rollouts during upgrades Jun 2, 2023

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 2, 2023

ykakarap force-pushed the pr-double-rollout_topology branch from 5fb211a to 13d456d Compare June 2, 2023 05:32

ykakarap mentioned this pull request Jun 2, 2023

📖 update lifecycle hooks documentation with new upgrade rules #8782

Merged

ykakarap mentioned this pull request Jun 3, 2023

🌱 add E2E test for MachineSet Preflight checks #8698

Merged

topology controller should avoid unnecessary rollouts during upgrades

0be8e6e

ykakarap force-pushed the pr-double-rollout_topology branch from 13d456d to 0be8e6e Compare June 5, 2023 16:07

k8s-ci-robot assigned sbueringer Jun 5, 2023

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 5, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2023

k8s-ci-robot merged commit 0ba7f45 into kubernetes-sigs:main Jun 9, 2023
10 checks passed

k8s-ci-robot added this to the v1.5 milestone Jun 9, 2023

nikParasyr mentioned this pull request Jul 26, 2023

fix(magnum): enable upgrades with cluster_template_id changes terraform-provider-openstack/terraform-provider-openstack#1598

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 topology controller should avoid unnecessary rollouts during upgrades #8628

🐛 topology controller should avoid unnecessary rollouts during upgrades #8628

ykakarap commented May 10, 2023 •

edited by sbueringer

Loading

ykakarap commented May 15, 2023

killianmuldoon left a comment

sbueringer left a comment •

edited

Loading

sbueringer left a comment

sbueringer left a comment •

edited

Loading

fabriziopandini left a comment

ykakarap commented May 30, 2023

k8s-ci-robot commented May 30, 2023

ykakarap commented May 30, 2023

ykakarap commented May 31, 2023

sbueringer left a comment

ykakarap commented Jun 2, 2023

ykakarap commented Jun 2, 2023

sbueringer commented Jun 2, 2023

fabriziopandini commented Jun 5, 2023

k8s-ci-robot commented Jun 5, 2023 •

edited

Loading

sbueringer commented Jun 5, 2023

sbueringer commented Jun 5, 2023

k8s-ci-robot commented Jun 5, 2023

k8s-ci-robot commented Jun 5, 2023

ykakarap commented Jun 5, 2023

sbueringer commented Jun 9, 2023

🐛 topology controller should avoid unnecessary rollouts during upgrades #8628

🐛 topology controller should avoid unnecessary rollouts during upgrades #8628

Conversation

ykakarap commented May 10, 2023 • edited by sbueringer Loading

ykakarap commented May 15, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

sbueringer left a comment • edited Loading

Choose a reason for hiding this comment

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer left a comment • edited Loading

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

ykakarap commented May 30, 2023

k8s-ci-robot commented May 30, 2023

ykakarap commented May 30, 2023

ykakarap commented May 31, 2023

sbueringer left a comment

Choose a reason for hiding this comment

ykakarap commented Jun 2, 2023

ykakarap commented Jun 2, 2023

sbueringer commented Jun 2, 2023

fabriziopandini commented Jun 5, 2023

k8s-ci-robot commented Jun 5, 2023 • edited Loading

sbueringer commented Jun 5, 2023

sbueringer commented Jun 5, 2023

k8s-ci-robot commented Jun 5, 2023

k8s-ci-robot commented Jun 5, 2023

ykakarap commented Jun 5, 2023

sbueringer commented Jun 9, 2023

ykakarap commented May 10, 2023 •

edited by sbueringer

Loading

sbueringer left a comment •

edited

Loading

sbueringer left a comment •

edited

Loading

k8s-ci-robot commented Jun 5, 2023 •

edited

Loading