Skip to content

UPSTREAM: <carry>: do not set the generation for cached object #106

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
merged 1 commit into from
Oct 4, 2022
Merged

UPSTREAM: <carry>: do not set the generation for cached object #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/validation/field"
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/apiserver/pkg/features"
apiserverstorage "k8s.io/apiserver/pkg/storage"
"k8s.io/apiserver/pkg/storage/names"
Expand Down Expand Up @@ -114,6 +115,15 @@ func (a customResourceStrategy) PrepareForCreate(ctx context.Context, obj runtim
}

accessor, _ := meta.Accessor(obj)
if _, found := accessor.GetAnnotations()[genericapirequest.AnnotationKey]; found {
// in general the shard annotation is not attached to objects, instead, it is assigned by the storage layer on the fly
// to avoid an additional UPDATE request (mismatch on the generation field) replicated objects have the shard annotation set
// thus we need to remove the shard annotation and simply return early so that the generation is not reset to 1
annotations := accessor.GetAnnotations()
delete(annotations, genericapirequest.AnnotationKey)
accessor.SetAnnotations(annotations)
Copy link
Member

@ncdc ncdc Oct 3, 2022

Choose a reason for hiding this comment

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

I don't believe you need to get/set the map. You should be able to delete directly. https://go.dev/play/p/r7Wr8PbmwN9. Hopefully meta.Accessor doesn't change that?

Copy link
Author

@p0lyn0mial p0lyn0mial Oct 4, 2022

Choose a reason for hiding this comment

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

ah, right, recently I have worked with an unstructured object that gave me a copy, here indeed we get a pointer.

return
}
accessor.SetGeneration(1)
}

Expand Down Expand Up @@ -144,6 +154,11 @@ func (a customResourceStrategy) PrepareForUpdate(ctx context.Context, obj, old r
if !apiequality.Semantic.DeepEqual(newCopyContent, oldCopyContent) {
oldAccessor, _ := meta.Accessor(oldCustomResourceObject)
newAccessor, _ := meta.Accessor(newCustomResourceObject)
if _, found := oldAccessor.GetAnnotations()[genericapirequest.AnnotationKey]; found {
// the presence of the annotation indicates the object is from the cache server.
// since the objects from the cache should not be modified in any way, just return early.
return
}
newAccessor.SetGeneration(oldAccessor.GetGeneration() + 1)
}
}
Expand Down
7 changes: 6 additions & 1 deletion staging/src/k8s.io/apiserver/pkg/registry/rest/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,12 @@ func BeforeUpdate(strategy RESTUpdateStrategy, ctx context.Context, obj, old run
if err != nil {
return err
}
objectMeta.SetGeneration(oldMeta.GetGeneration())
if len(oldMeta.GetAnnotations()[genericapirequest.AnnotationKey]) == 0 || objectMeta.GetGeneration() == 0 {
Copy link
Author

@p0lyn0mial p0lyn0mial Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think that comparing to an empty string is more readable but I can change to _, found := accessor.GetAnnotations()[genericapirequest.AnnotationKey]; !found

// the absence of the annotation indicates the object is NOT from the cache server,
// if the new object doesn't have its generation set, just rewrite it from the old object
// otherwise we are dealing with an object from the cache server that wants its generation to be updated
objectMeta.SetGeneration(oldMeta.GetGeneration())
}

// Ensure managedFields state is removed unless ServerSideApply is enabled
if !utilfeature.DefaultFeatureGate.Enabled(features.ServerSideApply) {
Expand Down