MTSRE-259 | integrate status reporting the hard way

[yashvardhan-kukreja]

Implementation details

main.go runs a central heartbeat reporter loop whose purpose is to periodically report the heartbeat which the addon intends to report. This central heartbeat reporter loop is setup via the following way

reference-addon/cmd/reference-addon-manager/main.go

Lines 98 to 107 in ecfb850

	// the following section hooks up a heartbeat reporter with the current addon/operator
	r := controllers.ReferenceAddonReconciler{
	Client: mgr.GetClient(),
	Log: ctrl.Log.WithName("controllers").WithName("ReferenceAddon"),
	Scheme: mgr.GetScheme(),
	}
	// setup the heartbeat reporter
	if err := addoninstancesdk.SetupHeartbeatReporter(&r, mgr); err != nil {
	setupLog.Error(err, "unable to setup heartbeat reporter")
	os.Exit(1)

reference-addon/internal/addoninstancesdk/sdk.go

Line 53 in ecfb850

func SetupHeartbeatReporter(r ReconcilerWithHeartbeat, mgr manager.Manager) error {

ReconcilerWithHeartbeat is just an interface which can be implemented by a Reconciler struct which happens to have some additional methods required for heartbeat reporting to happen. More on ReconcilerWithHeartbeat later :)

With every iteration of the heartbeat reporter loop, latest heartbeat is reported concurrently

reference-addon/internal/addoninstancesdk/sdk.go

Lines 79 to 111 in ecfb850

	// Heartbeat reporter section: report a heartbeat at an interval ('currentAddonInstanceConfiguration.HeartbeatUpdatePeriod' seconds)
	// 1. Each iteration spins up a go-routine, where that go-routine (concurrently) sends the heartbeat and syncs the 'currentAddonInstanceConfiguration' with the latest one.
	// 2. Spinning up go-routines here because that would end up ensuring that the for-loop runs almost exactly at a periodic rate of 'current heartbeatUpdatePeriod' seconds,
	// instead of running at a rate of (current heartbeatUpdatePeriod + time to send latest heartbeat + time to sync `currentAddonInstanceConfiguration` with latest one) seconds.
	for {
	go func(stop chan error) {
	if err := SendLatestHeartbeat(ctx, r); err != nil {
	mgr.GetLogger().Error(err, "error occurred while sending the latest heartbeat")
	}
	addonInstanceConfigurationMutex.Lock()
	defer addonInstanceConfigurationMutex.Unlock()
	latestAddonInstanceConfiguration, err := getAddonInstanceConfiguration(ctx, mgr.GetClient(), addonName, r.GetAddonTargetNamespace())
	if err != nil {
	// TODO(ykukreja): report error instead and stop the heartbeat reporter loop? (instead of `return`, write `stop <- err`)
	mgr.GetLogger().Error(err, fmt.Sprintf("failed to get the AddonInstance configuration corresponding to the Addon '%s'", addonName))
	return
	}
	currentAddonInstanceConfiguration = latestAddonInstanceConfiguration
	}(stop)
	select {
	// stop this heartbeat reporter loop if any of the above go-routines endup raising an error/exit
	case err := <-stop:
	mgr.GetLogger().Error(err, "failed to report certain heartbeats")
	return err
	default:
	// waiting for heartbeat update period for executing the next iteration
	<-time.After(currentAddonInstanceConfiguration.HeartbeatUpdatePeriod.Duration)
	}
	}
	}

btw, a heartbeat is a metav1.Condition object only.

ReconcilerWithHeartbeat is essentially any Reconciler struct which implements some additional methods which are required for by the heartbeat reporter.

reference-addon/internal/addoninstancesdk/sdk.go

Lines 25 to 31 in ecfb850

	type ReconcilerWithHeartbeat interface {
	reconcile.Reconciler
	GetAddonName() string // so as to give the addon developer the freedom to return addon's name the way they want: through a direct hardcoded string or through env var populated by downwards API
	GetAddonTargetNamespace() string // so as to give the addon developer the freedom to return targetNamespace the way they want: through a direct hardcoded string or through env var populated by downwards API
	HandleAddonInstanceConfigurationChanges(newAddonInstanceSpec addonsv1alpha1.AddonInstanceSpec) error
	GetClient() client.Client
	}

From the perspective of ReferenceAddonReconciler

The ReferenceAddonReconciler implements the ReconcilerWithHeartbeat by having the following methods:

reference-addon/internal/controllers/reference_addon_controller.go

Lines 57 to 74 in ecfb850

	func (r ReferenceAddonReconciler) GetAddonName() string {
	return "reference-addon"
	}
	func (r ReferenceAddonReconciler) GetAddonTargetNamespace() string {
	// assuming ADDON_TARGET_NAMESPACE would be populated via downwards API in the deployment spec of the reference addon
	return os.Getenv("ADDON_TARGET_NAMESPACE")
	}
	func (r ReferenceAddonReconciler) GetClient() client.Client {
	return r.Client
	}
	// the following 'HandleAddonInstanceConfigurationChanges' method can be absolutely anything depending how reference-addon would want to deal with AddonInstance's configuration change
	func (r *ReferenceAddonReconciler) HandleAddonInstanceConfigurationChanges(newAddonInstanceSpec addonsv1alpha1.AddonInstanceSpec) error {
	fmt.Println("Handling AddonInstance's configuration changes, whooossh!!!")
	return nil
	}

And various sections of Reference Addon's codebase can use the addoninstancesdk.SetAndSendHeartbeat(..., heartbeat) function to report the heartbeat which is supposed to be reported. For example, if there's a section in the codebase that spots an error which correlates to the addon being unhealthy, then, that section of codebase can use addoninstancesdk.SetAndSendHeartbeat(r, someUnhealthyHeartbeat) to do so.

For example:

reference-addon/internal/controllers/reference_addon_controller.go

Lines 43 to 52 in ecfb850

	// if the ReferenceAddon object getting reconciled has the name "reference-addon", only then report a successful heartbeat
	if strings.HasPrefix(req.NamespacedName.Name, "reference-addon") || strings.HasPrefix(req.NamespacedName.Name, "redhat-") {
	if err := addoninstancesdk.SetAndSendHeartbeat(r, successfulCondition); err != nil {
	return ctrl.Result{}, fmt.Errorf("failed to set and sent heartbeat: %w", err)
	}
	} else {
	if err := addoninstancesdk.SetAndSendHeartbeat(r, failureCondition); err != nil {
	return ctrl.Result{}, fmt.Errorf("failed to set and sent heartbeat: %w", err)
	}
	}

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yashvardhan-kukreja
To complete the pull request process, please ask for approval from thetechnick after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[yashvardhan-kukreja]

/retest

[yashvardhan-kukreja]

Todo: if the current approach of heartbeat reporting (r.latestHeartbeat, r.GetLatestHeartbeat(), r.SetLatestHeartbeat()) gets approved, then we can proceed with adding concurrency safety to ReferenceAddonReconciler.latestHeartbeat via mutex

[thetechnick]

This file is in dire need of some unittests :)

[yashvardhan-kukreja]

Yes! I planned to write the unit tests after the implementation was accepted for this status reporting. Otherwise, after every review, the tests would have required to get refactored as well :)

[yashvardhan-kukreja]

/hold

[openshift-ci]

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

Test name	Commit	Details	Required	Rerun command
ci/prow/lint	03e33c1	link	true	/test lint

Full PR test history. Your PR dashboard.

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.

[yashvardhan-kukreja]

closing in favor of #8 and #9