Skip to content

Enable deep JSON merging for Kubernetes resources when updating them #1019

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

Merged
ibm-ci-bot merged 7 commits into from
Mar 27, 2024
Merged

Enable deep JSON merging for Kubernetes resources when updating them #1019

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f23382c
enable deep json merge for k8s resources
Feb 27, 2024
73450a4
fix deep merge for resource spec
Mar 11, 2024
65e5ee8
merge for all resrources type
Mar 12, 2024
3a4a1a1
each changed object updated once
Mar 14, 2024
9705c95
compare whole resrouces file instead of spec section
Mar 25, 2024
b59fd58
set ownerreferences
Mar 25, 2024
2e86588
ensure ann, label and set owner on updated resrouce
Mar 27, 2024
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
79 changes: 42 additions & 37 deletions controllers/operandrequest/reconcile_operand.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,12 +771,12 @@ func (r *Reconciler) updateCustomResource(ctx context.Context, existingCR unstru
// Merge spec from update existing CR and OperandConfig spec
updatedCRSpec := util.MergeCR(updatedExistingCRRaw, crConfig)

CRgeneration := existingCR.GetGeneration()

if reflect.DeepEqual(existingCR.Object["spec"], updatedCRSpec) && !forceUpdate {
return true, nil
}

CRgeneration := existingCR.GetGeneration()

klog.V(2).Infof("updating custom resource with apiversion: %s, kind: %s, %s/%s", apiversion, kind, namespace, name)

existingCR.Object["spec"] = updatedCRSpec
Expand Down Expand Up @@ -1057,61 +1057,66 @@ func (r *Reconciler) updateK8sResource(ctx context.Context, existingK8sRes unstr
return true, nil
}

// isEqual := r.CheckAnnotation(existingK8sRes, newAnnotations) && r.CheckLabel(existingK8sRes, newLabels)
if k8sResConfig != nil {

k8sResConfigDecoded := make(map[string]interface{})
k8sResConfigUnmarshalErr := json.Unmarshal(k8sResConfig.Raw, &k8sResConfigDecoded)
if k8sResConfigUnmarshalErr != nil {
klog.Errorf("failed to unmarshal k8s Resource Config: %v", k8sResConfigUnmarshalErr)
}

Copy link
Contributor

@Daniel-Fan Daniel-Fan Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe we could remove this section.

k8sResConfigDecoded := make(map[string]interface{})
k8sResConfigUnmarshalErr := json.Unmarshal(k8sResConfig.Raw, &k8sResConfigDecoded)

for k, v := range k8sResConfigDecoded {
// isEqual = isEqual && reflect.DeepEqual(existingK8sRes.Object[k], v)
existingK8sRes.Object[k] = v
// Convert existing k8s resource spec to string
existingK8sResRaw, err := json.Marshal(existingK8sRes.Object)
if err != nil {
klog.Error(err)
return false, err
}
}

CRgeneration := existingK8sRes.GetGeneration()
// Merge spec from existing CR and OperandConfig spec
updatedExistingK8sRes := util.MergeCR(existingK8sResRaw, k8sResConfig.Raw)

// if isEqual {
// return true, nil
// }
r.EnsureAnnotation(existingK8sRes, newAnnotations)
r.EnsureLabel(existingK8sRes, newLabels)
if err := r.setOwnerReferences(ctx, &existingK8sRes, ownerReferences); err != nil {
return false, errors.Wrapf(err, "failed to set ownerReferences for k8s resource -- Kind: %s, NamespacedName: %s/%s", kind, namespace, name)
}

r.EnsureAnnotation(existingK8sRes, newAnnotations)
r.EnsureLabel(existingK8sRes, newLabels)
if err := r.setOwnerReferences(ctx, &existingK8sRes, ownerReferences); err != nil {
return false, errors.Wrapf(err, "failed to set ownerReferences for k8s resource -- Kind: %s, NamespacedName: %s/%s", kind, namespace, name)
}
if reflect.DeepEqual(existingK8sRes.Object, updatedExistingK8sRes) {
return true, nil
}
klog.Infof("updating k8s resource with apiversion: %s, kind: %s, %s/%s", apiversion, kind, namespace, name)

klog.V(2).Infof("updating k8s resource with apiversion: %s, kind: %s, %s/%s", apiversion, kind, namespace, name)
existingK8sRes.Object = updatedExistingK8sRes
Copy link
Contributor

@Daniel-Fan Daniel-Fan Mar 25, 2024

Choose a reason for hiding this comment

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

It seems that we are not ensureAnnotation, ensureLabel and setOwnerReferences on updatedObject. Currently it is applying to existingK8sRes

We might need to change the sequence here

  1. deep copy a original version of existingK8sRes.Object - does not exists now.
  2. existingK8sRes.Object = updatedExistingK8sRes
  3. Ensure annotation, label and OwnerReference on new updatedObject
  4. Check if new assigned object is deep equal with original one.


err = r.Update(ctx, &existingK8sRes)
CRgeneration := existingK8sRes.GetGeneration()

if err != nil {
return false, errors.Wrapf(err, "failed to update k8s resource -- Kind: %s, NamespacedName: %s/%s", kind, namespace, name)
}
err = r.Update(ctx, &existingK8sRes)

UpdatedK8sRes := unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": apiversion,
"kind": kind,
},
}
if err != nil {
return false, errors.Wrapf(err, "failed to update k8s resource -- Kind: %s, NamespacedName: %s/%s", kind, namespace, name)
}

err = r.Client.Get(ctx, types.NamespacedName{
Name: name,
Namespace: namespace,
}, &UpdatedK8sRes)
UpdatedK8sRes := unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": apiversion,
"kind": kind,
},
}

if err != nil {
return false, errors.Wrapf(err, "failed to get k8s resource -- Kind: %s, NamespacedName: %s/%s", kind, namespace, name)
err = r.Client.Get(ctx, types.NamespacedName{
Name: name,
Namespace: namespace,
}, &UpdatedK8sRes)

}
if err != nil {
return false, errors.Wrapf(err, "failed to get k8s resource -- Kind: %s, NamespacedName: %s/%s", kind, namespace, name)

if UpdatedK8sRes.GetGeneration() != CRgeneration {
klog.V(2).Infof("Finish updating the k8s Resource: -- Kind: %s, NamespacedName: %s/%s", kind, namespace, name)
}
}

if UpdatedK8sRes.GetGeneration() != CRgeneration {
klog.Infof("Finish updating the k8s Resource: -- Kind: %s, NamespacedName: %s/%s", kind, namespace, name)
}
}
return true, nil
})

Expand Down
19 changes: 19 additions & 0 deletions controllers/util/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,25 @@ func checkKeyBeforeMerging(key string, defaultMap interface{}, changedMap interf
checkKeyBeforeMerging(newKey, defaultMapRef[newKey], changedMapRef[newKey], finalMap[key].(map[string]interface{}))
}
}
case []interface{}:
if changedMap == nil {
finalMap[key] = defaultMap
} else if _, ok := changedMap.([]interface{}); ok { //Check that the changed map value is also a slice []interface
defaultMapRef := defaultMap
changedMapRef := changedMap.([]interface{})
for i := range defaultMapRef {
if _, ok := defaultMapRef[i].(map[string]interface{}); ok {
if len(changedMapRef) <= i {
finalMap[key] = append(finalMap[key].([]interface{}), defaultMapRef[i])
} else {

for newKey := range defaultMapRef[i].(map[string]interface{}) {
checkKeyBeforeMerging(newKey, defaultMapRef[i].(map[string]interface{})[newKey], changedMapRef[i].(map[string]interface{})[newKey], finalMap[key].([]interface{})[i].(map[string]interface{}))
}
}
}
}
}
default:
//Check if the value was set, otherwise set it
if changedMap == nil {
Expand Down