Skip to content

🌱 Add conditions for deletion workflows #3527

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
Merged
merged 1 commit into from
Aug 28, 2020
Merged

🌱 Add conditions for deletion workflows #3527

merged 1 commit into from
Aug 28, 2020

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
This PR adds conditions for providing evidence of the delete workflow for CAPI* and CAPD
*experimental objects are not included in this PR

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 25, 2020
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 25, 2020
fabriziopandini
fabriziopandini commented Aug 25, 2020
View reviewed changes
controllers/cluster_controller.go Outdated
@@ -162,6 +154,28 @@ func (r *ClusterReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, reterr e
return r.reconcile(ctx, cluster)
}

func patchCluster(patchHelper *patch.Helper, ctx context.Context, cluster *clusterv1.Cluster, options ...patch.Option) error {
Copy link
Member Author

@fabriziopandini fabriziopandini Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The Patch operation is becoming really verbose so I moved it to a separate func designed to be used also for issuing patches in the middle of the reconcile loop, if necessary
The same change applies to all the controllers

@fabriziopandini fabriziopandini force-pushed the reconcile-delete-conditions branch from 58c7e01 to d7a925e Compare August 25, 2020 13:34
vincepri
vincepri reviewed Aug 25, 2020
View reviewed changes
controlplane/kubeadm/controllers/controller.go
Comment on lines +217 to +239
conditions.SetSummary(kcp,
conditions.WithConditions(
controlplanev1.MachinesSpecUpToDateCondition,
controlplanev1.ResizedCondition,
controlplanev1.MachinesReadyCondition,
controlplanev1.AvailableCondition,
controlplanev1.CertificatesAvailableCondition,
),
)

// Patch the object, ignoring conflicts on the conditions owned by this controller.
return patchHelper.Patch(
ctx,
kcp,
patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
clusterv1.ReadyCondition,
controlplanev1.MachinesSpecUpToDateCondition,
controlplanev1.ResizedCondition,
controlplanev1.MachinesReadyCondition,
controlplanev1.AvailableCondition,
controlplanev1.CertificatesAvailableCondition,
}},
)
Copy link
Member

Choose a reason for hiding this comment

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

Seems there is a lot of repetition here, is there a way we could have a conditions.Owned method when doing the SetSummary

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a repetition between Summary and Owned conditions, but they are not the same.
However I agree there is a pattern that repeats across controllers, so this is probably something we can address within the context of #3517

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need owned conditions here? I guess this is actually going to be solved once we get server side apply in place

Copy link
Member Author

Choose a reason for hiding this comment

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

Owned conditions are required to avoid reconcile conflicts that can happen due to stale reads or to patch issued in the middle of a reconcile loop.
With server-side apply some of these problems a going away, but I'm not sure all of them.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, let's revisit later, I'm not 100% sure this is the direction we want to go long term, but seems fine for now

@fabriziopandini
Copy link
Member Author

/hold
investigating test failure

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 25, 2020
@fabriziopandini fabriziopandini force-pushed the reconcile-delete-conditions branch from d7a925e to fc03fea Compare August 25, 2020 15:47
@fabriziopandini
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 25, 2020
@fabriziopandini fabriziopandini mentioned this pull request Aug 26, 2020
Merged
detiber
detiber reviewed Aug 26, 2020
View reviewed changes
Copy link
Member

@detiber detiber left a comment

Choose a reason for hiding this comment

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

Overall, this looks great, just a few minor questions/suggestions.

@vincepri vincepri self-assigned this Aug 26, 2020
@vincepri
Copy link
Member

/milestone v0.3.9

@k8s-ci-robot k8s-ci-robot added this to the v0.3.9 milestone Aug 26, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2020
@fabriziopandini fabriziopandini force-pushed the reconcile-delete-conditions branch from cbe146a to cc8e664 Compare August 27, 2020 09:12
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2020
@detiber
Copy link
Member

detiber commented Aug 27, 2020

lgtm pending squash

@fabriziopandini fabriziopandini force-pushed the reconcile-delete-conditions branch from cc8e664 to f6441cb Compare August 27, 2020 14:40
@fabriziopandini
Copy link
Member Author

/test pull-cluster-api-e2e

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

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/assign @detiber @ncdc

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2020
@vincepri
Copy link
Member

/milestone v0.3.9

@fabriziopandini fabriziopandini force-pushed the reconcile-delete-conditions branch from c4427be to 061583b Compare August 28, 2020 16:30
@fabriziopandini
Copy link
Member Author

squashed commits

@vincepri
Copy link
Member

/test pull-cluster-api-test

@vincepri
Copy link
Member

/assign @CecileRobertMichon
for final lgtm

@CecileRobertMichon
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2020
@k8s-ci-robot k8s-ci-robot merged commit 506f16b into kubernetes-sigs:master Aug 28, 2020
@fabriziopandini fabriziopandini deleted the reconcile-delete-conditions branch August 31, 2020 10:26
@fabriziopandini fabriziopandini mentioned this pull request Oct 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncdc ncdc ncdc left review comments

@detiber detiber detiber left review comments

@vincepri vincepri vincepri approved these changes

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

Assignees

@ncdc ncdc

@detiber detiber

@vincepri vincepri

@CecileRobertMichon CecileRobertMichon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v0.3.9
Development

Successfully merging this pull request may close these issues.

Should we utilize conditions for deletion flows
6 participants
@fabriziopandini @vincepri @detiber @k8s-ci-robot @CecileRobertMichon @ncdc