cc from original PR: @bgrant0607, @ghodss, @janetkuo, @mikedanese, @jlowdermilk, @jackgr

Full discussion was in #1702, with earlier discussion in #1178.

For users of kubectl apply, the annotations are not unnecessary, as they record the previous configuration specified by the user, which otherwise is not recorded anywhere, for use in the 3-way merge, the other 2 inputs being the new configuration and the live API state.

Originally I intended only kubectl apply to set/update these annotations, so that users/clients that didn't use kubectl apply would not be affected. We could potentially revert to that behavior, at the cost of making kubectl apply somewhat less convenient.

When using kubectl apply against older or non-kubectl clients, it's true that the first apply operation doesn't have the benefit of the annotations, so removal of items from the spec cannot be inferred automatically.

const totalAnnotationSizeLimitB int = 64 * (1 << 10) // 64 kB

We could double the total annotation size limit, or exempt these annotations from the limit, or remove the limit, in favor of size limits enforced via LimitRange and ResourceQuota.

In the future we might store this information in separate API resources, but that won't reduce the amount of storage consumed.

The apply command can do a 2 way merge between the current state of the API object and desired state client-side. By definition, there are only 2 things to compare in such a scenario, so a 3 way merge is not possible. When the annotation is present, it defines a third version of the data, so then a 3 way merge is possible.

Regarding the benefit, it allows users to modify previous configuration and reapply it. Without the annotation, there's no way to tell what the user has removed since the previous version, and therefore no way to know what can safely be removed from the current state.

Regarding the size increase, can we resolve the problem with the annotation size limit by increasing it?

For users of kubectl apply, the annotations are not unnecessary, as they record the previous configuration specified by the user, which otherwise is not recorded anywhere, for use in the 3-way merge, the other 2 inputs being the new configuration and the live API state.

I see. It's unfortunate we don't have access to historic resourceVersions via the API. Putting copies of old versions in as annotations seems like a really messy way to accomplish that.

Originally I intended only kubectl apply to set/update these annotations, so that users/clients that didn't use kubectl apply would not be affected. We could potentially revert to that behavior, at the cost of making kubectl apply somewhat less convenient.

Updating the output of so many commands, and affecting all objects in the system created by someone just using kubectl seems like a lot of impact to support one feature. Can we start by making the annotation opt-in (via an invocation of apply or an option to the mutating commands), or making the historic and desired versions be input to the apply command?

can we resolve the problem with the annotation size limit by increasing it?

That seems like a bandaid. Were performance implications considered?

cc @smarterclayton @deads2k

@liggitt Access to historic resourceVersions via the API is not sufficient. @jackgr or I should write up a design doc at some point, but you can see a concrete example here: #13007 (comment)

We could make apply opt-in by only creating the annotations in apply, and otherwise only updating them if they already exist.

Re. performance: The impact was not measured, no. I'm not concerned so much about the performance of apply itself. In general, we need to move everything towards writing and reading (#1459) just the parts of resources they need to touch.

Note that pods, which should be the most numerous and most frequently updated resources, should not normally have the annotations applied, since users should be creating pods via controllers.

cc @wojtek-t re. whether we've observed a performance impact

Since this halves the space available for annotations on every resource, I
think it should be more narrowly scoped. Doubling the annotation size I
think puts us above the etcd 2.0 value limit.

On Oct 19, 2015, at 6:22 PM, Brian Grant notifications@github.com wrote:

@liggitt https://github.com/liggitt Access to historic resourceVersions
via the API is not sufficient. @jackgr https://github.com/jackgr or I
should write up a design doc at some point, but you can see a concrete
example here: #13007 (comment)
#13007 (comment)

We could make apply opt-in by only creating the annotations in apply, and
otherwise only updating them if they already exist.

Re. performance: The impact was not measured, no. I'm not concerned so much
about the performance of apply itself. In general, we need to move
everything towards writing and reading (#1459
#1459) just the parts of
resources they need to touch.

Note that pods, which should be the most numerous and most frequently
updated resources, should not normally have the annotations applied, since
users should be creating pods via controllers.

—
Reply to this email directly or view it on GitHub
#15878 (comment)
.

Less than half, potentially. If you create a replication controller with a large number of containers or volumes, etc, you could potentially hit the size limit with just the apply annotation. Likely, no, but core commands having edge cases like that concerns me.

@liggitt What scenario are you seeing where resources are close the size limit? 64K seems like a lot.

What is the etcd value size limit? I see a limit of 1MB for raft messages.

BTW, annotations were originally added to the system for this precise purpose (#1201), so I'd like to find a way to make them work.

Slightly less than that due to metadata. Are we only at 64k for
annotations? I thought we allowed up to 240k - must be misremembering.

On Mon, Oct 19, 2015 at 9:41 PM, Brian Grant notifications@github.com
wrote:

What is the etcd value size limit? I see a limit of 1MB for raft messages.

—
Reply to this email directly or view it on GitHub
#15878 (comment)
.

@smarterclayton See the line of code setting totalAnnotationSizeLimitB that I copy-pasted above.

Fair, but for components that are already doing that it doubles the size of
those objects (OpenShift encodes the current deployment config into an RC,
but we would now need to strip that field). Not a total obstacle, but it
does feel aggressive to carve out that state assuming every CLI operation
would eventually result in an apply. I had shared Jordan's assumption that
only resources created via apply would inherit this state.

On Mon, Oct 19, 2015 at 9:44 PM, Brian Grant notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton See the line of code
setting totalAnnotationSizeLimitB that I copy-pasted above.

—
Reply to this email directly or view it on GitHub
#15878 (comment)
.

What scenario are you seeing where resources are close the size limit? 64K seems like a lot.

• A secret containing several files in its data map (I would assume any sort of config object as well, once that gets figured out)
• OpenShift's template object (essentially a persisted List + parameter definitions), which also had the distinction of running up against the 64k annotation limit in practice, as template providers wanted to use annotations to store things like icons and descriptions

It's especially painful when this annotation makes the object fail with a "FieldValueTooLong" error on the annotations field when the user didn't actually ask for any annotations.

I propose the following for 1.1:

1. Only apply should create the other annotations. Other commands may update them if they exist, but should not add the annotations if they do not.
2. Increase the annotation size limit.

For 1.2 or later we should consider:

1. Elide field values where possible (e.g., large secret data values). Note that values of strategic merge key fields can't be omitted.
2. Better error messages.
3. Resource size enforcement (i.e., not just/specifically annotation size).
4. Aggregate resource size enforcement via ResourceQuota.

+1

Jack Greenfield | Google

On Tue, Oct 20, 2015 at 2:11 PM, Brian Grant notifications@github.com
wrote:

I propose the following for 1.1:

1. Only apply should create the other annotations. Other commands may
   update them if they exist, but should not add the annotations if they do
   not.
2. Increase the annotation size limit.

For 1.2 or later we should consider:

1. Elide field values where possible (e.g., large secret data values).
   Note that values of strategic merge key fields can't be omitted.
2. Better error messages.
3. Resource size enforcement (i.e., not just/specifically annotation size).
4. Aggregate resource size enforcement via ResourceQuota.

—
Reply to this email directly or view it on GitHub
#15878 (comment)
.

Thanks, +1

On Tue, Oct 20, 2015 at 5:12 PM, Jack Greenfield notifications@github.com
wrote:

+1

Jack Greenfield | Google

On Tue, Oct 20, 2015 at 2:11 PM, Brian Grant notifications@github.com
wrote:

I propose the following for 1.1:

1. Only apply should create the other annotations. Other commands may
   update them if they exist, but should not add the annotations if they do
   not.
2. Increase the annotation size limit.

For 1.2 or later we should consider:

1. Elide field values where possible (e.g., large secret data values).
   Note that values of strategic merge key fields can't be omitted.
2. Better error messages.
3. Resource size enforcement (i.e., not just/specifically annotation
   size).
4. Aggregate resource size enforcement via ResourceQuota.

—
Reply to this email directly or view it on GitHub
<
#15878 (comment)

.

—
Reply to this email directly or view it on GitHub
#15878 (comment)
.

Proposed annotation size limit: 256k?

256k doesn't seem insane, though with the primary fetch mechanism being lists of all objects of a type, with no field filtering, I really hope most objects don't make use of that headroom

I agree we're going to need field filtering.

But not for 1.1.

Waiting on cherrypick of #16207 @janetkuo

Done

can this be a little larger?

for istio EnvoyFilter grpc descriptor bin file is not large enough.

@elvizlai please propose a PR with the change you need and we can debate there. Folks don't usually look at chatter in closed issues.