Align controller and API structure across repositories

[maximiliantech]

Description

This issue will be used for discussion around topics for controller and API alignment through our openMCP repositories. The issue description will be updated continuously.

Decisions that we agreed upon:

• Switch from own ConditionList implementation to []metav1.Condition 👍

• CommonStatus struct will be placed in api/common directory, e.g. Align controller and API structure across repositories #70 (comment) 👍

• ObjectReference and NamespacedObjectReference will be replaced by https://github.com/kubernetes/apimachinery/blob/v0.33.2/pkg/types/namespacedname.go#L27 + defining aliases, e.g. Align controller and API structure across repositories #70 (comment) 👍

• We define concrete Phases of a Cluster resource 👍

  • Ready -> something is successul
  • Progressing -> else progressing (creating/updating)
  • Terminating -> when deleting
  • e.g. Align controller and API structure across repositories #70 (comment)

• Emit event when phase changes, decision done with Align controller and API structure across repositories #70 (comment) 👍

• last reconcile time can be deleted 👍

• using openmcp-operator as best practice for all other repos to come 👍

[reshnm]

Example common status:

package common

import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

const (
	// StatusPhaseReady indicates that the resource is ready. All conditions are met and are in status "True".
	StatusPhaseReady = "Ready"
	// StatusPhaseProgressing indicates that the resource is not ready and being created or updated. At least one condition is not met and is in status "False".
	StatusPhaseProgressing = "Progressing"
	// StatusPhaseTerminating indicates that the resource is not ready and in deletion. At least one condition is not met and is in status "False".
	StatusPhaseTerminating = "Terminating"
)

// Status represents the common status for openMCP resources.
type Status struct {
	// ObservedGeneration represents the .metadata.generation that the status was set based upon.
	ObservedGeneration int64 `json:"observedGeneration,omitempty"`

	// Conditions is a list of conditions that apply to the resource.
	Conditions []metav1.Condition `json:"conditions,omitempty"`

	// Phase indicates the current phase of the resource.
	Phase string `json:"phase,omitempty"`
}

[reshnm]

Example references:

package common

import (
	corev1 "k8s.io/api/core/v1"
	apimachinery "k8s.io/apimachinery/pkg/types"
)

// ObjectReference is a reference to an object in any namespace.
type ObjectReference apimachinery.NamespacedName

// LocalObjectReference is a reference to an object in the same namespace as the resource referencing it.
type LocalObjectReference corev1.LocalObjectReference

// SecretReference is a reference to a secret in any namespace with a key.
type SecretReference struct {
	ObjectReference `json:",inline"`
	// Key is the key in the secret to use.
	Key string `json:"key"`
}

// LocalSecretReference is a reference to a secret in the same namespace as the resource referencing it with a key.
type LocalSecretReference struct {
	LocalObjectReference `json:",inline"`
	// Key is the key in the secret to use.
	Key string `json:"key"`
}

[reshnm]

Question: The functions apimeta.SetStatusCondition and apimeta.RemoveStatusCondition are returning a boolean to indicate wether the conditions have been changed. Should we also emit an Event in case of a change to the Conditions?

@maximiliantech @Diaphteiros

[reshnm]

Condition Status Handlung

Status Values:

• True: The controller was able to check the condition and decided that it is in the expected state
• False: The controller was able to check the condition and decided that it is NOT in the expected state
• Unknown: The controller was not able to check the condition (maybe due to an error unrelated to the condition)

[reshnm]

We do not set conditions preemptively, but only when the actiual check for the condition happened. Means we wont pre-fill all possible conditions at the start of the reconcile.

[Diaphteiros]

We do not set conditions preemptively, but only when the actiual check for the condition happened. Means we wont pre-fill all possible conditions at the start of the reconcile.

The disadvantage of this approach is that conditions will appear and disappear in error cases.

I would prefer having all the conditions there every time and just have the ones that could not be evaluated set to unknown, but this logic is somewhat annoying to implement. It's not a strong preference though, I would also be fine with doing it the way your proposed.

[maximiliantech]

Question: The functions apimeta.SetStatusCondition and apimeta.RemoveStatusCondition are returning a boolean to indicate wether the conditions have been changed. Should we also emit an Event in case of a change to the Conditions?

@ValentinGerlach @reshnm @maximiliantech ... agreed that whenever a condition changes its status there should be an event that states the name of the condition, the previous and the new value
The same goes for the Phase: Phase changed from X to Y.

[maximiliantech]

The disadvantage of this approach is that conditions will appear and disappear in error cases.

@ValentinGerlach They would not disappear. To disappear, we need to actively remove them. So that should be no problem.

[Diaphteiros]

@maximiliantech OK, but then it will be a mix of up-to-date and outdated conditions and one always has to check which ones are the currently relevant ones.

[ValentinGerlach]

@maximiliantech OK, but then it will be a mix of up-to-date and outdated conditions and one always has to check which ones are the currently relevant ones.

That's OK in my opinion. Each condition has an ObservedGeneration that helps to understand if a condition is out of date. The documentation says:

For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date with respect to the current state of the instance.

I would not remove "unknown"/out-of-date conditions automatically on every iteration because we might have cases where two controllers reconcile the same resource. They would remove each others "unknown" conditions then

[Diaphteiros]

Question regarding #70 (comment):

Our cluster api uses ClusterRequest and AccessRequest. The Phases for both resources are currently named Granted, Denied, and Pending, which fits better in the context of requests than Ready, Terminating, and Progressing.

What do we do with the request phases?

Option 1:

We also use the default phases Ready, Progressing, and Terminating for it.

Advantage: No additional phases.

Disadvantage: The situation that a request has been denied and will never be 'ready' cannot be expressed with the default phases. This is currently just a theoretical problem though, as our controllers never deny a request at the moment.

Option 2:

We keep the request phases Granted, Pending, and Denied and define them in the cluster api package, as it is right now.

Option 3:

We keep the current request phases, but move them into the common package. This option is preferred if we expect to have further 'Request' resource types later on.

---

WDYT?

[Diaphteiros]

Question: The functions apimeta.SetStatusCondition and apimeta.RemoveStatusCondition are returning a boolean to indicate wether the conditions have been changed. Should we also emit an Event in case of a change to the Conditions?

@reshnm
Are you thinking of one event per reconciliation if any status condition changed or one event per changed status condition (which could be multiple ones per reconciliation)?

I have the feeling that the latter option would be more helpful during debugging, but it would also produce a lot of events, which can easily become too much for resources with lots of conditions ...

[Diaphteiros]

I started implementing some of the changes discussed here: #79