Skip to content

Commit 6306c83

Browse files
author
Ryan Zhang
committed
address comments
Signed-off-by: Ryan Zhang <[email protected]>
1 parent 23c55bc commit 6306c83

File tree

12 files changed

+816
-206
lines changed

12 files changed

+816
-206
lines changed

apis/placement/v1beta1/commons.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ const (
109109
ParentNamespaceLabel = fleetPrefix + "parent-placement-namespace"
110110

111111
// CRPGenerationAnnotation indicates the generation of the CRP from which an object is derived or last updated.
112+
// TODO: rename this variable
112113
CRPGenerationAnnotation = fleetPrefix + "CRP-generation"
113114

114115
// EnvelopeConfigMapAnnotation indicates the configmap is an envelope configmap containing resources we need to apply to the member cluster instead of the configMap itself.

pkg/controllers/clusterresourceplacement/controller.go

Lines changed: 47 additions & 57 deletions
Large diffs are not rendered by default.

pkg/controllers/clusterresourceplacement/controller_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3569,7 +3569,7 @@ func TestIsRolloutComplete(t *testing.T) {
35693569
Conditions: tc.conditions,
35703570
},
35713571
}
3572-
got := isRolloutCompleted(crp)
3572+
got := isPlacementRolloutCompleted(crp)
35733573
if got != tc.want {
35743574
t.Errorf("isRolloutCompleted() got %v, want %v", got, tc.want)
35753575
}

pkg/controllers/clusterresourceplacement/placement_status.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,9 +97,9 @@ func appendFailedToScheduleResourcePlacementStatuses(
9797
return allRPS
9898
}
9999

100-
// determineExpectedCRPAndResourcePlacementStatusCondType determines the expected condition types for the CRP and resource placement statuses
100+
// determineExpectedPlacementAndResourcePlacementStatusCondType determines the expected condition types for the CRP and resource placement statuses
101101
// given the currently in-use apply strategy.
102-
func determineExpectedCRPAndResourcePlacementStatusCondType(placementObj fleetv1beta1.PlacementObj) []condition.ResourceCondition {
102+
func determineExpectedPlacementAndResourcePlacementStatusCondType(placementObj fleetv1beta1.PlacementObj) []condition.ResourceCondition {
103103
placementSpec := placementObj.GetPlacementSpec()
104104
switch {
105105
case placementSpec.Strategy.ApplyStrategy == nil:
@@ -113,6 +113,7 @@ func determineExpectedCRPAndResourcePlacementStatusCondType(placementObj fleetv1
113113

114114
// appendScheduledResourcePlacementStatuses appends the resource placement statuses for the
115115
// scheduled clusters to the list of all resource placement statuses.
116+
// it returns the updated list of resource placement statuses.
116117
func (r *Reconciler) appendScheduledResourcePlacementStatuses(
117118
ctx context.Context,
118119
allRPS []fleetv1beta1.ResourcePlacementStatus,
@@ -196,8 +197,9 @@ func (r *Reconciler) appendScheduledResourcePlacementStatuses(
196197
return allRPS, rpsSetCondTypeCounter, nil
197198
}
198199

199-
// setCRPConditions sets the CRP conditions based on the resource placement statuses.
200-
func setCRPConditions(
200+
// TODO: make this work with RP
201+
// setPlacementConditions currently sets the CRP conditions based on the resource placement statuses.
202+
func setPlacementConditions(
201203
placementObj fleetv1beta1.PlacementObj,
202204
allRPS []fleetv1beta1.ResourcePlacementStatus,
203205
rpsSetCondTypeCounter [condition.TotalCondition][condition.TotalConditionStatus]int,

pkg/controllers/rollout/controller.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -500,7 +500,7 @@ func determineBindingsToUpdate(
500500
readyBindings, canBeReadyBindings, canBeUnavailableBindings []placementv1beta1.BindingObj,
501501
) ([]toBeUpdatedBinding, []toBeUpdatedBinding) {
502502
toBeUpdatedBindingList := make([]toBeUpdatedBinding, 0)
503-
// TODO: Fix the bug that we don't shrink to zero when there are bindings that are not
503+
// TODO: Fix the bug that we don't shrink to zero when there are bindings that are not ready yet.
504504
// calculate the max number of bindings that can be unavailable according to user specified maxUnavailable
505505
maxNumberToRemove := calculateMaxToRemove(placementObj, targetNumber, readyBindings, canBeUnavailableBindings)
506506
// we can still update the bindings that are failed to apply already regardless of the maxNumberToRemove

pkg/scheduler/framework/framework.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -763,7 +763,7 @@ func (f *framework) updatePolicySnapshotStatusFromBindings(
763763
policyRef := klog.KObj(policy)
764764

765765
// Retrieve the corresponding CRP generation.
766-
observedCRPGeneration, err := annotations.ExtractObservedCRPGenerationFromPolicySnapshot(policy)
766+
observedCRPGeneration, err := annotations.ExtractObservedPlacementGenerationFromPolicySnapshot(policy)
767767
if err != nil {
768768
klog.ErrorS(err, "Failed to retrieve CRP generation from annotation", "clusterSchedulingPolicySnapshot", policyRef)
769769
return controller.NewUnexpectedBehaviorError(err)
@@ -1344,7 +1344,7 @@ func (f *framework) updatePolicySnapshotStatusForPickFixedPlacementType(
13441344
policyRef := klog.KObj(policy)
13451345

13461346
// Retrieve the corresponding CRP generation.
1347-
observedCRPGeneration, err := annotations.ExtractObservedCRPGenerationFromPolicySnapshot(policy)
1347+
observedCRPGeneration, err := annotations.ExtractObservedPlacementGenerationFromPolicySnapshot(policy)
13481348
if err != nil {
13491349
klog.ErrorS(err, "Failed to retrieve CRP generation from annotation", "clusterSchedulingPolicySnapshot", policyRef)
13501350
return controller.NewUnexpectedBehaviorError(err)

pkg/utils/annotations/annotations.go

Lines changed: 28 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -42,34 +42,44 @@ func ExtractNumOfClustersFromPolicySnapshot(policy fleetv1beta1.PolicySnapshotOb
4242
return numOfClusters, nil
4343
}
4444

45-
// ExtractObservedCRPGenerationFromPolicySnapshot extracts the observed CRP generation from policySnapshot.
46-
func ExtractObservedCRPGenerationFromPolicySnapshot(policy fleetv1beta1.PolicySnapshotObj) (int64, error) {
47-
crpGenerationStr, ok := policy.GetAnnotations()[fleetv1beta1.CRPGenerationAnnotation]
48-
if !ok {
49-
return 0, fmt.Errorf("cannot find annotation %s", fleetv1beta1.CRPGenerationAnnotation)
45+
// ExtractSubindexFromResourceSnapshot is a helper function to extract subindex from ResourceSnapshot objects.
46+
func ExtractSubindexFromResourceSnapshot(snapshot fleetv1beta1.ResourceSnapshotObj) (doesExist bool, subindex int, err error) {
47+
annotations := snapshot.GetAnnotations()
48+
if annotations == nil {
49+
return false, 0, nil
5050
}
5151

52-
// Cast the annotation to an integer; throw an error if the cast cannot be completed or the value is negative.
53-
observedCRPGeneration, err := strconv.Atoi(crpGenerationStr)
54-
if err != nil || observedCRPGeneration < 0 {
55-
return 0, fmt.Errorf("invalid annotation %s: %s is not a valid value: %w", fleetv1beta1.CRPGenerationAnnotation, crpGenerationStr, err)
52+
subindexStr, exists := annotations[fleetv1beta1.SubindexOfResourceSnapshotAnnotation]
53+
if !exists || subindexStr == "" {
54+
return false, 0, nil
5655
}
5756

58-
return int64(observedCRPGeneration), nil
57+
subindex, err = strconv.Atoi(subindexStr)
58+
if err != nil {
59+
return false, 0, fmt.Errorf("invalid subindex annotation value %q: %w", subindexStr, err)
60+
}
61+
62+
if subindex < 0 {
63+
return false, 0, fmt.Errorf("subindex cannot be negative: %d", subindex)
64+
}
65+
66+
return true, subindex, nil
5967
}
6068

61-
// ExtractSubindexFromClusterResourceSnapshot extracts the subindex value from the annotations of a clusterResourceSnapshot.
62-
func ExtractSubindexFromClusterResourceSnapshot(snapshot fleetv1beta1.ResourceSnapshotObj) (doesExist bool, subindex int, err error) {
63-
subindexStr, ok := snapshot.GetAnnotations()[fleetv1beta1.SubindexOfResourceSnapshotAnnotation]
69+
// ExtractObservedPlacementGenerationFromPolicySnapshot extracts the observed placement generation from policySnapshot.
70+
func ExtractObservedPlacementGenerationFromPolicySnapshot(policy fleetv1beta1.PolicySnapshotObj) (int64, error) {
71+
placementGenerationStr, ok := policy.GetAnnotations()[fleetv1beta1.CRPGenerationAnnotation]
6472
if !ok {
65-
return false, -1, nil
73+
return 0, fmt.Errorf("cannot find annotation %s", fleetv1beta1.CRPGenerationAnnotation)
6674
}
67-
subindex, err = strconv.Atoi(subindexStr)
68-
if err != nil || subindex < 0 {
69-
return true, -1, fmt.Errorf("invalid annotation %s: %s is invalid: %w", fleetv1beta1.SubindexOfResourceSnapshotAnnotation, subindexStr, err)
75+
76+
// Cast the annotation to an integer; throw an error if the cast cannot be completed or the value is negative.
77+
observedplacementGeneration, err := strconv.Atoi(placementGenerationStr)
78+
if err != nil || observedplacementGeneration < 0 {
79+
return 0, fmt.Errorf("invalid annotation %s: %s is not a valid value: %w", fleetv1beta1.CRPGenerationAnnotation, placementGenerationStr, err)
7080
}
7181

72-
return true, subindex, nil
82+
return int64(observedplacementGeneration), nil
7383
}
7484

7585
// ExtractNumberOfResourceSnapshotsFromResourceSnapshot extracts the number of clusterResourceSnapshots in a group from the master clusterResourceSnapshot.
@@ -125,27 +135,3 @@ func ParseResourceGroupHashFromAnnotation(resourceSnapshot fleetv1beta1.Resource
125135
}
126136
return v, nil
127137
}
128-
129-
// ExtractSubindexFromResourceSnapshot is a helper function to extract subindex from ResourceSnapshot objects.
130-
func ExtractSubindexFromResourceSnapshot(snapshot fleetv1beta1.ResourceSnapshotObj) (doesExist bool, subindex int, err error) {
131-
annotations := snapshot.GetAnnotations()
132-
if annotations == nil {
133-
return false, 0, nil
134-
}
135-
136-
subindexStr, exists := annotations[fleetv1beta1.SubindexOfResourceSnapshotAnnotation]
137-
if !exists || subindexStr == "" {
138-
return false, 0, nil
139-
}
140-
141-
subindex, err = strconv.Atoi(subindexStr)
142-
if err != nil {
143-
return false, 0, fmt.Errorf("invalid subindex annotation value %q: %w", subindexStr, err)
144-
}
145-
146-
if subindex < 0 {
147-
return false, 0, fmt.Errorf("subindex cannot be negative: %d", subindex)
148-
}
149-
150-
return true, subindex, nil
151-
}

0 commit comments

Comments
 (0)