Skip to content

Commit e9e49a7

Browse files
author
Ryan Zhang
committed
fix a bunch of issues
Signed-off-by: Ryan Zhang <[email protected]>
1 parent 0e9003a commit e9e49a7

File tree

6 files changed

+1080
-245
lines changed

6 files changed

+1080
-245
lines changed

‎pkg/controllers/clusterresourceplacement/controller.go‎

Lines changed: 43 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1056,7 +1056,7 @@ func (r *Reconciler) setPlacementStatus(
10561056
// For external rollout strategy, if clusters observe different resource snapshot versions,
10571057
// we set RolloutStarted to Unknown without any other conditions since we do not know exactly which version is rolling out.
10581058
// We also need to reset ObservedResourceIndex and selectedResources.
1059-
rolloutStartedUnknown, err := r.determineRolloutStateForCRPWithExternalRolloutStrategy(ctx, crp, selected, allRPS, selectedResourceIDs)
1059+
rolloutStartedUnknown, err := r.determineRolloutStateForPlacementWithExternalRolloutStrategy(ctx, placementObj, selected, allRPS, selectedResourceIDs)
10601060
if err != nil || rolloutStartedUnknown {
10611061
return true, err
10621062
}
@@ -1068,17 +1068,18 @@ func (r *Reconciler) setPlacementStatus(
10681068
return true, nil
10691069
}
10701070

1071-
func (r *Reconciler) determineRolloutStateForCRPWithExternalRolloutStrategy(
1071+
// TODO: This currently only works for cluster-scoped placements due to condition type differences
1072+
func (r *Reconciler) determineRolloutStateForPlacementWithExternalRolloutStrategy(
10721073
ctx context.Context,
1073-
crp *fleetv1beta1.ClusterResourcePlacement,
1074+
placementObj fleetv1beta1.PlacementObj,
10741075
selected []*fleetv1beta1.ClusterDecision,
10751076
allRPS []fleetv1beta1.ResourcePlacementStatus,
10761077
selectedResourceIDs []fleetv1beta1.ResourceIdentifier,
10771078
) (bool, error) {
10781079
if len(selected) == 0 {
10791080
// This should not happen as we already checked in setPlacementStatus.
1080-
err := controller.NewUnexpectedBehaviorError(fmt.Errorf("selected cluster list is empty for placement %s when checking per-cluster rollout state", crp.Name))
1081-
klog.ErrorS(err, "Should not happen: selected cluster list is empty in determineRolloutStateForCRPWithExternalRolloutStrategy()")
1081+
err := controller.NewUnexpectedBehaviorError(fmt.Errorf("selected cluster list is empty for placement %s when checking per-cluster rollout state", placementObj.GetName()))
1082+
klog.ErrorS(err, "Should not happen: selected cluster list is empty in determineRolloutStateForPlacementWithExternalRolloutStrategy()")
10821083
return false, err
10831084
}
10841085

@@ -1094,77 +1095,83 @@ func (r *Reconciler) determineRolloutStateForCRPWithExternalRolloutStrategy(
10941095
if differentResourceIndicesObserved {
10951096
// If clusters observe different resource snapshot versions, we set RolloutStarted condition to Unknown.
10961097
// ObservedResourceIndex and selectedResources are reset, too.
1097-
klog.V(2).InfoS("Placement has External rollout strategy and different resource snapshot versions are observed across clusters, set RolloutStarted condition to Unknown", "clusterResourcePlacement", klog.KObj(crp))
1098-
crp.Status.ObservedResourceIndex = ""
1099-
crp.Status.SelectedResources = []fleetv1beta1.ResourceIdentifier{}
1100-
crp.SetConditions(metav1.Condition{
1098+
klog.V(2).InfoS("Placement has External rollout strategy and different resource snapshot versions are observed across clusters, set RolloutStarted condition to Unknown", "placement", klog.KObj(placementObj))
1099+
placementStatus := placementObj.GetPlacementStatus()
1100+
placementStatus.ObservedResourceIndex = ""
1101+
placementStatus.SelectedResources = []fleetv1beta1.ResourceIdentifier{}
1102+
placementObj.SetConditions(metav1.Condition{
11011103
Type: string(fleetv1beta1.ClusterResourcePlacementRolloutStartedConditionType),
11021104
Status: metav1.ConditionUnknown,
11031105
Reason: condition.RolloutControlledByExternalControllerReason,
11041106
Message: "Rollout is controlled by an external controller and different resource snapshot versions are observed across clusters",
1105-
ObservedGeneration: crp.Generation,
1107+
ObservedGeneration: placementObj.GetGeneration(),
11061108
})
1107-
// As CRP status will refresh even if the spec has not changed, we reset any unused conditions
1108-
// to avoid confusion.
1109+
// As placement status will refresh even if the spec has not changed, we reset any unused conditions to avoid confusion.
11091110
for i := condition.RolloutStartedCondition + 1; i < condition.TotalCondition; i++ {
1110-
meta.RemoveStatusCondition(&crp.Status.Conditions, string(i.ClusterResourcePlacementConditionType()))
1111+
meta.RemoveStatusCondition(&placementStatus.Conditions, string(i.ClusterResourcePlacementConditionType()))
11111112
}
11121113
return true, nil
11131114
}
1114-
1115+
// all bindings have the same observed resource snapshot.
11151116
if observedResourceIndex == "" {
11161117
// All bindings have empty resource snapshot name, we set the rollout condition to Unknown.
11171118
// ObservedResourceIndex and selectedResources are reset, too.
1118-
klog.V(2).InfoS("Placement has External rollout strategy and no resource snapshot name is observed across clusters, set RolloutStarted condition to Unknown", "clusterResourcePlacement", klog.KObj(crp))
1119-
crp.Status.ObservedResourceIndex = ""
1120-
crp.Status.SelectedResources = []fleetv1beta1.ResourceIdentifier{}
1121-
crp.SetConditions(metav1.Condition{
1119+
klog.V(2).InfoS("Placement has External rollout strategy and no resource snapshot name is observed across clusters, set RolloutStarted condition to Unknown", "placement", klog.KObj(placementObj))
1120+
placementStatus := placementObj.GetPlacementStatus()
1121+
placementStatus.ObservedResourceIndex = ""
1122+
placementStatus.SelectedResources = []fleetv1beta1.ResourceIdentifier{}
1123+
placementObj.SetConditions(metav1.Condition{
11221124
Type: string(fleetv1beta1.ClusterResourcePlacementRolloutStartedConditionType),
11231125
Status: metav1.ConditionUnknown,
11241126
Reason: condition.RolloutControlledByExternalControllerReason,
11251127
Message: "Rollout is controlled by an external controller and no resource snapshot name is observed across clusters, probably rollout has not started yet",
1126-
ObservedGeneration: crp.Generation,
1128+
ObservedGeneration: placementObj.GetGeneration(),
11271129
})
1128-
// As CRP status will refresh even if the spec has not changed, we reset any unused conditions
1129-
// to avoid confusion.
1130+
// As placement status will refresh even if the spec has not changed, we reset any unused conditions to avoid confusion.
11301131
for i := condition.RolloutStartedCondition + 1; i < condition.TotalCondition; i++ {
1131-
meta.RemoveStatusCondition(&crp.Status.Conditions, string(i.ClusterResourcePlacementConditionType()))
1132+
meta.RemoveStatusCondition(&placementStatus.Conditions, string(i.ClusterResourcePlacementConditionType()))
11321133
}
1134+
11331135
return true, nil
11341136
}
11351137

11361138
// All bindings have the same observed resource snapshot.
11371139
// We only set the ObservedResourceIndex and selectedResources, as the conditions will be set with setCRPConditions.
11381140
// If all clusters observe the latest resource snapshot, we do not need to go through all the resource snapshots again to collect selected resources.
1139-
if observedResourceIndex == crp.Status.ObservedResourceIndex {
1140-
crp.Status.SelectedResources = selectedResourceIDs
1141+
placementStatus := placementObj.GetPlacementStatus()
1142+
if observedResourceIndex == placementStatus.ObservedResourceIndex {
1143+
placementStatus.SelectedResources = selectedResourceIDs
11411144
} else {
1142-
crp.Status.ObservedResourceIndex = observedResourceIndex
1143-
selectedResources, err := controller.CollectResourceIdentifiersFromResourceSnapshot(ctx, r.Client, crp.Name, observedResourceIndex)
1145+
placementStatus.ObservedResourceIndex = observedResourceIndex
1146+
// Construct placement key for the resource collection function
1147+
placementKey := placementObj.GetName()
1148+
if placementObj.GetNamespace() != "" {
1149+
placementKey = placementObj.GetNamespace() + "/" + placementObj.GetName()
1150+
}
1151+
selectedResources, err := controller.CollectResourceIdentifiersFromResourceSnapshot(ctx, r.Client, placementKey, observedResourceIndex)
11441152
if err != nil {
1145-
klog.ErrorS(err, "Failed to collect resource identifiers from clusterResourceSnapshot", "clusterResourcePlacement", klog.KObj(crp), "resourceSnapshotIndex", observedResourceIndex)
1153+
klog.ErrorS(err, "Failed to collect resource identifiers from resourceSnapshot", "placement", klog.KObj(placementObj), "resourceSnapshotIndex", observedResourceIndex)
11461154
return false, err
11471155
}
1148-
crp.Status.SelectedResources = selectedResources
1156+
placementStatus.SelectedResources = selectedResources
11491157
}
11501158

11511159
for i := range len(selected) {
11521160
rolloutStartedCond := meta.FindStatusCondition(allRPS[i].Conditions, string(fleetv1beta1.ResourceRolloutStartedConditionType))
1153-
if !condition.IsConditionStatusTrue(rolloutStartedCond, crp.Generation) &&
1154-
!condition.IsConditionStatusFalse(rolloutStartedCond, crp.Generation) {
1161+
if !condition.IsConditionStatusTrue(rolloutStartedCond, placementObj.GetGeneration()) &&
1162+
!condition.IsConditionStatusFalse(rolloutStartedCond, placementObj.GetGeneration()) {
11551163
klog.V(2).InfoS("Placement has External rollout strategy and some cluster is in RolloutStarted Unknown state, set RolloutStarted condition to Unknown",
1156-
"clusterName", allRPS[i].ClusterName, "observedResourceIndex", observedResourceIndex, "clusterResourcePlacement", klog.KObj(crp))
1157-
crp.SetConditions(metav1.Condition{
1164+
"clusterName", allRPS[i].ClusterName, "observedResourceIndex", observedResourceIndex, "placement", klog.KObj(placementObj))
1165+
placementObj.SetConditions(metav1.Condition{
11581166
Type: string(fleetv1beta1.ClusterResourcePlacementRolloutStartedConditionType),
11591167
Status: metav1.ConditionUnknown,
11601168
Reason: condition.RolloutControlledByExternalControllerReason,
11611169
Message: fmt.Sprintf("Rollout is controlled by an external controller and cluster %s is in RolloutStarted Unknown state", allRPS[i].ClusterName),
1162-
ObservedGeneration: crp.Generation,
1170+
ObservedGeneration: placementObj.GetGeneration(),
11631171
})
1164-
// As CRP status will refresh even if the spec has not changed, we reset any unused conditions
1165-
// to avoid confusion.
1172+
// As placement status will refresh even if the spec has not changed, we reset any unused conditions to avoid confusion.
11661173
for i := condition.RolloutStartedCondition + 1; i < condition.TotalCondition; i++ {
1167-
meta.RemoveStatusCondition(&crp.Status.Conditions, string(i.ClusterResourcePlacementConditionType()))
1174+
meta.RemoveStatusCondition(&placementStatus.Conditions, string(i.ClusterResourcePlacementConditionType()))
11681175
}
11691176
return true, nil
11701177
}

‎pkg/controllers/clusterresourceplacement/controller_test.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import (
3939
"sigs.k8s.io/controller-runtime/pkg/client/fake"
4040

4141
fleetv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
42+
"github.com/kubefleet-dev/kubefleet/pkg/utils"
4243
"github.com/kubefleet-dev/kubefleet/pkg/utils/condition"
4344
"github.com/kubefleet-dev/kubefleet/pkg/utils/controller"
4445
"github.com/kubefleet-dev/kubefleet/pkg/utils/defaulter"
@@ -3576,7 +3577,7 @@ func TestIsRolloutComplete(t *testing.T) {
35763577
}
35773578
}
35783579

3579-
func TestDetermineRolloutStateForCRPWithExternalRolloutStrategy(t *testing.T) {
3580+
func TestDetermineRolloutStateForPlacementWithExternalRolloutStrategy(t *testing.T) {
35803581
namespaceResourceContent := *resource.NamespaceResourceContentForTest(t)
35813582
deploymentResourceContent := *resource.DeploymentResourceContentForTest(t)
35823583

@@ -4427,23 +4428,24 @@ func TestDetermineRolloutStateForCRPWithExternalRolloutStrategy(t *testing.T) {
44274428
var cmpOptions = []cmp.Option{
44284429
// ignore the message as we may change the message in the future
44294430
cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime"),
4431+
cmpopts.SortSlices(utils.LessFuncResourceIdentifier),
44304432
}
4431-
gotRolloutUnknown, gotErr := r.determineRolloutStateForCRPWithExternalRolloutStrategy(context.Background(), crp, tc.selected, tc.allRPS, tc.selectedResources)
4433+
gotRolloutUnknown, gotErr := r.determineRolloutStateForPlacementWithExternalRolloutStrategy(context.Background(), crp, tc.selected, tc.allRPS, tc.selectedResources)
44324434
if (gotErr != nil) != tc.wantErr {
4433-
t.Errorf("determineRolloutStateForCRPWithExternalRolloutStrategy() got error %v, want error %t", gotErr, tc.wantErr)
4435+
t.Errorf("determineRolloutStateForPlacementWithExternalRolloutStrategy() got error %v, want error %t", gotErr, tc.wantErr)
44344436
}
44354437
if !tc.wantErr {
44364438
if gotRolloutUnknown != tc.wantRolloutUnknown {
4437-
t.Errorf("determineRolloutStateForCRPWithExternalRolloutStrategy() got RolloutUnknown set to %v, want %v", gotRolloutUnknown, tc.wantRolloutUnknown)
4439+
t.Errorf("determineRolloutStateForPlacementWithExternalRolloutStrategy() got RolloutUnknown set to %v, want %v", gotRolloutUnknown, tc.wantRolloutUnknown)
44384440
}
44394441
if crp.Status.ObservedResourceIndex != tc.wantObservedResourceIndex {
4440-
t.Errorf("determineRolloutStateForCRPWithExternalRolloutStrategy() got crp.Status.ObservedResourceIndex set to %v, want %v", crp.Status.ObservedResourceIndex, tc.wantObservedResourceIndex)
4442+
t.Errorf("determineRolloutStateForPlacementWithExternalRolloutStrategy() got crp.Status.ObservedResourceIndex set to %v, want %v", crp.Status.ObservedResourceIndex, tc.wantObservedResourceIndex)
44414443
}
4442-
if diff := cmp.Diff(tc.wantSelectedResources, crp.Status.SelectedResources); diff != "" {
4443-
t.Errorf("determineRolloutStateForCRPWithExternalRolloutStrategy() got crp.Status.SelectedResources mismatch (-want, +got):\n%s", diff)
4444+
if diff := cmp.Diff(tc.wantSelectedResources, crp.Status.SelectedResources, cmpOptions...); diff != "" {
4445+
t.Errorf("determineRolloutStateForPlacementWithExternalRolloutStrategy() got crp.Status.SelectedResources mismatch (-want, +got):\n%s", diff)
44444446
}
44454447
if diff := cmp.Diff(tc.wantConditions, crp.Status.Conditions, cmpOptions...); diff != "" {
4446-
t.Errorf("determineRolloutStateForCRPWithExternalRolloutStrategy() got crp.Status.Conditions mismatch (-want, +got):\n%s", diff)
4448+
t.Errorf("determineRolloutStateForPlacementWithExternalRolloutStrategy() got crp.Status.Conditions mismatch (-want, +got):\n%s", diff)
44474449
}
44484450
}
44494451
})

0 commit comments

Comments
 (0)