Skip to content

Commit

Permalink
fix cluster labels update conflictions.
Browse files Browse the repository at this point in the history 
Signed-off-by: morvencao <[email protected]>
  • Loading branch information
@morvencao
morvencao committed Nov 23, 2022
1 parent 18e7982 commit 1ed4b3c
Show file tree
Hide file tree
Showing 2 changed files with 155 additions and 56 deletions.
124 changes: 86 additions & 38 deletions pkg/hub/addon/discovery_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,20 @@ package addon

import (
"context"
"encoding/json"
"fmt"
"reflect"
"strings"
"time"

"github.com/openshift/library-go/pkg/controller/factory"
"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/resource/resourcemerge"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/client-go/tools/cache"
"k8s.io/klog/v2"
Expand Down Expand Up @@ -110,48 +112,65 @@ func (c *addOnFeatureDiscoveryController) sync(ctx context.Context, syncCtx fact
func (c *addOnFeatureDiscoveryController) syncAddOn(ctx context.Context, clusterName, addOnName string) error {
klog.V(4).Infof("Reconciling addOn %q", addOnName)

labels := map[string]string{}
cluster, err := c.clusterLister.Get(clusterName)
if errors.IsNotFound(err) {
// no cluster, it could be deleted
return nil
}
if err != nil {
return fmt.Errorf("unable to find cluster with name %q: %w", clusterName, err)
}
// no work if cluster is deleting
if !cluster.DeletionTimestamp.IsZero() {
return nil
}

labels := cluster.Labels
if len(labels) == 0 {
labels = map[string]string{}
}

// make a copy to check if the cluster labels are changed
labelsCopy := map[string]string{}
for key, value := range labels {
labelsCopy[key] = value
}

addOn, err := c.addOnLister.ManagedClusterAddOns(clusterName).Get(addOnName)
switch {
case errors.IsNotFound(err):
// addon is deleted
key := fmt.Sprintf("%s%s-", addOnFeaturePrefix, addOnName)
labels[key] = ""
key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName)
delete(labels, key)
labels[fmt.Sprintf("%s-", key)] = ""
case err != nil:
return err
case !addOn.DeletionTimestamp.IsZero():
key := fmt.Sprintf("%s%s-", addOnFeaturePrefix, addOnName)
labels[key] = ""
key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName)
delete(labels, key)
labels[fmt.Sprintf("%s-", key)] = ""
default:
key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOn.Name)
labels[key] = getAddOnLabelValue(addOn)
}

cluster, err := c.clusterLister.Get(clusterName)
if errors.IsNotFound(err) {
// no cluster, it could be deleted
// no work if the labels are not changed
if reflect.DeepEqual(labelsCopy, labels) {
return nil
}

patchBytes, err := json.Marshal(map[string]interface{}{
"metadata": map[string]interface{}{
"labels": labels,
},
})
if err != nil {
return fmt.Errorf("unable to find cluster with name %q: %w", clusterName, err)
return fmt.Errorf("failed to create patch for cluster %s: %w", cluster.Name, err)
}
// no work if cluster is deleting
if !cluster.DeletionTimestamp.IsZero() {
return nil
}

// merge labels
modified := false
cluster = cluster.DeepCopy()
resourcemerge.MergeMap(&modified, &cluster.Labels, labels)

// no work if the cluster labels have no change
if !modified {
return nil
}
// patch the cluster labels
_, err = c.clusterClient.ClusterV1().ManagedClusters().Patch(ctx, cluster.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{})

// otherwise, update cluster
_, err = c.clusterClient.ClusterV1().ManagedClusters().Update(ctx, cluster, metav1.UpdateOptions{})
return err
}

Expand All @@ -172,43 +191,72 @@ func (c *addOnFeatureDiscoveryController) syncCluster(ctx context.Context, clust
}

// build labels for existing addons
addOnLabels := map[string]string{}
addOnLabels := cluster.Labels
if len(addOnLabels) == 0 {
addOnLabels = map[string]string{}
}

// make a copy to check if the cluster labels are changed
addOnLabelsCopy := map[string]string{}
for key, value := range addOnLabels {
addOnLabelsCopy[key] = value
}

newAddonLabels := map[string]string{}
addOns, err := c.addOnLister.ManagedClusterAddOns(clusterName).List(labels.Everything())
if err != nil {
return fmt.Errorf("unable to list addOns of cluster %q: %w", clusterName, err)
}

// no work if no addons update
if len(addOnLabels) == 0 && len(addOns) == 0 {
return nil
}

for _, addOn := range addOns {
key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOn.Name)

// addon is deleting
if !addOn.DeletionTimestamp.IsZero() {
delete(addOnLabels, key)
addOnLabels[fmt.Sprintf("%s-", key)] = ""
continue
}
key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOn.Name)

addOnLabels[key] = getAddOnLabelValue(addOn)
newAddonLabels[key] = getAddOnLabelValue(addOn)
}

// remove addon lable if its corresponding addon no longer exists
for key := range cluster.Labels {
for key := range addOnLabels {
if !strings.HasPrefix(key, addOnFeaturePrefix) {
continue
}

if _, ok := addOnLabels[key]; !ok {
if _, ok := newAddonLabels[key]; !ok {
delete(addOnLabels, key)
addOnLabels[fmt.Sprintf("%s-", key)] = ""
}
}

// merge labels
modified := false
cluster = cluster.DeepCopy()
resourcemerge.MergeMap(&modified, &cluster.Labels, addOnLabels)

// no work if the cluster labels have no change
if !modified {
// no work if the labels are not changed
if reflect.DeepEqual(addOnLabelsCopy, addOnLabels) {
return nil
}

// otherwise, update cluster
_, err = c.clusterClient.ClusterV1().ManagedClusters().Update(ctx, cluster, metav1.UpdateOptions{})
// build cluster labels patch
patchBytes, err := json.Marshal(map[string]interface{}{
"metadata": map[string]interface{}{
"labels": addOnLabels,
},
})
if err != nil {
return fmt.Errorf("failed to create patch for cluster %s: %w", cluster.Name, err)
}

// patch the cluster labels
_, err = c.clusterClient.ClusterV1().ManagedClusters().Patch(ctx, cluster.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{})

return err
}

Expand Down
87 changes: 69 additions & 18 deletions pkg/hub/addon/discovery_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package addon

import (
"context"
"encoding/json"
"fmt"
"testing"
"time"
Expand Down Expand Up @@ -100,9 +101,9 @@ func TestDiscoveryController_SyncAddOn(t *testing.T) {
},
},
validateActions: func(t *testing.T, actions []clienttesting.Action) {
testinghelpers.AssertActions(t, actions, "update")
actual := actions[0].(clienttesting.UpdateActionImpl).Object
assertNoAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon1")
testinghelpers.AssertActions(t, actions, "patch")
actualPatchAction := actions[0].(clienttesting.PatchActionImpl)
assertPatchNoAddonLabel(t, actualPatchAction, "addon1")
},
},
{
Expand All @@ -116,10 +117,15 @@ func TestDiscoveryController_SyncAddOn(t *testing.T) {
addOn: &addonv1alpha1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{
Name: "addon1",
Namespace: clusterName,
DeletionTimestamp: &deleteTime,
},
},
validateActions: testinghelpers.AssertNoActions,
validateActions: func(t *testing.T, actions []clienttesting.Action) {
testinghelpers.AssertActions(t, actions, "patch")
actualPatchAction := actions[0].(clienttesting.PatchActionImpl)
assertPatchNoAddonLabel(t, actualPatchAction, "addon1")
},
},
{
name: "new addon is added",
Expand All @@ -136,9 +142,9 @@ func TestDiscoveryController_SyncAddOn(t *testing.T) {
},
},
validateActions: func(t *testing.T, actions []clienttesting.Action) {
testinghelpers.AssertActions(t, actions, "update")
actual := actions[0].(clienttesting.UpdateActionImpl).Object
assertAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon1", addOnStatusUnreachable)
testinghelpers.AssertActions(t, actions, "patch")
actualPatchAction := actions[0].(clienttesting.PatchActionImpl)
assertPatchAddonLabel(t, actualPatchAction, "addon1", addOnStatusUnreachable)
},
},
{
Expand All @@ -159,9 +165,9 @@ func TestDiscoveryController_SyncAddOn(t *testing.T) {
},
},
validateActions: func(t *testing.T, actions []clienttesting.Action) {
testinghelpers.AssertActions(t, actions, "update")
actual := actions[0].(clienttesting.UpdateActionImpl).Object
assertAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon1", addOnStatusUnreachable)
testinghelpers.AssertActions(t, actions, "patch")
actualPatchAction := actions[0].(clienttesting.PatchActionImpl)
assertPatchAddonLabel(t, actualPatchAction, "addon1", addOnStatusUnreachable)
},
},
{
Expand Down Expand Up @@ -257,9 +263,9 @@ func TestDiscoveryController_Sync(t *testing.T) {
},
},
validateActions: func(t *testing.T, actions []clienttesting.Action) {
testinghelpers.AssertActions(t, actions, "update")
actual := actions[0].(clienttesting.UpdateActionImpl).Object
assertAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon1", addOnStatusUnreachable)
testinghelpers.AssertActions(t, actions, "patch")
actualPatchAction := actions[0].(clienttesting.PatchActionImpl)
assertPatchAddonLabel(t, actualPatchAction, "addon1", addOnStatusUnreachable)
},
},
{
Expand Down Expand Up @@ -329,11 +335,11 @@ func TestDiscoveryController_Sync(t *testing.T) {
},
},
validateActions: func(t *testing.T, actions []clienttesting.Action) {
testinghelpers.AssertActions(t, actions, "update")
actual := actions[0].(clienttesting.UpdateActionImpl).Object
assertAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon1", addOnStatusUnreachable)
assertAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon3", addOnStatusAvailable)
assertNoAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon4")
testinghelpers.AssertActions(t, actions, "patch")
actualPatchAction := actions[0].(clienttesting.PatchActionImpl)
assertPatchAddonLabel(t, actualPatchAction, "addon1", addOnStatusUnreachable)
assertPatchAddonLabel(t, actualPatchAction, "addon3", addOnStatusAvailable)
assertPatchNoAddonLabel(t, actualPatchAction, "addon4")
},
},
}
Expand Down Expand Up @@ -402,3 +408,48 @@ func assertNoAddonLabel(t *testing.T, cluster *clusterv1.ManagedCluster, addOnNa
t.Errorf("label %q found", key)
}
}

func assertPatchAddonLabel(t *testing.T, actionPatch clienttesting.PatchActionImpl, addOnName, addOnStatus string) {
var patchObj map[string]map[string]map[string]string
if err := json.Unmarshal(actionPatch.Patch, &patchObj); err != nil {
t.Errorf("failed to unmarshal patch %s: %v", patchObj, err)
}
metadata, ok := patchObj["metadata"]
if !ok {
t.Errorf("patch %s doesn't contain metadata field", patchObj)
}
labels, ok := metadata["labels"]
if !ok {
t.Errorf("patch %s doesn't contain metadata.labels field", patchObj)
}

key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName)
value, ok := labels[key]
if !ok {
t.Errorf("label %q not found", key)
}

if value != addOnStatus {
t.Errorf("expect label value %q but found %q", addOnStatus, value)
}
}

func assertPatchNoAddonLabel(t *testing.T, actionPatch clienttesting.PatchActionImpl, addOnName string) {
var patchObj map[string]map[string]map[string]string
if err := json.Unmarshal(actionPatch.Patch, &patchObj); err != nil {
t.Errorf("failed to unmarshal patch %s: %v", patchObj, err)
}
metadata, ok := patchObj["metadata"]
if !ok {
t.Errorf("patch %s doesn't contain metadata field", patchObj)
}
labels, ok := metadata["labels"]
if !ok {
t.Errorf("patch %s doesn't contain metadata.labels field", patchObj)
}

key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName)
if _, ok := labels[key]; ok {
t.Errorf("label %q found", key)
}
}

0 comments on commit 1ed4b3c

Please sign in to comment.