feat: Handle large annotation limit #343

Arvindthiru · 2022-11-21T08:54:47Z

Description of your changes

Fixes #

I have:

Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

Ealianis · 2022-11-30T17:06:39Z

pkg/controllers/work/apply_controller.go


 const (
 	workFieldManagerName = "work-api-agent"
+	fleetSystemNamespace = "fleet-system"


I think it's time for us to create a constants package. We avoided it during the reserved namespace work as we used a prefix. But my current PR is using a string literal for an ObjectKey.

https://github.com/Azure/fleet/pull/346/files#diff-df64e37f2222b761c62bd8bf32c0347aae0fe1f46ff6acf0e66c2b61bd30d761R398

Ealianis · 2022-11-30T17:09:48Z

pkg/controllers/work/apply_controller.go

 const (
 	workFieldManagerName = "work-api-agent"
+	fleetSystemNamespace = "fleet-system"
+	configMapNamePrefix  = "configmap-"


Do we need pkg level constants / vars if they are only used once within the source? I believe I've received that feedback before.

pkg/controllers/work/apply_controller.go

Ealianis · 2022-11-30T18:03:24Z

pkg/controllers/work/patch_util.go

 	return metadataAccessor.SetAnnotations(obj, annotations)
 }

+// setModifiedConfigurationAnnotation serializes the object into byte stream and returns it.


Suggested change

// setModifiedConfigurationAnnotation serializes the object into byte stream and returns it.

// computeModifiedConfiguration serializes the object into a byte stream and returns it.

pkg/controllers/work/apply_controller.go

michaelawyu · 2022-11-30T18:11:25Z

pkg/controllers/work/apply_controller.go

+	generator := names.SimpleNameGenerator
+	var configMapName string
+	var configMap v1.ConfigMap
+	for {


Hi Arvind! Yeah, I agree with Ealianis' comment; this is not a very good pattern, the for-loop here essentially bypasses the exponential back-off mechanism that controller-runtime provides, and in adverse situations the controller might get trapped in an infinite loop.

Plus, the logic here may not work as you would expect; controller-runtime clients use cached reads; if the cache is not fresh enough, name collisions can still occur.

michaelawyu · 2022-11-30T18:12:00Z

pkg/controllers/work/apply_controller.go

 const (
 	workFieldManagerName = "work-api-agent"
+	fleetSystemNamespace = "fleet-system"
+	configMapNamePrefix  = "configmap-"


A nit here: this prefix might be a little generic.

michaelawyu · 2022-11-30T18:14:40Z

pkg/controllers/work/apply_controller.go

 		if err == nil {
 			klog.V(2).InfoS("successfully created the manifest", "gvr", gvr, "manifest", manifestRef)
+			// create configmap
+			if err := r.createConfigMap(ctx, configMapName, owner, manifestHash, lastModifiedConfig); err != nil {


Hi Arvind! The createConfigMap function is repeatedly called throughout the code, though in actuality it only needs to happen once; a little bit refactoring could help make the workflow and the code more clear.

Or more precisely, this place might not be the best one to do this create call.

As we have discussed over the meeting, the name collision check currently in the PR might not work in adverse conditions (e.g. out-of-sync caches), and if the check fails to guard against name collisions, two manifests may share the same configmap and the behavior is undetermined.

Plus, w/o doubly links (or a guaranteed 1:1 name mapping), partial failures may also lead to undetermined behaviors.

Ealianis · 2022-11-30T18:15:49Z

pkg/controllers/work/apply_controller.go

+	// add applied work owner reference. let's check if it works
+	addOwnerRef(owner, configMap)
+	if err := r.spokeClient.Create(ctx, configMap); err != nil {
+		klog.ErrorS(err, "config map create failed", "configMap")


It looks as though multiple error logs will be recorded contain the same message.
See line 716. https://github.com/Azure/fleet/pull/343/files#diff-9c51763423a3a9a026f47bc21555619edb62a1cf676c669b643e57b53c39c7caR716

michaelawyu · 2022-11-30T18:31:36Z

pkg/controllers/work/apply_controller.go

+// We can calculate the manifestHash & lastAppliedConfig again wiping out previous state or make sure the annotations in the last applied config doesn't cause issue in patch.
+func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference) error {
+	// we can also use obj.getAnnotations().
+	annots, err := metadataAccessor.Annotations(obj)


Hi Arvind! We might need to double-confirm this; for unstructured objects, this function seems to return a copy of the annotations map, but in this piece of code all the updates are in-place.

See also:
https://github.com/kubernetes/apimachinery/blob/4fbe8e4db39b17a53c0756caf58466baad871871/pkg/apis/meta/v1/unstructured/unstructured.go#L407
https://github.com/kubernetes/apimachinery/blob/4fbe8e4db39b17a53c0756caf58466baad871871/pkg/apis/meta/v1/unstructured/helpers.go#L166

michaelawyu · 2022-11-30T18:32:58Z

pkg/controllers/work/apply_controller.go

+	if ok1 || ok2 {
+		delete(annots, manifestHashAnnotation)
+		delete(annots, lastAppliedConfigAnnotation)
+		configMapName, err := r.getRandomConfigMapName(ctx)


(See the comment above about refactoring the createConfigMap call; a little bit refactoring can help here)

michaelawyu · 2022-11-30T18:36:03Z

pkg/controllers/work/apply_controller.go

+	if !ok {
+		return nil, errors.New("manifest doesn't have a config map")
+	}
+	if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil {


Hi Arvind! I understand the purpose of this code snippet here, but it could really use some refactoring.

The createConfigMap call happens for the third time

It really is not a very good idea to use recursion here.

michaelawyu · 2022-11-30T18:44:13Z

pkg/controllers/work/apply_controller.go

+	setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj)
+
+	// Use manifest generation to check if configmap is outdated
+	if curObj.GetGeneration() != curObjConfigMap.GetGeneration() {


Hi Arvind! A few issues here:

first of all, IIRC Kubernetes API server does not track generations for configMaps; the field will always be 0

This checks compares the generations of two very different objects, which will only work if the two values are in-sync (there are no guarantees)

Yes, was checking the logs and realized the generation for config map is always zero and the manifest's generation is also zero after patch

michaelawyu · 2022-11-30T18:45:16Z

pkg/controllers/work/apply_controller.go

 	// We only try to update the object if its spec hash value has changed.
-	if manifestObj.GetAnnotations()[manifestHashAnnotation] != curObj.GetAnnotations()[manifestHashAnnotation] {
-		return r.patchCurrentResource(ctx, gvr, manifestObj, curObj)
+	if manifestHash != curObjConfigMap.Data[manifestHashKey] {


Hi Arvind! This branch will never run if I understand it correctly; the code above just has the configMap updates so the hashes will always match, right?

Ideally the configmap would get updated only if it's outdated meaning a we patched the object but for some reason we were not able to update the configmap

michaelawyu · 2022-11-30T18:48:31Z

pkg/controllers/work/apply_controller.go

 	}
+
+	// calculate manifest hash & last modified config again to set the new manifest hash & last applied config after patch gets applied
+	if err := r.updateConfigMap(ctx, curObjConfigMap, manifestObj); err != nil {


Hi Arvind! Just to double-check, is it really necessary to update the configmap for a second time here?

This need to be updated with the object from the hub cluster

michaelawyu · 2022-11-30T18:52:17Z

pkg/controllers/work/patch_util.go

 // threeWayMergePatch creates a patch by computing a three-way diff based on
 // an object's current state, modified state, and last-applied-state recorded in its annotation.
-func threeWayMergePatch(currentObj, manifestObj client.Object) (client.Patch, error) {
+func threeWayMergePatch(currentObj, manifestObj client.Object, currentObjConfigMap *v1.ConfigMap) (client.Patch, error) {


Hi Arvind! It might be helpful to take another look at the implementation of the CreateThreeWayMergePatch function; the semantics do not seem to correlate with the args passed in at this moment.

Discussed in the meeting, the args match

…xt exits (Azure#343) * Minor changes Signed-off-by: michaelawyu <chenyu1@microsoft.com> * Minor fixes Signed-off-by: michaelawyu <chenyu1@microsoft.com> * Minor fixes Signed-off-by: michaelawyu <chenyu1@microsoft.com> --------- Signed-off-by: michaelawyu <chenyu1@microsoft.com>

Arvindthiru added 3 commits November 21, 2022 00:53

Not working

aeb9cf9

Working update

7c095bd

compute manifestHash again

8fa0974

Arvindthiru changed the title ~~Feat: Handle large annotation limit~~ feat: Handle large annotation limit Nov 22, 2022

Arvindthiru added 12 commits November 22, 2022 19:46

More comments to address

50a7227

Address issues

56c4391

fix migrateToConfigMap

8a8006e

clean up code

af67b7f

Make methods readable

4a15973

Minor fixes

e168bf5

Remove comments

40c224a

Concern comments

ef2e3a9

Use generateName for configMap

ba87225

refactor code

f189468

testing

04a0414

edit comment

61552a6

Arvindthiru force-pushed the LargeAnnotation branch from d5a52d4 to 61552a6 Compare November 30, 2022 08:46

Ealianis reviewed Nov 30, 2022

View reviewed changes

pkg/controllers/work/apply_controller.go Outdated Show resolved Hide resolved

fix getRandomConfigMapName

25e0f1a

Ealianis reviewed Nov 30, 2022

View reviewed changes

pkg/controllers/work/apply_controller.go Outdated Show resolved Hide resolved

michaelawyu reviewed Nov 30, 2022

View reviewed changes

Ealianis reviewed Nov 30, 2022

View reviewed changes

Use manifest key for check

f3e0007

michaelawyu reviewed Nov 30, 2022

View reviewed changes

Arvindthiru added 7 commits December 1, 2022 13:59

Address comments

1fcf980

Changes

68ee8f1

Handle configmap deletion

24a9538

refactor getConfigMap

d7d4949

fix getConfigMap

8e750a0

clean up

338e783

check for annotations is nil

70f9313

Arvindthiru force-pushed the LargeAnnotation branch from f71efe3 to 70f9313 Compare December 2, 2022 18:30

Arvindthiru added 8 commits December 2, 2022 10:34

fix comment

ec366cf

fix manifestIdentifier

35b467c

fix check

ad0aa40

fix check

504fc4c

changes

f9f3e33

Changes

514e475

Version check for config map

a0f41d7

change

16f8d37

Arvindthiru closed this Feb 2, 2023

weng271190436 mentioned this pull request Dec 1, 2025

chore: backport 20251201 #1238

Merged

1 task

	// setModifiedConfigurationAnnotation serializes the object into byte stream and returns it.
	// computeModifiedConfiguration serializes the object into a byte stream and returns it.

feat: Handle large annotation limit #343

feat: Handle large annotation limit #343

Uh oh!

Conversation

Arvindthiru commented Nov 21, 2022

Description of your changes

How has this code been tested

Special notes for your reviewer

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ealianis Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelawyu Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelawyu Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelawyu Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ealianis Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arvindthiru Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ealianis Nov 30, 2022 •

edited

Loading

michaelawyu Nov 30, 2022 •

edited

Loading

michaelawyu Nov 30, 2022 •

edited

Loading

michaelawyu Nov 30, 2022 •

edited

Loading

Ealianis Nov 30, 2022 •

edited

Loading

Arvindthiru Nov 30, 2022 •

edited

Loading