✨ Conditions for OpenStackMachines #1288

apricote · 2022-06-30T10:06:29Z

What this PR does / why we need it:

Adds Conditions to the OpenStackMachineStatus.

See this example clusterctl describe output, that showcases the different conditions, and how they "bubble up" to the Machine resources:

So far there are two conditions:

InstanceReady (related to the OpenStack Instance) and
APIServerIngressReadyCondition (for ControlPlane machines -> LoadBalancer Member or Floating IP attachment)

I set the severity based on:

Info, if the reason is expected (missing bootstrap secret)
Warning, if the condition is eventually retried (no call to handleUpdateMachineError)
Error if handleUpdateMachineError was called and the machine is no longer retried/reconciled

The Ready condition is set to the aggregate of thhe two custom conditions, and the CAPI Machine resource "bubbles up" to Ready message, so any errors can be observed easily, without having to inspect the OpenStackMachine

Which issue(s) this PR fixes:
Fixes #832
Replaces #972

Open Tasks:

~~Tests for OpenStackMachineController~~ Requires proper envtest mocks
Fix APIServerIngressReadyCondition on non-control-plane machines when using api loadbalancer (see 🌱 only reconcile loadbalancer member if machine is control-plane #1294)

Open Questions:

Should we add the conditions to all existing api versions (because its a backwards compatible change) or only to the most recent version (v1alpha6)?

Pro: Makes conversion a no-op
Pro: Changes are backwards compatible
Contra: Changing old API versions feels wrong to me

/hold

k8s-ci-robot · 2022-06-30T10:06:31Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

netlify · 2022-06-30T10:06:45Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`e91d60b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/62e2731de0f85800082c6d19
😎 Deploy Preview	https://deploy-preview-1288--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

tobiasgiese

Implementing conditions is a really huge improvement, tbh! I like it.
Some nits, but @mdbooth can certainly help better here 🙂

controllers/openstackmachine_controller.go

apricote · 2022-07-11T08:43:46Z

@tobiasgiese Implemented your suggestions and tried to make other messages consistent with them.

General status update:

there is still a bug where non-control-plane machines get the APIServerIngressReady condition, this will be fixed by 🌱 only reconcile loadbalancer member if machine is control-plane #1294 and I plan to rebase this PR once its merged
Tests are currently hard to add, because we are missing proper envtest mocks (Envtest mocks #1236), I would be fine with merging this without adding unit tests.

seanschneeweiss · 2022-07-18T21:39:54Z

This PR is such a great addon for CAPO. Thanks a lot.

Should we add the conditions to all existing api versions (because its a backwards compatible change) or only to the most recent version (v1alpha6)?

I think it is totally fine to add it to the recent version only.

apricote · 2022-07-25T12:27:18Z

I rebased the PR against the current main commit. The bug where the APIServerIngressReady condition was added to non-CP machines is fixed now.

The PR is ready for review.

apricote · 2022-07-26T07:28:05Z

ERROR: (gcloud.compute.instances.create) Could not fetch resource:

The zone 'projects/k8s-boskos-gce-project-12/zones/us-east4-a' does not have enough resources available to fulfill the request. '(resource type:compute)'.

/retest

seanschneeweiss

/lgtm
👍

tobiasgiese

All in all this looks pretty solid 👌🏻
/approve

controllers/openstackmachine_controller.go

apricote · 2022-07-27T09:14:48Z

/hold cancel

apricote · 2022-07-27T09:33:30Z

Tests will be fixed by #1313. I will rebase once that PR is merged.

seanschneeweiss

~~I wonder if the severity of some conditions should be considered as Warning and not as Error. WDYT?~~

seanschneeweiss · 2022-07-29T14:16:41Z

controllers/openstackmachine_controller.go

 		return ctrl.Result{}, nil
 	case infrav1.InstanceStateDeleted:
 		// we should avoid further actions for DELETED VM
 		scope.Logger.Info("Instance state is DELETED, no actions")
+		conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeletedReason, clusterv1.ConditionSeverityError, "")


This might be an expected state, so don't know if we should really considers this error severity.

seanschneeweiss · 2022-07-29T14:55:45Z

I wonder if the severity of some conditions should be considered as Warning and not as Error. WDYT?

Oh sorry, I guess the severity can be adapted as soon as the handleUpdateMachineError is removed in #1116.

k8s-ci-robot · 2022-07-29T15:05:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apricote, seanschneeweiss, tobiasgiese

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~~ [seanschneeweiss,tobiasgiese]

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

tobiasgiese · 2022-07-29T15:14:57Z

/lgtm

IMO we should check wether it is required (machine == control-plane) before calling `loadbalancer.Service#ReconcileLoadBalancerMember`. The service should not be concerned with the type of machine it is called on. This also fixes one of the open bugs in PR kubernetes-sigs#1288.

k8s-ci-robot requested review from mdbooth and tobiasgiese June 30, 2022 10:06

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 30, 2022

mdbooth self-assigned this Jun 30, 2022

tobiasgiese reviewed Jun 30, 2022

View reviewed changes

controllers/openstackmachine_controller.go Outdated Show resolved Hide resolved

controllers/openstackmachine_controller.go Outdated Show resolved Hide resolved

controllers/openstackmachine_controller.go Outdated Show resolved Hide resolved

tobiasgiese mentioned this pull request Jul 2, 2022

Implementation of Conditions feature in Openstack Machine Controller #972

Closed

apricote mentioned this pull request Jul 11, 2022

🌱 only reconcile loadbalancer member if machine is control-plane #1294

Merged

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

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

apricote marked this pull request as ready for review July 25, 2022 12:26

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 Jul 25, 2022

k8s-ci-robot requested a review from tobiasgiese July 25, 2022 12:26

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 26, 2022

seanschneeweiss reviewed Jul 26, 2022

View reviewed changes

k8s-ci-robot assigned seanschneeweiss Jul 26, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2022

tobiasgiese approved these changes Jul 27, 2022

View reviewed changes

controllers/openstackmachine_controller.go Outdated Show resolved Hide resolved

controllers/openstackmachine_controller.go Outdated Show resolved Hide resolved

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 27, 2022

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

feat: Conditions for OpenStackMachine

e91d60b

apricote requested a review from seanschneeweiss July 29, 2022 13:21

seanschneeweiss reviewed Jul 29, 2022

View reviewed changes

seanschneeweiss approved these changes Jul 29, 2022

View reviewed changes

k8s-ci-robot assigned tobiasgiese Jul 29, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2022

k8s-ci-robot merged commit 6859acb into kubernetes-sigs:main Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Conditions for OpenStackMachines #1288

✨ Conditions for OpenStackMachines #1288

apricote commented Jun 30, 2022 •

edited

Loading

k8s-ci-robot commented Jun 30, 2022

netlify bot commented Jun 30, 2022 •

edited

Loading

tobiasgiese left a comment •

edited

Loading

apricote commented Jul 11, 2022

seanschneeweiss commented Jul 18, 2022

apricote commented Jul 25, 2022

apricote commented Jul 26, 2022

seanschneeweiss left a comment

tobiasgiese left a comment

apricote commented Jul 27, 2022

apricote commented Jul 27, 2022

seanschneeweiss left a comment •

edited

Loading

seanschneeweiss Jul 29, 2022

seanschneeweiss commented Jul 29, 2022

k8s-ci-robot commented Jul 29, 2022

tobiasgiese commented Jul 29, 2022

✨ Conditions for OpenStackMachines #1288

✨ Conditions for OpenStackMachines #1288

Conversation

apricote commented Jun 30, 2022 • edited Loading

k8s-ci-robot commented Jun 30, 2022

netlify bot commented Jun 30, 2022 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

tobiasgiese left a comment • edited Loading

Choose a reason for hiding this comment

apricote commented Jul 11, 2022

seanschneeweiss commented Jul 18, 2022

apricote commented Jul 25, 2022

apricote commented Jul 26, 2022

seanschneeweiss left a comment

Choose a reason for hiding this comment

tobiasgiese left a comment

Choose a reason for hiding this comment

apricote commented Jul 27, 2022

apricote commented Jul 27, 2022

seanschneeweiss left a comment • edited Loading

Choose a reason for hiding this comment

seanschneeweiss Jul 29, 2022

Choose a reason for hiding this comment

seanschneeweiss commented Jul 29, 2022

k8s-ci-robot commented Jul 29, 2022

tobiasgiese commented Jul 29, 2022

apricote commented Jun 30, 2022 •

edited

Loading

netlify bot commented Jun 30, 2022 •

edited

Loading

tobiasgiese left a comment •

edited

Loading

seanschneeweiss left a comment •

edited

Loading