✨ Add support for KCP remediation during cluster provisioning #7963

fabriziopandini · 2023-01-20T20:28:23Z

What this PR does / why we need it:
This PR proposes an amendment to KCP remediation so it will be possible to remediate failure happening while provisioning the CP (both first CP and CP machines while current replica < desired replica); It also introduces opt-in control of remediation retry behavior.

#7855 is the amendment to the KCP proposal describing those changes

Which issue(s) this PR fixes:
Fixes #7496

controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_types.go

vincepri

First set of comments

controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_types.go

controlplane/kubeadm/internal/controllers/helpers.go

controlplane/kubeadm/internal/controllers/remediation.go

test/infrastructure/docker/internal/controllers/dockermachine_controller.go

test/e2e/kcp_remediations_test.go

test/e2e/kcp_remediations.go

sbueringer

Sorry for the review spam :)

Now reviewed everything except the remediation_test.go (anticipating some changes there based on prod-code changes)

Great work on the e2e test!!

Wondering if it makes sense to move the e2e test into a separate PR that we can merge through independently.
I guess most of the test should work against main as well (of course not remediating the first machine).

Might just be nice to keep both PRs and reviews more focused and to have some sort of "remediation worked before the change and after" confirmation.

test/e2e/kcp_remediations.go

fabriziopandini · 2023-02-03T10:48:50Z

@vincepri @sbueringer
Thanks for the great feedback, this PR should be ready for a final pass now!

Please note that:

Now last remediation data is stored on machines, and this is the source of truth that KCP uses to take decisions and to surface the last remediation on kcp.status (kcp.status is not used anymore for taking decisions).
The change made it possible to have a remediation history for each machine, thus handling also the use case of failures flickering from one machine to another.
WRT to defaults for the new API fields, I'm proposing to use current behavior as a default since we don't have demand or a set of trusted data to use for picking up other values.

fabriziopandini · 2023-02-03T10:57:53Z

/retest

fabriziopandini · 2023-02-03T10:58:21Z

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

internal/controllers/machinehealthcheck/machinehealthcheck_targets.go

sbueringer · 2023-02-07T15:16:05Z

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

sbueringer · 2023-02-07T15:54:17Z

@fabriziopandini I'm pretty sure the e2e test doesn't work on Linux. I'm working on a fix which works at least on my Machine. I'll open a PR to verify if it works in Prow as well.

sbueringer · 2023-02-07T16:15:33Z

PR is open: #8075

Let's see if that works. I also improved the token creation.

sbueringer

first part of next round of review
(everything except internal/controllers)

docs/book/src/reference/labels_and_annotations.md

test/e2e/kcp_remediations.go

controlplane/kubeadm/internal/controllers/helpers.go

controlplane/kubeadm/internal/controllers/remediation.go

sbueringer · 2023-02-07T17:33:49Z

@fabriziopandini Opened a PR in your repo (fabriziopandini#55) which merges #8075 into the current PR

sbueringer · 2023-02-08T12:56:59Z

just fyi, I'll address my own and other findings and then merge the fixes into this PR via #8075

controlplane/kubeadm/internal/controllers/remediation.go

Co-authored-by: sbueringer <[email protected]>

fabriziopandini · 2023-02-09T20:39:47Z

@sbueringer @vincepri @jackfrancis this is ready for a final pass
kudos to @sbueringer for nailing down the work on fabriziopandini#55 and #8075, it really helped to push this to the last mile!

sbueringer · 2023-02-12T13:38:20Z

/lgtm

k8s-ci-robot · 2023-02-12T13:38:26Z

LGTM label has been added.

Git tree hash: 18d61cd0fc14f15b5e8f38bfd4e1c26871e678d8

sbueringer · 2023-02-15T18:43:13Z

starting lazy consensus from community meeting, 1 week from today (target Feb 22)

sbueringer · 2023-02-21T18:17:58Z

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

sbueringer · 2023-02-21T20:09:28Z

Unrelated flake
/retest

sbueringer · 2023-02-22T04:25:43Z

/lgtm
/approve

k8s-ci-robot · 2023-02-22T04:25:56Z

[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

sbueringer · 2023-02-22T04:40:27Z

/retest
the usual unit test flake

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 20, 2023

k8s-ci-robot requested review from enxebre and JoelSpeed January 20, 2023 20:28

fabriziopandini force-pushed the improve-kcp-remediation-2 branch from 18c798f to d461439 Compare January 21, 2023 12:56

sbueringer reviewed Jan 23, 2023

View reviewed changes

vincepri reviewed Jan 23, 2023

View reviewed changes

sbueringer reviewed Jan 30, 2023

View reviewed changes

controlplane/kubeadm/internal/controllers/helpers.go Outdated Show resolved Hide resolved

sbueringer reviewed Jan 30, 2023

View reviewed changes

sbueringer reviewed Jan 31, 2023

View reviewed changes

controlplane/kubeadm/internal/controllers/remediation.go Outdated Show resolved Hide resolved

sbueringer reviewed Jan 31, 2023

View reviewed changes

controlplane/kubeadm/internal/controllers/remediation.go Outdated Show resolved Hide resolved

sbueringer reviewed Jan 31, 2023

View reviewed changes

test/e2e/kcp_remediations.go Outdated Show resolved Hide resolved

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

fabriziopandini force-pushed the improve-kcp-remediation-2 branch from 1d2d1a3 to 17e2676 Compare February 3, 2023 10:42

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

fabriziopandini changed the title ~~[WIP] ✨ Add support for KCP remediation during cluster provisioning~~ ✨ Add support for KCP remediation during cluster provisioning Feb 3, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2023

sbueringer reviewed Feb 3, 2023

View reviewed changes

internal/controllers/machinehealthcheck/machinehealthcheck_targets.go Outdated Show resolved Hide resolved

sbueringer reviewed Feb 3, 2023

View reviewed changes

internal/controllers/machinehealthcheck/machinehealthcheck_targets.go Outdated Show resolved Hide resolved

fabriziopandini force-pushed the improve-kcp-remediation-2 branch from c71c4f3 to 980dde5 Compare February 3, 2023 14:11

sbueringer mentioned this pull request Feb 7, 2023

[WIP][DNR] 🌱 Improve & fix KCP remediation e2e test #8075

Closed

sbueringer reviewed Feb 7, 2023

View reviewed changes

docs/book/src/reference/labels_and_annotations.md Outdated Show resolved Hide resolved

test/e2e/kcp_remediations.go Outdated Show resolved Hide resolved

test/e2e/kcp_remediations.go Outdated Show resolved Hide resolved

jackfrancis reviewed Feb 7, 2023

View reviewed changes

test/e2e/kcp_remediations.go Show resolved Hide resolved

jackfrancis reviewed Feb 7, 2023

View reviewed changes

test/e2e/kcp_remediations.go Outdated Show resolved Hide resolved

sbueringer reviewed Feb 7, 2023

View reviewed changes

controlplane/kubeadm/internal/controllers/helpers.go Show resolved Hide resolved

controlplane/kubeadm/internal/controllers/remediation.go Outdated Show resolved Hide resolved

sbueringer mentioned this pull request Feb 8, 2023

Optimize CAPD machine controller to reduce noisy error messages #8086

Closed

sbueringer reviewed Feb 8, 2023

View reviewed changes

controlplane/kubeadm/internal/controllers/remediation.go Show resolved Hide resolved

Add support for KCP remediation during cluster provisioning

e98e2de

Co-authored-by: sbueringer <[email protected]>

fabriziopandini force-pushed the improve-kcp-remediation-2 branch from 980dde5 to e98e2de Compare February 9, 2023 20:36

k8s-ci-robot assigned sbueringer Feb 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2023

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

k8s-ci-robot merged commit 11e8e87 into kubernetes-sigs:main Feb 22, 2023

k8s-ci-robot added this to the v1.4 milestone Feb 22, 2023

sbueringer mentioned this pull request Feb 27, 2023

KCP: add condition to surface remediation state #8188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add support for KCP remediation during cluster provisioning #7963

✨ Add support for KCP remediation during cluster provisioning #7963

fabriziopandini commented Jan 20, 2023

vincepri left a comment

sbueringer left a comment •

edited

Loading

fabriziopandini commented Feb 3, 2023

fabriziopandini commented Feb 3, 2023

fabriziopandini commented Feb 3, 2023

sbueringer commented Feb 7, 2023

sbueringer commented Feb 7, 2023

sbueringer commented Feb 7, 2023

sbueringer left a comment

sbueringer commented Feb 7, 2023 •

edited

Loading

sbueringer commented Feb 8, 2023 •

edited

Loading

fabriziopandini commented Feb 9, 2023

sbueringer commented Feb 12, 2023

k8s-ci-robot commented Feb 12, 2023

sbueringer commented Feb 15, 2023

sbueringer commented Feb 21, 2023

sbueringer commented Feb 21, 2023

sbueringer commented Feb 22, 2023

k8s-ci-robot commented Feb 22, 2023

sbueringer commented Feb 22, 2023

✨ Add support for KCP remediation during cluster provisioning #7963

✨ Add support for KCP remediation during cluster provisioning #7963

Conversation

fabriziopandini commented Jan 20, 2023

vincepri left a comment

Choose a reason for hiding this comment

sbueringer left a comment • edited Loading

Choose a reason for hiding this comment

fabriziopandini commented Feb 3, 2023

fabriziopandini commented Feb 3, 2023

fabriziopandini commented Feb 3, 2023

sbueringer commented Feb 7, 2023

sbueringer commented Feb 7, 2023

sbueringer commented Feb 7, 2023

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer commented Feb 7, 2023 • edited Loading

sbueringer commented Feb 8, 2023 • edited Loading

fabriziopandini commented Feb 9, 2023

sbueringer commented Feb 12, 2023

k8s-ci-robot commented Feb 12, 2023

sbueringer commented Feb 15, 2023

sbueringer commented Feb 21, 2023

sbueringer commented Feb 21, 2023

sbueringer commented Feb 22, 2023

k8s-ci-robot commented Feb 22, 2023

sbueringer commented Feb 22, 2023

sbueringer left a comment •

edited

Loading

sbueringer commented Feb 7, 2023 •

edited

Loading

sbueringer commented Feb 8, 2023 •

edited

Loading