Skip to content
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

🌱 Audit patch withOwnedConditions #11396

Merged
merged 1 commit into from
Nov 11, 2024
+58 −4
Merged

🌱 Audit patch withOwnedConditions #11396

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
14 changes: 14 additions & 0 deletions internal/controllers/cluster/cluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,20 @@ func patchCluster(ctx context.Context, patchHelper *patch.Helper, cluster *clust
clusterv1.ControlPlaneReadyCondition,
clusterv1.InfrastructureReadyCondition,
}},
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
clusterv1.ClusterInfrastructureReadyV1Beta2Condition,
clusterv1.ClusterControlPlaneAvailableV1Beta2Condition,
clusterv1.ClusterControlPlaneInitializedV1Beta2Condition,
clusterv1.ClusterWorkersAvailableV1Beta2Condition,
clusterv1.ClusterMachinesReadyV1Beta2Condition,
clusterv1.ClusterMachinesUpToDateV1Beta2Condition,
clusterv1.ClusterRemoteConnectionProbeV1Beta2Condition,
clusterv1.ClusterScalingUpV1Beta2Condition,
clusterv1.ClusterScalingDownV1Beta2Condition,
clusterv1.ClusterRemediatingV1Beta2Condition,
clusterv1.ClusterDeletingV1Beta2Condition,
clusterv1.ClusterAvailableV1Beta2Condition,
}},
)
return patchHelper.Patch(ctx, cluster, options...)
}
Expand Down
48 changes: 44 additions & 4 deletions internal/controllers/machinehealthcheck/machinehealthcheck_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
defer func() {
// Always attempt to patch the object and status after each reconciliation.
// Patch ObservedGeneration only if the reconciliation completed successfully
patchOpts := []patch.Option{}
patchOpts := []patch.Option{
patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
clusterv1.RemediationAllowedCondition,
}},
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
clusterv1.MachineHealthCheckRemediationAllowedV1Beta2Condition,
}},
}
if reterr == nil {
patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
}
Expand Down Expand Up @@ -300,7 +307,18 @@ func (r *Reconciler) reconcile(ctx context.Context, logger logr.Logger, cluster
}
errList := []error{}
for _, t := range append(healthy, unhealthy...) {
Copy link
Member

Choose a reason for hiding this comment

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

Not new with this PR, but I'm really confused why we are patching all Machines twice (if I'm reading this correctly)

Copy link
Member Author

Choose a reason for hiding this comment

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

My current understanding is that we are patching here only if remediation is not allowed (and we patch both unhealthy and healthy machines).

The other two patch machines are for when remediation is allowed, one for healthy machines, the other for unhealthy.

But if you want we can take a deeper look together

Copy link
Member

Choose a reason for hiding this comment

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

Let's take a look together later. To me it looks like some of these patches are always no-ops

if err := t.patchHelper.Patch(ctx, t.Machine); err != nil {
patchOpts := []patch.Option{
patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
clusterv1.MachineHealthCheckSucceededCondition,
// Note: intentionally leaving out OwnerRemediated condition which is mostly controlled by the owner.
}},
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
clusterv1.MachineHealthCheckSucceededV1Beta2Condition,
// Note: intentionally leaving out OwnerRemediated condition which is mostly controlled by the owner.
// (Same for ExternallyRemediated condition)
}},
}
if err := t.patchHelper.Patch(ctx, t.Machine, patchOpts...); err != nil {
errList = append(errList, errors.Wrapf(err, "failed to patch machine status for machine: %s/%s", t.Machine.Namespace, t.Machine.Name))
continue
}
Expand Down Expand Up @@ -380,7 +398,18 @@ func (r *Reconciler) patchHealthyTargets(ctx context.Context, logger logr.Logger
}
}

if err := t.patchHelper.Patch(ctx, t.Machine); err != nil {
patchOpts := []patch.Option{
patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
clusterv1.MachineHealthCheckSucceededCondition,
// Note: intentionally leaving out OwnerRemediated condition which is mostly controlled by the owner.
}},
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
clusterv1.MachineHealthCheckSucceededV1Beta2Condition,
// Note: intentionally leaving out OwnerRemediated condition which is mostly controlled by the owner.
// (Same for ExternallyRemediated condition)
}},
}
if err := t.patchHelper.Patch(ctx, t.Machine, patchOpts...); err != nil {
logger.Error(err, "failed to patch healthy machine status for machine", "Machine", klog.KObj(t.Machine))
errList = append(errList, errors.Wrapf(err, "failed to patch healthy machine status for machine: %s/%s", t.Machine.Namespace, t.Machine.Name))
}
Expand Down Expand Up @@ -486,7 +515,18 @@ func (r *Reconciler) patchUnhealthyTargets(ctx context.Context, logger logr.Logg
}
}

if err := t.patchHelper.Patch(ctx, t.Machine); err != nil {
patchOpts := []patch.Option{
patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
clusterv1.MachineHealthCheckSucceededCondition,
// Note: intentionally leaving out OwnerRemediated condition which is mostly controlled by the owner.
}},
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
clusterv1.MachineHealthCheckSucceededV1Beta2Condition,
// Note: intentionally leaving out OwnerRemediated condition which is mostly controlled by the owner.
// (Same for ExternallyRemediated condition)
}},
}
if err := t.patchHelper.Patch(ctx, t.Machine, patchOpts...); err != nil {
errList = append(errList, errors.Wrapf(err, "failed to patch unhealthy machine status for machine: %s/%s", t.Machine.Namespace, t.Machine.Name))
continue
}
Expand Down