Skip to content

✨ Default Probes Refactor #2586

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

Open
dtfranz wants to merge 1 commit into
base: main
Choose a base branch
from
+245 −92
Open

✨ Default Probes Refactor #2586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions api/v1/clusterextensionrevision_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,14 @@ const (
type ProbeType string

const (
ProbeTypeFieldCondition ProbeType = "ConditionEqual"
ProbeTypeFieldEqual ProbeType = "FieldsEqual"
ProbeTypeConditionEqual ProbeType = "ConditionEqual"
ProbeTypeFieldsEqual ProbeType = "FieldsEqual"
ProbeTypeFieldValue ProbeType = "FieldValue"

// Deprecated: use ProbeTypeConditionEqual instead.
ProbeTypeFieldCondition = ProbeTypeConditionEqual
// Deprecated: use ProbeTypeFieldsEqual instead.
ProbeTypeFieldEqual = ProbeTypeFieldsEqual
)

// Assertion is a discriminated union which defines the probe type and definition used as an assertion.
Expand Down
111 changes: 110 additions & 1 deletion internal/operator-controller/applier/boxcutter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import (

"helm.sh/helm/v3/pkg/release"
"helm.sh/helm/v3/pkg/storage/driver"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -208,10 +211,12 @@ func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
annotations[labels.ServiceAccountNamespaceKey] = ext.Spec.Namespace

phases := PhaseSort(objects)
progressionProbes := defaultProgressionProbes()

spec := ocv1ac.ClusterExtensionRevisionSpec().
WithLifecycleState(ocv1.ClusterExtensionRevisionLifecycleStateActive).
WithPhases(phases...)
WithPhases(phases...).
WithProgressionProbes(progressionProbes...)
if p := ext.Spec.ProgressDeadlineMinutes; p > 0 {
spec.WithProgressDeadlineMinutes(p)
}
Expand Down Expand Up @@ -602,6 +607,110 @@ func latestRevisionNumber(prevRevisions []ocv1.ClusterExtensionRevision) int64 {
return prevRevisions[len(prevRevisions)-1].Spec.Revision
}

func defaultProgressionProbes() []*ocv1ac.ProgressionProbeApplyConfiguration {
crdProbe := ocv1ac.ProgressionProbe().
WithSelector(ocv1ac.ObjectSelector().
WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: "apiextensions.k8s.io",
Kind: "CustomResourceDefinition",
})).
WithAssertions(ocv1ac.Assertion().
WithType(ocv1.ProbeTypeConditionEqual).
WithConditionEqual(
ocv1ac.ConditionEqualProbe().
WithType(string(apiextensions.Established)).
WithStatus(string(corev1.ConditionTrue))))

// Checks if the Type: "Ready" Condition is "True"
readyConditionAssertion := ocv1ac.Assertion().
WithType(ocv1.ProbeTypeConditionEqual).
WithConditionEqual(
ocv1ac.ConditionEqualProbe().
WithType("Ready").
WithStatus("True"))

certProbe := ocv1ac.ProgressionProbe().
WithSelector(ocv1ac.ObjectSelector().
WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: "acme.cert-manager.io",
Kind: "Certificate",
})).
Comment on lines +636 to +639
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The default cert-manager Certificate probe uses selector group "acme.cert-manager.io", but the repo’s manifests and other logic (e.g., applier/phase.go) use the standard "cert-manager.io" API group for Certificate. With the current group, this probe will never select cert-manager Certificate objects, so the intended readiness gate won’t run.

Copilot uses AI. Check for mistakes.
WithAssertions(readyConditionAssertion)
issuerProbe := ocv1ac.ProgressionProbe().
WithSelector(ocv1ac.ObjectSelector().
WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: "acme.cert-manager.io",
Kind: "Issuer",
})).
Comment on lines +644 to +647
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The default cert-manager Issuer probe uses selector group "acme.cert-manager.io", but the repo’s cert-manager objects are in the "cert-manager.io" group. As written, this probe won’t match Issuer resources and will effectively be a no-op readiness check.

Copilot uses AI. Check for mistakes.
WithAssertions(readyConditionAssertion)

// namespaceActiveProbe is a probe which asserts that the namespace is in "Active" phase
namespaceActiveProbe := ocv1ac.ProgressionProbe().
WithSelector(ocv1ac.ObjectSelector().
WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: corev1.GroupName,
Kind: "Namespace",
})).
WithAssertions(ocv1ac.Assertion().
WithType(ocv1.ProbeTypeFieldValue).
WithFieldValue(ocv1ac.FieldValueProbe().
WithFieldPath("status.phase").
WithValue(string(corev1.NamespaceActive))))

// pvcBoundProbe is a probe which asserts that the PVC is in "Bound" phase
pvcBoundProbe := ocv1ac.ProgressionProbe().
WithSelector(ocv1ac.ObjectSelector().
WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: corev1.GroupName,
Kind: "PersistentVolumeClaim",
})).
WithAssertions(ocv1ac.Assertion().
WithType(ocv1.ProbeTypeFieldValue).
WithFieldValue(ocv1ac.FieldValueProbe().
WithFieldPath("status.phase").
WithValue(string(corev1.ClaimBound))))

// Checks if the Type: "Available" Condition is "True".
availableConditionAssertion := ocv1ac.Assertion().
WithType(ocv1.ProbeTypeConditionEqual).
WithConditionEqual(ocv1ac.ConditionEqualProbe().
WithType(string(appsv1.DeploymentAvailable)).
WithStatus(string(corev1.ConditionTrue)))

// Checks if status.updatedReplicas == status.replicas.
// Works for StatefulSets, Deployments and ReplicaSets.
replicasUpdatedAssertion := ocv1ac.Assertion().
WithType(ocv1.ProbeTypeFieldsEqual).
WithFieldsEqual(ocv1ac.FieldsEqualProbe().
WithFieldA("status.updatedReplicas").
WithFieldB("status.replicas"))

statefulSetProbe := ocv1ac.ProgressionProbe().WithSelector(
ocv1ac.ObjectSelector().WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: appsv1.GroupName,
Kind: "StatefulSet",
}),
).WithAssertions(replicasUpdatedAssertion, availableConditionAssertion)

deploymentProbe := ocv1ac.ProgressionProbe().WithSelector(
ocv1ac.ObjectSelector().WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: appsv1.GroupName,
Kind: "Deployment",
}),
).WithAssertions(replicasUpdatedAssertion, availableConditionAssertion)

return []*ocv1ac.ProgressionProbeApplyConfiguration{
deploymentProbe, statefulSetProbe, pvcBoundProbe, namespaceActiveProbe, issuerProbe, certProbe, crdProbe,
}
}

func splitManifestDocuments(file string) []string {
// Estimate: typical manifests have ~50-100 lines per document
// Pre-allocate for reasonable bundle size to reduce allocations
Expand Down
101 changes: 100 additions & 1 deletion internal/operator-controller/applier/boxcutter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
apierrors "k8s.io/apimachinery/pkg/api/errors"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -143,7 +144,105 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T)
},
},
}),
),
)).
WithProgressionProbes(
ocv1ac.ProgressionProbe().WithSelector(
ocv1ac.ObjectSelector().WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: appsv1.GroupName,
Kind: "Deployment",
})).
WithAssertions(
ocv1ac.Assertion().
WithType(ocv1.ProbeTypeFieldsEqual).
WithFieldsEqual(ocv1ac.FieldsEqualProbe().
WithFieldA("status.updatedReplicas").
WithFieldB("status.replicas")),
ocv1ac.Assertion().
WithType(ocv1.ProbeTypeConditionEqual).
WithConditionEqual(ocv1ac.ConditionEqualProbe().
WithType(string(appsv1.DeploymentAvailable)).
WithStatus(string(corev1.ConditionTrue)))),
ocv1ac.ProgressionProbe().WithSelector(
ocv1ac.ObjectSelector().WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: appsv1.GroupName,
Kind: "StatefulSet",
})).
WithAssertions(
ocv1ac.Assertion().
WithType(ocv1.ProbeTypeFieldsEqual).
WithFieldsEqual(ocv1ac.FieldsEqualProbe().
WithFieldA("status.updatedReplicas").
WithFieldB("status.replicas")),
ocv1ac.Assertion().
WithType(ocv1.ProbeTypeConditionEqual).
WithConditionEqual(ocv1ac.ConditionEqualProbe().
WithType(string(appsv1.DeploymentAvailable)).
WithStatus(string(corev1.ConditionTrue)))),
ocv1ac.ProgressionProbe().
WithSelector(ocv1ac.ObjectSelector().
WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: corev1.GroupName,
Kind: "PersistentVolumeClaim",
})).
WithAssertions(ocv1ac.Assertion().
WithType(ocv1.ProbeTypeFieldValue).
WithFieldValue(ocv1ac.FieldValueProbe().
WithFieldPath("status.phase").
WithValue(string(corev1.ClaimBound)))),
ocv1ac.ProgressionProbe().
WithSelector(ocv1ac.ObjectSelector().
WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: corev1.GroupName,
Kind: "Namespace",
})).
WithAssertions(ocv1ac.Assertion().
WithType(ocv1.ProbeTypeFieldValue).
WithFieldValue(ocv1ac.FieldValueProbe().
WithFieldPath("status.phase").
WithValue(string(corev1.NamespaceActive)))),
ocv1ac.ProgressionProbe().
WithSelector(ocv1ac.ObjectSelector().
WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: "acme.cert-manager.io",
Kind: "Issuer",
})).
Comment on lines +210 to +213
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

Test expectation for the default Issuer probe is using group "acme.cert-manager.io", but the codebase’s cert-manager resources are "cert-manager.io". Update the expected selector group to match the correct API group so this test doesn’t encode the wrong default behavior.

Copilot uses AI. Check for mistakes.
WithAssertions(ocv1ac.Assertion().
WithType(ocv1.ProbeTypeConditionEqual).
WithConditionEqual(
ocv1ac.ConditionEqualProbe().
WithType("Ready").
WithStatus("True"))),
ocv1ac.ProgressionProbe().
WithSelector(ocv1ac.ObjectSelector().
WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: "acme.cert-manager.io",
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

Test expectation for the default Certificate probe is using group "acme.cert-manager.io", but the manifests in this repo use "cert-manager.io" for Certificate. Update the expected selector group so the test matches the intended (and correct) default probes.

Suggested change
Group: "acme.cert-manager.io",
Group: "cert-manager.io",

Copilot uses AI. Check for mistakes.
Kind: "Certificate",
})).
WithAssertions(ocv1ac.Assertion().
WithType(ocv1.ProbeTypeConditionEqual).
WithConditionEqual(
ocv1ac.ConditionEqualProbe().
WithType("Ready").
WithStatus("True"))),
ocv1ac.ProgressionProbe().
WithSelector(ocv1ac.ObjectSelector().
WithType(ocv1.SelectorTypeGroupKind).
WithGroupKind(metav1.GroupKind{
Group: "apiextensions.k8s.io",
Kind: "CustomResourceDefinition",
})).
WithAssertions(ocv1ac.Assertion().
WithType(ocv1.ProbeTypeConditionEqual).
WithConditionEqual(
ocv1ac.ConditionEqualProbe().
WithType(string(apiextensions.Established)).
WithStatus(string(corev1.ConditionTrue)))),
),
)
assert.Equal(t, expected, rev)
Expand Down
94 changes: 6 additions & 88 deletions internal/operator-controller/controllers/clusterextensionrevision_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@ import (
"strings"
"time"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -479,9 +476,7 @@ func (c *ClusterExtensionRevisionReconciler) buildBoxcutterPhases(ctx context.Co

opts := []boxcutter.RevisionReconcileOption{
boxcutter.WithPreviousOwners(previousObjs),
boxcutter.WithProbe(boxcutter.ProgressProbeType, probing.And{
&namespaceActiveProbe, deploymentProbe, statefulSetProbe, crdProbe, issuerProbe, certProbe, &pvcBoundProbe, progressionProbes,
}),
boxcutter.WithProbe(boxcutter.ProgressProbeType, progressionProbes),
}

phases := make([]boxcutter.Phase, 0)
Expand Down Expand Up @@ -535,10 +530,10 @@ func buildProgressionProbes(progressionProbes []ocv1.ProgressionProbe) (probing.
for _, probe := range progressionProbe.Assertions {
switch probe.Type {
// Switch based on the union discriminator
case ocv1.ProbeTypeFieldCondition:
case ocv1.ProbeTypeConditionEqual:
conditionProbe := probing.ConditionProbe(probe.ConditionEqual)
assertions = append(assertions, &conditionProbe)
case ocv1.ProbeTypeFieldEqual:
case ocv1.ProbeTypeFieldsEqual:
fieldsEqualProbe := probing.FieldsEqualProbe(probe.FieldsEqual)
assertions = append(assertions, &fieldsEqualProbe)
case ocv1.ProbeTypeFieldValue:
Expand Down Expand Up @@ -570,90 +565,13 @@ func buildProgressionProbes(progressionProbes []ocv1.ProgressionProbe) (probing.
default:
return nil, fmt.Errorf("unknown progressionProbe selector type: %s", progressionProbe.Selector.Type)
}
userProbes = append(userProbes, selectorProbe)
userProbes = append(userProbes, &probing.ObservedGenerationProbe{
Prober: selectorProbe,
})
Comment on lines +568 to +570
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

buildProgressionProbes now wraps every user-defined selector probe in probing.ObservedGenerationProbe unconditionally. That changes semantics for probes that don’t have/maintain status.observedGeneration (e.g., many core resources), and there’s currently no API field on ocv1.ProgressionProbe to opt in/out despite the PR description mentioning a flag. Consider adding an explicit boolean (e.g., progressionProbe.observedGeneration) and only wrapping when it’s true (default false), then set it on the default probes that need it.

Suggested change
userProbes = append(userProbes, &probing.ObservedGenerationProbe{
Prober: selectorProbe,
})
userProbes = append(userProbes, selectorProbe)

Copilot uses AI. Check for mistakes.
}
return userProbes, nil
}

var (
deploymentProbe = &probing.GroupKindSelector{
GroupKind: schema.GroupKind{Group: appsv1.GroupName, Kind: "Deployment"},
Prober: deplStatefulSetProbe,
}
statefulSetProbe = &probing.GroupKindSelector{
GroupKind: schema.GroupKind{Group: appsv1.GroupName, Kind: "StatefulSet"},
Prober: deplStatefulSetProbe,
}
crdProbe = &probing.GroupKindSelector{
GroupKind: schema.GroupKind{Group: "apiextensions.k8s.io", Kind: "CustomResourceDefinition"},
Prober: &probing.ObservedGenerationProbe{
Prober: &probing.ConditionProbe{ // "Available" == "True"
Type: string(apiextensions.Established),
Status: string(corev1.ConditionTrue),
},
},
}
certProbe = &probing.GroupKindSelector{
GroupKind: schema.GroupKind{Group: "acme.cert-manager.io", Kind: "Certificate"},
Prober: &probing.ObservedGenerationProbe{
Prober: readyConditionProbe,
},
}
issuerProbe = &probing.GroupKindSelector{
GroupKind: schema.GroupKind{Group: "acme.cert-manager.io", Kind: "Issuer"},
Prober: &probing.ObservedGenerationProbe{
Prober: readyConditionProbe,
},
}

// namespaceActiveProbe is a probe which asserts that the namespace is in "Active" phase
namespaceActiveProbe = probing.GroupKindSelector{
GroupKind: schema.GroupKind{Group: corev1.GroupName, Kind: "Namespace"},
Prober: &applier.FieldValueProbe{
FieldPath: "status.phase",
Value: "Active",
},
}

// pvcBoundProbe is a probe which asserts that the PVC is in "Bound" phase
pvcBoundProbe = probing.GroupKindSelector{
GroupKind: schema.GroupKind{Group: corev1.GroupName, Kind: "PersistentVolumeClaim"},
Prober: &applier.FieldValueProbe{
FieldPath: "status.phase",
Value: "Bound",
},
}

// deplStaefulSetProbe probes Deployment, StatefulSet objects.
deplStatefulSetProbe = &probing.ObservedGenerationProbe{
Prober: probing.And{
availableConditionProbe,
replicasUpdatedProbe,
},
}

// Checks if the Type: "Available" Condition is "True".
availableConditionProbe = &probing.ConditionProbe{ // "Available" == "True"
Type: string(appsv1.DeploymentAvailable),
Status: string(corev1.ConditionTrue),
}

// Checks if the Type: "Ready" Condition is "True"
readyConditionProbe = &probing.ObservedGenerationProbe{
Prober: &probing.ConditionProbe{
Type: "Ready",
Status: "True",
},
}

// Checks if .status.updatedReplicas == .status.replicas.
// Works for StatefulSts, Deployments and ReplicaSets.
replicasUpdatedProbe = &probing.FieldsEqualProbe{
FieldA: ".status.updatedReplicas",
FieldB: ".status.replicas",
}
)

func setRetryingConditions(cer *ocv1.ClusterExtensionRevision, message string) {
markAsProgressing(cer, ocv1.ClusterExtensionRevisionReasonRetrying, message)
if meta.FindStatusCondition(cer.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable) != nil {
Expand Down
Loading
Loading