Skip to content

Commit ad9bda9

Browse files
jwttyjwtty
committed
fix comments
1 parent 4624825 commit ad9bda9

File tree

3 files changed

+66
-78
lines changed

3 files changed

+66
-78
lines changed

‎pkg/controllers/updaterun/controller_integration_test.go‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,6 @@ const (
3535
timeout = time.Second * 10
3636
// interval is the time to wait between retries for Eventually and Consistently
3737
interval = time.Millisecond * 250
38-
// duration is the time to check for Consistently
39-
duration = time.Second * 20
4038

4139
// numTargetClusters is the number of scheduled clusters
4240
numTargetClusters = 10
@@ -70,7 +68,7 @@ var _ = Describe("Test the clusterStagedUpdateRun controller", func() {
7068
Context("Test reconciling a clusterStagedUpdateRun", func() {
7169
It("Should add the finalizer to the clusterStagedUpdateRun", func() {
7270
By("Creating a new clusterStagedUpdateRun")
73-
updateRun := getTestClusterStagedUpdateRun()
71+
updateRun := generateTestClusterStagedUpdateRun()
7472
Expect(k8sClient.Create(ctx, updateRun)).Should(Succeed())
7573

7674
By("Checking the finalizer is added")
@@ -87,7 +85,7 @@ var _ = Describe("Test the clusterStagedUpdateRun controller", func() {
8785
Context("Test deleting a clusterStagedUpdateRun", func() {
8886
It("Should delete the clusterStagedUpdateRun without any clusterApprovalRequests", func() {
8987
By("Creating a new clusterStagedUpdateRun")
90-
updateRun := getTestClusterStagedUpdateRun()
88+
updateRun := generateTestClusterStagedUpdateRun()
9189
Expect(k8sClient.Create(ctx, updateRun)).Should(Succeed())
9290

9391
By("Checking the finalizer is added")
@@ -102,7 +100,7 @@ var _ = Describe("Test the clusterStagedUpdateRun controller", func() {
102100

103101
It("Should delete the clusterStagedUpdateRun if it failed", func() {
104102
By("Creating a new clusterStagedUpdateRun")
105-
updateRun := getTestClusterStagedUpdateRun()
103+
updateRun := generateTestClusterStagedUpdateRun()
106104
Expect(k8sClient.Create(ctx, updateRun)).Should(Succeed())
107105

108106
By("Checking the finalizer is added")
@@ -116,7 +114,7 @@ var _ = Describe("Test the clusterStagedUpdateRun controller", func() {
116114
Expect(k8sClient.Status().Update(ctx, updateRun)).Should(Succeed(), "failed to update the clusterStagedUpdateRun")
117115

118116
By("Creating a clusterApprovalRequest")
119-
approvalRequest := getTestApprovalRequest("req1")
117+
approvalRequest := generateTestApprovalRequest("req1")
120118
Expect(k8sClient.Create(ctx, approvalRequest)).Should(Succeed())
121119

122120
By("Deleting the clusterStagedUpdateRun")
@@ -131,7 +129,7 @@ var _ = Describe("Test the clusterStagedUpdateRun controller", func() {
131129

132130
It("Should not block deletion though the clusterStagedUpdateRun is still processing", func() {
133131
By("Creating a new clusterStagedUpdateRun")
134-
updateRun := getTestClusterStagedUpdateRun()
132+
updateRun := generateTestClusterStagedUpdateRun()
135133
Expect(k8sClient.Create(ctx, updateRun)).Should(Succeed())
136134

137135
By("Checking the finalizer is added")
@@ -143,7 +141,7 @@ var _ = Describe("Test the clusterStagedUpdateRun controller", func() {
143141
Expect(k8sClient.Status().Update(ctx, updateRun)).Should(Succeed(), "failed to add condition to the clusterStagedUpdateRun")
144142

145143
By("Creating a clusterApprovalRequest")
146-
approvalRequest := getTestApprovalRequest("req1")
144+
approvalRequest := generateTestApprovalRequest("req1")
147145
Expect(k8sClient.Create(ctx, approvalRequest)).Should(Succeed())
148146

149147
By("Deleting the clusterStagedUpdateRun")
@@ -158,7 +156,7 @@ var _ = Describe("Test the clusterStagedUpdateRun controller", func() {
158156

159157
It("Should delete all ClusterApprovalRequest objects associated with the clusterStagedUpdateRun", func() {
160158
By("Creating a new clusterStagedUpdateRun")
161-
updateRun := getTestClusterStagedUpdateRun()
159+
updateRun := generateTestClusterStagedUpdateRun()
162160
Expect(k8sClient.Create(ctx, updateRun)).Should(Succeed())
163161

164162
By("Creating ClusterApprovalRequests")
@@ -208,7 +206,7 @@ var _ = Describe("Test the clusterStagedUpdateRun controller", func() {
208206
})
209207
})
210208

211-
func getTestClusterStagedUpdateRun() *placementv1alpha1.ClusterStagedUpdateRun {
209+
func generateTestClusterStagedUpdateRun() *placementv1alpha1.ClusterStagedUpdateRun {
212210
return &placementv1alpha1.ClusterStagedUpdateRun{
213211
ObjectMeta: metav1.ObjectMeta{
214212
Name: testUpdateRunName,
@@ -221,7 +219,7 @@ func getTestClusterStagedUpdateRun() *placementv1alpha1.ClusterStagedUpdateRun {
221219
}
222220
}
223221

224-
func getTestClusterResourcePlacement() *placementv1beta1.ClusterResourcePlacement {
222+
func generateTestClusterResourcePlacement() *placementv1beta1.ClusterResourcePlacement {
225223
return &placementv1beta1.ClusterResourcePlacement{
226224
ObjectMeta: metav1.ObjectMeta{
227225
Name: testCRPName,
@@ -246,7 +244,7 @@ func getTestClusterResourcePlacement() *placementv1beta1.ClusterResourcePlacemen
246244
}
247245
}
248246

249-
func getTestClusterSchedulingPolicySnapshot(idx int) *placementv1beta1.ClusterSchedulingPolicySnapshot {
247+
func generateTestClusterSchedulingPolicySnapshot(idx int) *placementv1beta1.ClusterSchedulingPolicySnapshot {
250248
return &placementv1beta1.ClusterSchedulingPolicySnapshot{
251249
ObjectMeta: metav1.ObjectMeta{
252250
Name: fmt.Sprintf(placementv1beta1.PolicySnapshotNameFmt, testCRPName, idx),
@@ -267,7 +265,7 @@ func getTestClusterSchedulingPolicySnapshot(idx int) *placementv1beta1.ClusterSc
267265
}
268266
}
269267

270-
func getTestClusterResourceBinding(policySnapshotName, targetCluster string) *placementv1beta1.ClusterResourceBinding {
268+
func generateTestClusterResourceBinding(policySnapshotName, targetCluster string) *placementv1beta1.ClusterResourceBinding {
271269
binding := &placementv1beta1.ClusterResourceBinding{
272270
ObjectMeta: metav1.ObjectMeta{
273271
Name: "binding-" + testResourceSnapshotName + "-" + targetCluster,
@@ -284,7 +282,7 @@ func getTestClusterResourceBinding(policySnapshotName, targetCluster string) *pl
284282
return binding
285283
}
286284

287-
func getTestMemberCluster(idx int, clusterName string, labels map[string]string) *clusterv1beta1.MemberCluster {
285+
func generateTestMemberCluster(idx int, clusterName string, labels map[string]string) *clusterv1beta1.MemberCluster {
288286
clusterLabels := make(map[string]string)
289287
for k, v := range labels {
290288
clusterLabels[k] = v
@@ -306,7 +304,7 @@ func getTestMemberCluster(idx int, clusterName string, labels map[string]string)
306304
}
307305
}
308306

309-
func getTestClusterStagedUpdateStrategy() *placementv1alpha1.ClusterStagedUpdateStrategy {
307+
func generateTestClusterStagedUpdateStrategy() *placementv1alpha1.ClusterStagedUpdateStrategy {
310308
sortingKey := "index"
311309
return &placementv1alpha1.ClusterStagedUpdateStrategy{
312310
ObjectMeta: metav1.ObjectMeta{
@@ -352,7 +350,7 @@ func getTestClusterStagedUpdateStrategy() *placementv1alpha1.ClusterStagedUpdate
352350
}
353351
}
354352

355-
func getTestClusterResourceSnapshot() *placementv1beta1.ClusterResourceSnapshot {
353+
func generateTestClusterResourceSnapshot() *placementv1beta1.ClusterResourceSnapshot {
356354
clusterResourceSnapshot := &placementv1beta1.ClusterResourceSnapshot{
357355
ObjectMeta: metav1.ObjectMeta{
358356
Name: testResourceSnapshotName,
@@ -377,7 +375,7 @@ func getTestClusterResourceSnapshot() *placementv1beta1.ClusterResourceSnapshot
377375
return clusterResourceSnapshot
378376
}
379377

380-
func getTestClusterResourceOverride() *placementv1alpha1.ClusterResourceOverrideSnapshot {
378+
func generateTestClusterResourceOverride() *placementv1alpha1.ClusterResourceOverrideSnapshot {
381379
return &placementv1alpha1.ClusterResourceOverrideSnapshot{
382380
ObjectMeta: metav1.ObjectMeta{
383381
Name: testCROName,
@@ -425,7 +423,7 @@ func getTestClusterResourceOverride() *placementv1alpha1.ClusterResourceOverride
425423
}
426424
}
427425

428-
func getTestApprovalRequest(name string) *placementv1alpha1.ClusterApprovalRequest {
426+
func generateTestApprovalRequest(name string) *placementv1alpha1.ClusterApprovalRequest {
429427
return &placementv1alpha1.ClusterApprovalRequest{
430428
ObjectMeta: metav1.ObjectMeta{
431429
Name: name,

‎pkg/controllers/updaterun/initialization.go‎

Lines changed: 28 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -69,14 +69,16 @@ func (r *Reconciler) validateCRP(ctx context.Context, updateRun *placementv1alph
6969
klog.ErrorS(err, "Failed to get ClusterResourcePlacement", "clusterResourcePlacement", placementName, "clusterStagedUpdateRun", updateRunRef)
7070
if apierrors.IsNotFound(err) {
7171
// we won't continue or retry the initialization if the ClusterResourcePlacement is not found.
72-
return "", fmt.Errorf("%w: %s", errInitializedFailed, "parent clusterResourcePlacement not found")
72+
crpNotFoundErr := controller.NewUserError(fmt.Errorf("parent clusterResourcePlacement not found"))
73+
return "", fmt.Errorf("%w: %s", errInitializedFailed, crpNotFoundErr.Error())
7374
}
74-
return "", err
75+
return "", controller.NewAPIServerError(true, err)
7576
}
7677
// Check if the ClusterResourcePlacement has an external rollout strategy.
7778
if crp.Spec.Strategy.Type != placementv1beta1.ExternalRolloutStrategyType {
7879
klog.V(2).InfoS("The clusterResourcePlacement does not have an external rollout strategy", "clusterResourcePlacement", placementName, "clusterStagedUpdateRun", updateRunRef)
79-
return "", fmt.Errorf("%w: %s", errInitializedFailed, "parent clusterResourcePlacement does not have an external rollout strategy, current strategy: "+string(crp.Spec.Strategy.Type))
80+
wrongRolloutTypeErr := controller.NewUserError(fmt.Errorf("parent clusterResourcePlacement does not have an external rollout strategy, current strategy: " + string(crp.Spec.Strategy.Type)))
81+
return "", fmt.Errorf("%w: %s", errInitializedFailed, wrongRolloutTypeErr.Error())
8082
}
8183
updateRun.Status.ApplyStrategy = crp.Spec.Strategy.ApplyStrategy
8284
return crp.Name, nil
@@ -99,7 +101,7 @@ func (r *Reconciler) determinePolicySnapshot(
99101
if err := r.Client.List(ctx, &policySnapshotList, latestPolicyMatcher); err != nil {
100102
klog.ErrorS(err, "Failed to list the latest policy snapshots", "clusterResourcePlacement", placementName, "clusterStagedUpdateRun", updateRunRef)
101103
// err can be retried.
102-
return nil, -1, err
104+
return nil, -1, controller.NewAPIServerError(true, err)
103105
}
104106
if len(policySnapshotList.Items) != 1 {
105107
if len(policySnapshotList.Items) > 1 {
@@ -167,13 +169,16 @@ func (r *Reconciler) collectScheduledClusters(
167169
if err := r.Client.List(ctx, &bindingList, resourceBindingMatcher); err != nil {
168170
klog.ErrorS(err, "Failed to list clusterResourceBindings", "clusterResourcePlacement", placementName, "latestPolicySnapshot", latestPolicySnapshot.Name, "clusterStagedUpdateRun", updateRunRef)
169171
// list err can be retried.
170-
return nil, nil, err
172+
return nil, nil, controller.NewAPIServerError(true, err)
171173
}
172174
var toBeDeletedBindings, selectedBindings []*placementv1beta1.ClusterResourceBinding
173175
for i, binding := range bindingList.Items {
174176
if binding.Spec.SchedulingPolicySnapshotName == latestPolicySnapshot.Name {
175177
if binding.Spec.State != placementv1beta1.BindingStateScheduled && binding.Spec.State != placementv1beta1.BindingStateBound {
176-
return nil, nil, controller.NewUnexpectedBehaviorError(fmt.Errorf("binding `%s`'s state %s is not scheduled or bound", binding.Name, binding.Spec.State))
178+
stateErr := fmt.Errorf("binding `%s`'s state %s is not scheduled or bound", binding.Name, binding.Spec.State)
179+
klog.ErrorS(stateErr, "Failed to collect clusterResourceBindings", "clusterResourcePlacement", placementName, "latestPolicySnapshot", latestPolicySnapshot.Name, "clusterStagedUpdateRun", updateRunRef)
180+
// no more retries here.
181+
return nil, nil, fmt.Errorf("%w: %s", errInitializedFailed, stateErr.Error())
177182
}
178183
klog.V(2).InfoS("Found a scheduled binding", "binding", binding.Name, "clusterResourcePlacement", placementName, "latestPolicySnapshot", latestPolicySnapshot.Name, "clusterStagedUpdateRun", updateRunRef)
179184
selectedBindings = append(selectedBindings, &bindingList.Items[i])
@@ -206,10 +211,11 @@ func (r *Reconciler) generateStagesByStrategy(
206211
klog.ErrorS(err, "Failed to get StagedUpdateStrategy", "stagedUpdateStrategy", updateRun.Spec.StagedUpdateStrategyName, "clusterStagedUpdateRun", updateRunRef)
207212
if apierrors.IsNotFound(err) {
208213
// we won't continue or retry the initialization if the StagedUpdateStrategy is not found.
209-
return fmt.Errorf("%w: %s", errInitializedFailed, "referenced clusterStagedUpdateStrategy not found: "+updateRun.Spec.StagedUpdateStrategyName)
214+
strategyNotFoundErr := controller.NewUserError(fmt.Errorf("referenced clusterStagedUpdateStrategy not found: " + updateRun.Spec.StagedUpdateStrategyName))
215+
return fmt.Errorf("%w: %s", errInitializedFailed, strategyNotFoundErr.Error())
210216
}
211217
// other err can be retried.
212-
return err
218+
return controller.NewAPIServerError(true, err)
213219
}
214220
// This won't change even if the stagedUpdateStrategy changes or is deleted after the updateRun is initialized.
215221
updateRun.Status.StagedUpdateStrategySnapshot = &updateStrategy.Spec
@@ -255,7 +261,8 @@ func (r *Reconciler) computeRunStageStatus(
255261
if err := validateAfterStageTask(stage.AfterStageTasks); err != nil {
256262
klog.ErrorS(err, "Failed to validate the after stage tasks", "clusterStagedUpdateStrategy", updateStrategyName, "stage name", stage.Name, "clusterStagedUpdateRun", updateRunRef)
257263
// no more retries here.
258-
return fmt.Errorf("%w: the after stage tasks are invalid, clusterStagedUpdateStrategy: %s, stage: %s, err: %s", errInitializedFailed, updateStrategyName, stage.Name, err.Error())
264+
invalidAfterStageErr := controller.NewUserError(fmt.Errorf("the after stage tasks are invalid, clusterStagedUpdateStrategy: %s, stage: %s, err: %s", updateStrategyName, stage.Name, err.Error()))
265+
return fmt.Errorf("%w: %s", errInitializedFailed, invalidAfterStageErr.Error())
259266
}
260267

261268
curStageUpdatingStatus := placementv1alpha1.StageUpdatingStatus{StageName: stage.Name}
@@ -264,30 +271,31 @@ func (r *Reconciler) computeRunStageStatus(
264271
if err != nil {
265272
klog.ErrorS(err, "Failed to convert label selector", "clusterStagedUpdateStrategy", updateStrategyName, "stage name", stage.Name, "labelSelector", stage.LabelSelector, "clusterStagedUpdateRun", updateRunRef)
266273
// no more retries here.
267-
return fmt.Errorf("%w: the stage label selector is invalid, clusterStagedUpdateStrategy: %s, stage: %s, err: %s", errInitializedFailed, updateStrategyName, stage.Name, err.Error())
274+
invalidLabelErr := controller.NewUserError(fmt.Errorf("the stage label selector is invalid, clusterStagedUpdateStrategy: %s, stage: %s, err: %s", updateStrategyName, stage.Name, err.Error()))
275+
return controller.NewUserError(fmt.Errorf("%w: %s", errInitializedFailed, invalidLabelErr.Error()))
268276
}
269277
// List all the clusters that match the label selector.
270278
var clusterList clusterv1beta1.MemberClusterList
271279
if err := r.Client.List(ctx, &clusterList, &client.ListOptions{LabelSelector: labelSelector}); err != nil {
272280
klog.ErrorS(err, "Failed to list clusters for the stage", "clusterStagedUpdateStrategy", updateStrategyName, "stage name", stage.Name, "labelSelector", stage.LabelSelector, "clusterStagedUpdateRun", updateRunRef)
273281
// list err can be retried.
274-
return err
282+
return controller.NewAPIServerError(true, err)
275283
}
276284

277285
// Intersect the selected clusters with the clusters in the stage.
278286
for _, cluster := range clusterList.Items {
279287
if _, ok := allSelectedClusters[cluster.Name]; ok {
280288
if _, ok := allPlacedClusters[cluster.Name]; ok {
281289
// a cluster can only appear in one stage.
282-
dupErr := fmt.Errorf("cluster `%s` appears in more than one stages", cluster.Name)
290+
dupErr := controller.NewUserError(fmt.Errorf("cluster `%s` appears in more than one stages", cluster.Name))
283291
klog.ErrorS(dupErr, "Failed to compute the stage", "clusterStagedUpdateStrategy", updateStrategyName, "stage name", stage.Name, "clusterStagedUpdateRun", updateRunRef)
284292
// no more retries here.
285293
return fmt.Errorf("%w: %s", errInitializedFailed, dupErr.Error())
286294
}
287295
if stage.SortingLabelKey != nil {
288296
// interpret the label values as integers.
289297
if _, err := strconv.Atoi(cluster.Labels[*stage.SortingLabelKey]); err != nil {
290-
keyErr := fmt.Errorf("the sorting label `%s:%s` on cluster `%s` is not valid: %s", *stage.SortingLabelKey, cluster.Labels[*stage.SortingLabelKey], cluster.Name, err.Error())
298+
keyErr := controller.NewUserError(fmt.Errorf("the sorting label `%s:%s` on cluster `%s` is not valid: %s", *stage.SortingLabelKey, cluster.Labels[*stage.SortingLabelKey], cluster.Name, err.Error()))
291299
klog.ErrorS(keyErr, "Failed to sort clusters in the stage", "clusterStagedUpdateStrategy", updateStrategyName, "stage name", stage.Name, "clusterStagedUpdateRun", updateRunRef)
292300
// no more retries here.
293301
return fmt.Errorf("%w: %s", errInitializedFailed, keyErr.Error())
@@ -337,7 +345,7 @@ func (r *Reconciler) computeRunStageStatus(
337345

338346
// Check if the clusters are all placed.
339347
if len(allPlacedClusters) != len(allSelectedClusters) {
340-
missingErr := fmt.Errorf("some clusters are not placed in any stage")
348+
missingErr := controller.NewUserError(fmt.Errorf("some clusters are not placed in any stage"))
341349
for cluster := range allSelectedClusters {
342350
if _, ok := allPlacedClusters[cluster]; !ok {
343351
klog.ErrorS(missingErr, "Cluster is missing in any stage", "cluster", cluster, "clusterStagedUpdateStrategy", updateStrategyName, "clusterStagedUpdateRun", updateRunRef)
@@ -372,23 +380,23 @@ func (r *Reconciler) recordOverrideSnapshots(ctx context.Context, placementName
372380
if err := r.Client.Get(ctx, client.ObjectKey{Name: updateRun.Spec.ResourceSnapshotIndex}, &masterResourceSnapshot); err != nil {
373381
klog.ErrorS(err, "Failed to get the master resource snapshot", "resourceSnapshot", updateRun.Spec.ResourceSnapshotIndex, "clusterStagedUpdateRun", updateRunRef)
374382
if apierrors.IsNotFound(err) {
375-
snapshotNotFoundErr := fmt.Errorf("resource snapshot `%s` not found", updateRun.Spec.ResourceSnapshotIndex)
383+
snapshotNotFoundErr := controller.NewUserError(fmt.Errorf("resource snapshot `%s` not found", updateRun.Spec.ResourceSnapshotIndex))
376384
// we won't continue or retry the initialization if the resource snapshot is not found.
377385
return fmt.Errorf("%w: %s", errInitializedFailed, snapshotNotFoundErr.Error())
378386
}
379387
// err can be retried.
380-
return err
388+
return controller.NewAPIServerError(true, err)
381389
}
382390

383391
if parentCRP, ok := masterResourceSnapshot.Labels[placementv1beta1.CRPTrackingLabel]; !ok || parentCRP != placementName {
384-
wrongCRPErr := fmt.Errorf("resource snapshot `%s` is not associated with expected clusterResourcePlacement `%s`, got: `%s`", updateRun.Spec.ResourceSnapshotIndex, placementName, parentCRP)
392+
wrongCRPErr := controller.NewUserError(fmt.Errorf("resource snapshot `%s` is not associated with expected clusterResourcePlacement `%s`, got: `%s`", updateRun.Spec.ResourceSnapshotIndex, placementName, parentCRP))
385393
klog.ErrorS(wrongCRPErr, "Failed to get the master resource snapshot", "resourceSnapshot", updateRun.Spec.ResourceSnapshotIndex, "clusterStagedUpdateRun", updateRunRef)
386394
// no more retries here.
387395
return fmt.Errorf("%w: %s", errInitializedFailed, wrongCRPErr.Error())
388396
}
389397

390398
if hash, ok := masterResourceSnapshot.Annotations[placementv1beta1.ResourceGroupHashAnnotation]; !ok || len(hash) == 0 {
391-
nomasterErr := fmt.Errorf("resource snapshot `%s` is not a master snapshot", updateRun.Spec.ResourceSnapshotIndex)
399+
nomasterErr := controller.NewUserError(fmt.Errorf("resource snapshot `%s` is not a master snapshot", updateRun.Spec.ResourceSnapshotIndex))
392400
klog.ErrorS(nomasterErr, "Failed to get the master resource snapshot", "resourceSnapshot", updateRun.Spec.ResourceSnapshotIndex, "clusterStagedUpdateRun", updateRunRef)
393401
// no more retries here.
394402
return fmt.Errorf("%w: %s", errInitializedFailed, nomasterErr.Error())
@@ -429,7 +437,7 @@ func (r *Reconciler) recordInitializationSucceeded(ctx context.Context, updateRu
429437
if updateErr := r.Client.Status().Update(ctx, updateRun); updateErr != nil {
430438
klog.ErrorS(updateErr, "Failed to update the ClusterStagedUpdateRun status as initialized", "clusterStagedUpdateRun", klog.KObj(updateRun))
431439
// updateErr can be retried.
432-
return updateErr
440+
return controller.NewAPIServerError(false, updateErr)
433441
}
434442
return nil
435443
}
@@ -446,7 +454,7 @@ func (r *Reconciler) recordInitializationFailed(ctx context.Context, updateRun *
446454
if updateErr := r.Client.Status().Update(ctx, updateRun); updateErr != nil {
447455
klog.ErrorS(updateErr, "Failed to update the ClusterStagedUpdateRun status as failed to initialize", "clusterStagedUpdateRun", klog.KObj(updateRun))
448456
// updateErr can be retried.
449-
return updateErr
457+
return controller.NewAPIServerError(false, updateErr)
450458
}
451459
return nil
452460
}

0 commit comments

Comments
 (0)