feat: Annotation size limit #355

Arvindthiru · 2023-02-02T08:13:49Z

Description of your changes

The changes in the PR validates the annotation size of the last modified config annotation on the manifest and if it crosses the annotation size threshold it removes the annotation and creates/updates the object on the cluster using server side apply instead of using three way merge.

This ensures that when large objects are propagated we don't fail to apply the object

Fixes #295

I have:

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

How has this code been tested

Unit tests

Special notes for your reviewer

Redis guide: https://techcommunity.microsoft.com/t5/apps-on-azure-blog/run-scalable-and-resilient-redis-with-kubernetes-and-azure/ba-p/3247956

Propagated redis helm secret to member cluster

https://blog.atomist.com/kubernetes-apply-replace-patch/

https://stackoverflow.com/questions/28459418/use-of-put-vs-patch-methods-in-rest-api-real-life-scenarios/39338329#39338329

pkg/controllers/work/apply_controller.go

pkg/controllers/work/apply_controller_test.go

pkg/controllers/work/apply_controller.go

pkg/utils/test_util.go

pkg/controllers/work/apply_controller.go

zhiying-lin

please add e2e tests to cover create/update the large secret.
please update your PR description to indicate how/why your PR fixes the bug.

pkg/controllers/work/apply_controller.go

pkg/controllers/work/apply_controller_test.go

pkg/utils/test_util.go

pkg/controllers/work/apply_controller_test.go

pkg/utils/test_util.go

Arvindthiru · 2023-05-09T16:34:18Z

Putting the PR as draft to work on E2E

Arvindthiru · 2023-05-17T17:40:23Z

Instead of removing the last applied config annotation I'm currently setting the annotation to "". This is done because of the scenario described below.

Create an object within the annotation limit, we get an object with spec-hash and last modified config annotations on it
Then when we update the object to be large, we use server side apply to update the object but since it's server side apply even if we remove the annotation from the object in the controller it sees the pre-existing last applied config and since the controller is not managing the field now it leaves it alone and updates other fields
This is not the correct behavior as we have a large object with the small object's last applied config
To avoid this we can update the field to be empty, and the next time around when we update the object to be small three way merge works fine as it accepts empty string as last applied config it uses an empty map

pkg/controllers/work/apply_controller_test.go

test/e2e/work_load_test.go

pkg/controllers/work/apply_controller.go

pkg/controllers/work/apply_controller_test.go

pkg/controllers/work/patch_util.go

pkg/controllers/work/apply_controller_test.go

pkg/controllers/work/patch_util.go

zhiying-lin

one minor comment, others LGTM

pkg/controllers/work/apply_controller.go

pkg/controllers/work/manager.go

pkg/controllers/work/apply_controller.go

zhiying-lin · 2023-06-12T09:44:18Z

pkg/controllers/work/patch_util_test.go

+	for testName, testCase := range tests {
+		t.Run(testName, func(t *testing.T) {
+			gotBool, gotErr := setModifiedConfigurationAnnotation(testCase.obj)
+			assert.Equalf(t, testCase.wantBool, gotBool, "got bool not matching for Testcase %s", testName)


Can we try to avoid the Assert libraries in the UT?

"Avoid the use of 'assert' libraries to help your tests. "
https://github.com/golang/go/wiki/TestComments#assert-libraries

Signed-off-by: Zhiying Lin <zhiyingl456@gmail.com>

Ealianis reviewed Feb 2, 2023

View reviewed changes

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

Ealianis reviewed Feb 3, 2023

View reviewed changes

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

ryanzhang-oss reviewed Feb 3, 2023

View reviewed changes

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

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

Arvindthiru commented Feb 6, 2023

View reviewed changes

pkg/utils/test_util.go Outdated Show resolved Hide resolved

Arvindthiru marked this pull request as ready for review February 8, 2023 01:04

michaelawyu reviewed Apr 7, 2023

View reviewed changes

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

zhiying-lin requested changes Apr 23, 2023

View reviewed changes

Arvindthiru force-pushed the AnnotationSizeLimit branch from 17b3b12 to 956f1b4 Compare April 24, 2023 21:46

Arvindthiru marked this pull request as draft May 9, 2023 16:34

Arvindthiru force-pushed the AnnotationSizeLimit branch from 59176d9 to bd6e3c9 Compare May 17, 2023 01:02

Arvindthiru marked this pull request as ready for review May 17, 2023 01:05

zhiying-lin reviewed May 26, 2023

View reviewed changes

Arvindthiru force-pushed the AnnotationSizeLimit branch from bd6e3c9 to c541e01 Compare May 29, 2023 23:41

Arvindthiru commented May 29, 2023

View reviewed changes

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

zhiying-lin reviewed May 30, 2023

View reviewed changes

Arvindthiru force-pushed the AnnotationSizeLimit branch from 98a178b to 9999305 Compare May 31, 2023 20:50

zhiying-lin reviewed Jun 1, 2023

View reviewed changes

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

zhiying-lin previously approved these changes Jun 1, 2023

View reviewed changes

ryanzhang-oss reviewed Jun 2, 2023

View reviewed changes

Arvindthiru dismissed zhiying-lin’s stale review via e0b4eee June 7, 2023 17:46

Arvindthiru force-pushed the AnnotationSizeLimit branch 2 times, most recently from 9e7aecc to e0b4eee Compare June 7, 2023 17:50

Arvindthiru added 6 commits June 8, 2023 12:01

Annotation limit check

88a4026

unit tests

9ae7b1c

fix UT

ff48105

fix lint

8b10d12

Address comments

16de59e

fix UT, add comments

377e696

Arvindthiru added 22 commits June 8, 2023 12:01

refactor err checks

41d297e

fix comment

9bbe034

fix comment

4382ba5

refactor err check

70532f0

update comment

d90245c

force conflicts on apply

6054be2

set last applied config to empty string

7ec1f95

Working E2E

6922a6a

fix UT

65af7f7

fix integration

7dfff4a

update comment

f70588d

improve comment

b4924d3

add period for comments

103556a

address comments

d8a8436

fix UT, add comment

f2bede5

add period

4d436f9

add UTs

38fdab3

address comments

d7b29e0

fix comment

32a51c8

fix comment

cf063fd

address comments

d19b99d

address comments

5f8ddf0

Arvindthiru force-pushed the AnnotationSizeLimit branch from bd71313 to 5f8ddf0 Compare June 8, 2023 19:01

Arvindthiru added 2 commits June 8, 2023 12:03

fix comment

1f28abd

fix import order

eee4cd2

ryanzhang-oss approved these changes Jun 8, 2023

View reviewed changes

ryanzhang-oss merged commit 92ae7ec into Azure:main Jun 8, 2023

zhiying-lin reviewed Jun 12, 2023

View reviewed changes

weng271190436 pushed a commit that referenced this pull request Jan 6, 2026

chore: fix the collect-logs.sh (#355)

1e1765e

Signed-off-by: Zhiying Lin <zhiyingl456@gmail.com>

britaniar mentioned this pull request Jan 12, 2026

chore: Jan backport (01/12/2026) #1246

Merged

1 task

feat: Annotation size limit #355

feat: Annotation size limit #355

Uh oh!

Conversation

Arvindthiru commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

How has this code been tested

Special notes for your reviewer

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhiying-lin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Arvindthiru commented May 9, 2023

Uh oh!

Arvindthiru commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhiying-lin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhiying-lin Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Arvindthiru commented Feb 2, 2023 •

edited

Loading

Arvindthiru commented May 17, 2023 •

edited

Loading