[BOP-85][BOP-87] Add events and status for Addon and Manifest CRs #16

tppolkow · 2023-11-23T23:18:37Z

Add events & status to Addon & Manifest Objects.

k describe addon calico -n boundless-system
...
Status:
  Last Transition Time:  2023-11-27T19:00:55Z
  Reason:                Manifest Addon Created
  Type:                  Available
Events:
  Type    Reason            Age                From              Message
  ----    ------            ----               ----              -------
  Normal  SuccessfulCreate  10s (x2 over 10s)  addon controller  Created Manifest Addon boundless-system/calico

k describe manifest calico -n boundless-system
...
Status:
  Last Transition Time:  2023-11-27T19:00:55Z
  Reason:                Manifest Created
  Type:                  Available
Events:
  Type    Reason            Age   From                 Message
  ----    ------            ----  ----                 -------
  Normal  SuccessfulCreate  75s   manifest controller  Created Manifest boundless-system/calico

The events are also annotated with Addon label. We can tie all events for addon with below command. This will be useful by the CLI to get events for a specific addon.

k get events -A -o=jsonpath='{.items[?(@.metadata.annotations.Addon=="calico")]}' | jq
{
  "apiVersion": "v1",
  "count": 2,
  "eventTime": null,
  "firstTimestamp": "2023-11-27T19:00:55Z",
  "involvedObject": {
    "apiVersion": "boundless.mirantis.com/v1alpha1",
    "kind": "Addon",
    "name": "calico",
    "namespace": "boundless-system",
    "resourceVersion": "669",
    "uid": "3e35d63e-16e0-4134-8f47-ddfc193074e1"
  },
  "kind": "Event",
  "lastTimestamp": "2023-11-27T19:00:55Z",
  "message": "Created Manifest Addon boundless-system/calico",
  "metadata": {
    "annotations": {
      "Addon": "calico"
    },
    "creationTimestamp": "2023-11-27T19:00:55Z",
    "managedFields": [
      {
        "apiVersion": "v1",
        "fieldsType": "FieldsV1",
        "fieldsV1": {
          "f:count": {},
          "f:firstTimestamp": {},
          "f:involvedObject": {},
          "f:lastTimestamp": {},
          "f:message": {},
          "f:metadata": {
            "f:annotations": {
              ".": {},
              "f:Addon": {}
            }
          },
          "f:reason": {},
          "f:source": {
            "f:component": {}
          },
          "f:type": {}
        },
        "manager": "main",
        "operation": "Update",
        "time": "2023-11-27T19:00:55Z"
      }
    ],
    "name": "calico.179b9009b44cce20",
    "namespace": "boundless-system",
    "resourceVersion": "752",
    "uid": "143793d2-fd94-4470-8118-6930f4e5f254"
  },
  "reason": "SuccessfulCreate",
  "reportingComponent": "",
  "reportingInstance": "",
  "source": {
    "component": "addon controller"
  },
  "type": "Normal"
}
{
  "apiVersion": "v1",
  "count": 1,
  "eventTime": null,
  "firstTimestamp": "2023-11-27T19:00:55Z",
  "involvedObject": {
    "apiVersion": "boundless.mirantis.com/v1alpha1",
    "kind": "Manifest",
    "name": "calico",
    "namespace": "boundless-system",
    "resourceVersion": "684",
    "uid": "ffb87316-070b-44fd-b267-5e77900f8283"
  },
  "kind": "Event",
  "lastTimestamp": "2023-11-27T19:00:55Z",
  "message": "Created Manifest boundless-system/calico",
  "metadata": {
    "annotations": {
      "Addon": "calico"
    },
    "creationTimestamp": "2023-11-27T19:00:55Z",
    "managedFields": [
      {
        "apiVersion": "v1",
        "fieldsType": "FieldsV1",
        "fieldsV1": {
          "f:count": {},
          "f:firstTimestamp": {},
          "f:involvedObject": {},
          "f:lastTimestamp": {},
          "f:message": {},
          "f:metadata": {
            "f:annotations": {
              ".": {},
              "f:Addon": {}
            }
          },
          "f:reason": {},
          "f:source": {
            "f:component": {}
          },
          "f:type": {}
        },
        "manager": "main",
        "operation": "Update",
        "time": "2023-11-27T19:00:55Z"
      }
    ],
    "name": "calico.179b9009c05f2708",
    "namespace": "boundless-system",
    "resourceVersion": "786",
    "uid": "e425cfb2-eff9-4654-9b79-19b18699e3c6"
  },
  "reason": "SuccessfulCreate",
  "reportingComponent": "",
  "reportingInstance": "",
  "source": {
    "component": "manifest controller"
  },
  "type": "Normal"
}

Status fields for both manifest and addon objects is also reported when we get the resource. Status is specific to the specific CR, status of child CRs does not affect the parent Addon status.

tpolkowski@tpolkowski-MBP16-1947 boundless-cli % k get addon -A
NAMESPACE          NAME         STATUS
boundless-system   calico       Available
boundless-system   metallb      Available
boundless-system   my-grafana   Available
tpolkowski@tpolkowski-MBP16-1947 boundless-cli % k get manifest -A
NAMESPACE          NAME      STATUS
boundless-system   calico    Available
boundless-system   metallb   Degraded

 k describe manifest metallb -n boundless-system
...
Status:
  Last Transition Time:  2023-11-27T19:00:55Z
  Message:               Failure in http get request : 404
  Reason:                Failure in http get request
  Type:                  Degraded
Events:
  Type     Reason        Age    From                 Message
  ----     ------        ----   ----                 -------
  Warning  FailedCreate  6m55s  manifest controller  Failed to Create Manifest boundless-system/metallb : failure in http get request response code : 404

sakshisharma84 · 2023-11-28T17:37:08Z

api/v1alpha1/addon_types.go

+	ComponentProgressing StatusConditionType = "Progressing"
+
+	// ComponentDegraded means the component is not operating as desired and user action is required.
+	ComponentDegraded StatusConditionType = "Degraded"


Degraded sounds a bit confusing. Can we change it to something else?
It can be easily misinterpreted as the addon is degraded to a lower version. What do you think?

We should add Unhealthy also. Degraded is used for something running slow or in a lower version (as Sakshi mentioned). Unhealthy is for something not running at all.

Also, since these are standard k8s status terms, are we sure they aren't available in a library so we don't have to duplicate them?

I couldn't find these defined anywhere in at least the k8s standard libraries.

I'm not against adding Unhealthy status, I didn't see it being used in kubebuilder anywhere or in calico operator where I got most of these definitions from. I think having both is fine so will add it.

api/v1alpha1/addon_types.go

sakshisharma84 · 2023-11-28T18:06:37Z

@tppolkow Would it make sense to wait for the manifest update support as well? I don't intend to delay this PR. Just wanted to bring to your notice so that we don't miss it.

tppolkow · 2023-11-28T18:44:16Z

@tppolkow Would it make sense to wait for the manifest update support as well? I don't intend to delay this PR. Just wanted to bring to your notice so that we don't miss it.

I think we can just merge whichever is ready first, I can do a second pass for events/status updates related to manifest updates together with the PR I will put up for HelmChart events/statuses once your PR is merged.

ranyodh · 2023-11-28T20:02:16Z

controllers/addon_controller.go

@@ -63,6 +69,11 @@ func (r *AddonReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 		return ctrl.Result{}, err
 	}

+	// only update the status to progressing if its not already set to not trigger infinite reconciliations


question: is this something that you ran into while developing? According to the kubebuilder documentation, updates to status should not trigger a reconcile as this does not update the generation of the object.

suggestion: If the reconcile is still triggered, we should use the following predicate to filter changes where generation has not been changed:

// SetupWithManager sets up the controller with the Manager. func (r *AddonReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&boundlessv1alpha1.Addon{}). WithEventFilter(predicate.GenerationChangedPredicate{}). Complete(r) }

Reference:

https://github.com/kubernetes-sigs/controller-runtime/blob/v0.16.3/pkg/predicate/predicate.go#L156

https://sdk.operatorframework.io/docs/building-operators/golang/references/event-filtering/

Yes it is an issue I discovered during development. I tested it again now to double check and the operator is infinitely reconciling without this condition. Essentially each status update triggers a reconcile , which in turn updates the status, which results in another reconcile thus the infinite loop. I will look into the event filter option, agree it seems much better.

The predicate works to prevent the reconciliation issue. I've added it for now, but think we will need to update it when we implement the approach you suggested in your other comment since we will want reconciliation triggered from updates to child manifests/helmcharts.

ranyodh · 2023-11-28T20:43:34Z

controllers/addon_controller.go

@@ -63,6 +70,11 @@ func (r *AddonReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 		return ctrl.Result{}, err
 	}

+	// only update the status to progressing if its not already set to not trigger infinite reconciliations
+	if instance.Status.Type == "" {
+		r.updateStatus(ctx, logger, req.NamespacedName, boundlessv1alpha1.TypeComponentProgressing, "Creating Addon")


thought: I think we should avoid updating status at this (and other such stages). Controller should only do one status update at the end.

To implement the same behavior, controller should watch the the resources it needs to update its Status field. So, AddonController should watch HelmChart and Manifest CRD. And then check for their statuses to populate its own status.

I think HelmChart and Manifest should be owned by AddonController (https://book.kubebuilder.io/reference/watching-resources/operator-managed). Then it will get called for those objects too.
Or alternatively, this can Watch HelmChart.Status and Manifest.Status (if that is possible).

References:

https://cloud.redhat.com/blog/kubernetes-operators-best-practices

https://medium.com/@gallettilance/10-things-you-should-know-before-writing-a-kubernetes-controller-83de8f86d659

This does not need to prevent merging this PR, but we should create a followup task or @todo to look into this.

This approach makes sense to me for Addons - essentially we have the Addon react to changes in status from child Helmcharts and Manifests. Are you thinking this approach be limited to AddonController ... whereas HelmChart and Manifest do statuses more explicitly ? Since for Manifest and HelmCharts we don't know beforehand what resources are created.

Added a @todo, will take this up in follow up PR once helmchart statuses are figured out and we have updates for manifest working.

api/v1alpha1/addon_types.go

controllers/manifest_controller.go

api/v1alpha1/manifest_types.go

nwneisen

LGTM

Refactor AddonStatus & ManifestStatus to use common Status; Add unhealthy status type BOP-85/BOP-87 Cleanup imports Add predicate to prevent infinite reconcilations Improve manifest event messages; Add delay for requeues due to issues reaching manifest url

tppolkow force-pushed the BOP-85 branch 3 times, most recently from 1c7a20b to e527127 Compare November 27, 2023 18:57

tppolkow changed the title ~~WIP: BOP-85/87~~ [BOP-85][BOP-87] Add events and status for Addon and Manifest CRs Nov 27, 2023

tppolkow marked this pull request as ready for review November 27, 2023 19:09

tppolkow requested review from sakshisharma84, ranyodh and nwneisen November 27, 2023 19:09

sakshisharma84 reviewed Nov 28, 2023

View reviewed changes

nwneisen reviewed Nov 28, 2023

View reviewed changes

api/v1alpha1/addon_types.go Show resolved Hide resolved

api/v1alpha1/addon_types.go Outdated Show resolved Hide resolved

ranyodh reviewed Nov 28, 2023

View reviewed changes

api/v1alpha1/addon_types.go Show resolved Hide resolved

tppolkow force-pushed the BOP-85 branch from 3698d59 to 064425f Compare November 28, 2023 23:41

nwneisen reviewed Nov 29, 2023

View reviewed changes

controllers/manifest_controller.go Outdated Show resolved Hide resolved

controllers/manifest_controller.go Outdated Show resolved Hide resolved

nwneisen reviewed Nov 29, 2023

View reviewed changes

api/v1alpha1/manifest_types.go Show resolved Hide resolved

nwneisen approved these changes Nov 29, 2023

View reviewed changes

ranyodh approved these changes Nov 29, 2023

View reviewed changes

tppolkow force-pushed the BOP-85 branch from 7957ddd to ce00e7a Compare November 29, 2023 21:19

tppolkow added 2 commits November 29, 2023 16:35

Fix issues after rebase

9343e39

tppolkow force-pushed the BOP-85 branch from ce00e7a to 9343e39 Compare November 29, 2023 23:03

tppolkow merged commit 8d3325b into Mirantis:main Nov 29, 2023
3 checks passed

tppolkow deleted the BOP-85 branch November 29, 2023 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BOP-85][BOP-87] Add events and status for Addon and Manifest CRs #16

[BOP-85][BOP-87] Add events and status for Addon and Manifest CRs #16

tppolkow commented Nov 23, 2023 •

edited

Loading

sakshisharma84 Nov 28, 2023

nwneisen Nov 28, 2023 •

edited

Loading

nwneisen Nov 28, 2023

tppolkow Nov 28, 2023 •

edited

Loading

tppolkow Nov 28, 2023

sakshisharma84 commented Nov 28, 2023

tppolkow commented Nov 28, 2023

ranyodh Nov 28, 2023

tppolkow Nov 28, 2023

tppolkow Nov 28, 2023

ranyodh Nov 28, 2023

tppolkow Nov 28, 2023

nwneisen left a comment

[BOP-85][BOP-87] Add events and status for Addon and Manifest CRs #16

[BOP-85][BOP-87] Add events and status for Addon and Manifest CRs #16

Conversation

tppolkow commented Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

nwneisen Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tppolkow Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakshisharma84 commented Nov 28, 2023

tppolkow commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwneisen left a comment

Choose a reason for hiding this comment

tppolkow commented Nov 23, 2023 •

edited

Loading

nwneisen Nov 28, 2023 •

edited

Loading

tppolkow Nov 28, 2023 •

edited

Loading