Skip to content
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

[GarbageCollector] Add orphaning finalizer logic to GC #25599

Merged
merged 1 commit into from
May 26, 2016
Merged

[GarbageCollector] Add orphaning finalizer logic to GC #25599

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
2 changes: 2 additions & 0 deletions cmd/kube-apiserver/app/options/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
genericoptions "k8s.io/kubernetes/pkg/genericapiserver/options"
kubeletclient "k8s.io/kubernetes/pkg/kubelet/client"
"k8s.io/kubernetes/pkg/master/ports"
"k8s.io/kubernetes/pkg/registry/generic/registry"

"github.com/spf13/pflag"
)
Expand Down Expand Up @@ -83,4 +84,5 @@ func (s *APIServer) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&s.KubeletConfig.CAFile, "kubelet-certificate-authority", s.KubeletConfig.CAFile, "Path to a cert. file for the certificate authority.")
// TODO: delete this flag as soon as we identify and fix all clients that send malformed updates, like #14126.
fs.BoolVar(&validation.RepairMalformedUpdates, "repair-malformed-updates", validation.RepairMalformedUpdates, "If true, server will do its best to fix the update request to pass the validation, e.g., setting empty UID in update request to its existing value. This flag can be turned off after we fix all the clients that send malformed updates.")
fs.BoolVar(&registry.EnableGarbageCollector, "enable-garbage-collector", false, "Enables the generic garbage collector. MUST be synced with the corresponding flag of the kube-controller-manager.")
}
3 changes: 2 additions & 1 deletion docs/admin/kube-apiserver.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ kube-apiserver
--cors-allowed-origins=[]: List of allowed origins for CORS, comma separated. An allowed origin can be a regular expression to support subdomain matching. If this list is empty CORS will not be enabled.
--delete-collection-workers=1: Number of workers spawned for DeleteCollection call. These are used to speed up namespace cleanup.
--deserialization-cache-size=50000: Number of deserialized json objects to cache in memory.
--enable-garbage-collector[=false]: Enables the generic garbage collector. MUST be synced with the corresponding flag of the kube-controller-manager.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to say something about this being an alpha feature and admins should be careful and understand the risks before setting this to true? :)

--enable-swagger-ui[=false]: Enables swagger ui on the apiserver at /swagger-ui
--etcd-cafile="": SSL Certificate Authority file used to secure etcd communication
--etcd-certfile="": SSL certification file used to secure etcd communication
Expand Down Expand Up @@ -123,7 +124,7 @@ kube-apiserver
--watch-cache-sizes=[]: List of watch cache sizes for every resource (pods, nodes, etc.), comma separated. The individual override format: resource#size, where size is a number. It takes effect when watch-cache is enabled.
```

###### Auto generated by spf13/cobra on 18-May-2016
###### Auto generated by spf13/cobra on 23-May-2016


<!-- BEGIN MUNGE: GENERATED_ANALYTICS -->
Expand Down
1 change: 1 addition & 0 deletions hack/verify-flags/known-flags.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ enable-debugging-handlers
enable-hostpath-provisioner
enable-server
enable-swagger-ui
enable-garbage-collector
etcd-cafile
etcd-certfile
etcd-config
Expand Down
4 changes: 3 additions & 1 deletion pkg/api/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,9 @@ func IsServiceIPRequested(service *Service) bool {
}

var standardFinalizers = sets.NewString(
string(FinalizerKubernetes))
string(FinalizerKubernetes),
FinalizerOrphan,
)

func IsStandardFinalizerName(str string) bool {
return standardFinalizers.Has(str)
Expand Down
2 changes: 2 additions & 0 deletions pkg/api/meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ func (meta *ObjectMeta) GetLabels() map[string]string { return m
func (meta *ObjectMeta) SetLabels(labels map[string]string) { meta.Labels = labels }
func (meta *ObjectMeta) GetAnnotations() map[string]string { return meta.Annotations }
func (meta *ObjectMeta) SetAnnotations(annotations map[string]string) { meta.Annotations = annotations }
func (meta *ObjectMeta) GetFinalizers() []string { return meta.Finalizers }
func (meta *ObjectMeta) SetFinalizers(finalizers []string) { meta.Finalizers = finalizers }

func (meta *ObjectMeta) GetOwnerReferences() []metatypes.OwnerReference {
ret := make([]metatypes.OwnerReference, len(meta.OwnerReferences))
Expand Down
2 changes: 2 additions & 0 deletions pkg/api/meta/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ type Object interface {
SetLabels(labels map[string]string)
GetAnnotations() map[string]string
SetAnnotations(annotations map[string]string)
GetFinalizers() []string
SetFinalizers(finalizers []string)
GetOwnerReferences() []metatypes.OwnerReference
SetOwnerReferences([]metatypes.OwnerReference)
}
Expand Down
15 changes: 15 additions & 0 deletions pkg/api/meta/meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ type genericAccessor struct {
labels *map[string]string
annotations *map[string]string
ownerReferences reflect.Value
finalizers *[]string
}

func (a genericAccessor) GetNamespace() string {
Expand Down Expand Up @@ -527,6 +528,17 @@ func (a genericAccessor) SetAnnotations(annotations map[string]string) {
*a.annotations = annotations
}

func (a genericAccessor) GetFinalizers() []string {
if a.finalizers == nil {
return nil
}
return *a.finalizers
}

func (a genericAccessor) SetFinalizers(finalizers []string) {
*a.finalizers = finalizers
}

func (a genericAccessor) GetOwnerReferences() []metatypes.OwnerReference {
var ret []metatypes.OwnerReference
s := a.ownerReferences
Expand Down Expand Up @@ -599,6 +611,9 @@ func extractFromObjectMeta(v reflect.Value, a *genericAccessor) error {
if err := runtime.FieldPtr(v, "Annotations", &a.annotations); err != nil {
return err
}
if err := runtime.FieldPtr(v, "Finalizers", &a.finalizers); err != nil {
return err
}
ownerReferences := v.FieldByName("OwnerReferences")
if !ownerReferences.IsValid() {
return fmt.Errorf("struct %#v lacks OwnerReferences type", v)
Expand Down
33 changes: 33 additions & 0 deletions pkg/api/meta/meta_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ func TestAPIObjectMeta(t *testing.T) {
SelfLink: "some/place/only/we/know",
Labels: map[string]string{"foo": "bar"},
Annotations: map[string]string{"x": "y"},
Finalizers: []string{
"finalizer.1",
"finalizer.2",
},
},
}
var _ meta.Object = &j.ObjectMeta
Expand Down Expand Up @@ -72,6 +76,9 @@ func TestAPIObjectMeta(t *testing.T) {
if e, a := "some/place/only/we/know", accessor.GetSelfLink(); e != a {
t.Errorf("expected %v, got %v", e, a)
}
if e, a := []string{"finalizer.1", "finalizer.2"}, accessor.GetFinalizers(); !reflect.DeepEqual(e, a) {
t.Errorf("expected %v, got %v", e, a)
}

typeAccessor, err := meta.TypeAccessor(j)
if err != nil {
Expand All @@ -92,6 +99,7 @@ func TestAPIObjectMeta(t *testing.T) {
typeAccessor.SetKind("d")
accessor.SetResourceVersion("2")
accessor.SetSelfLink("google.com")
accessor.SetFinalizers([]string{"finalizer.3"})

// Prove that accessor changes the original object.
if e, a := "baz", j.Namespace; e != a {
Expand All @@ -118,6 +126,9 @@ func TestAPIObjectMeta(t *testing.T) {
if e, a := "google.com", j.SelfLink; e != a {
t.Errorf("expected %v, got %v", e, a)
}
if e, a := []string{"finalizer.3"}, j.Finalizers; !reflect.DeepEqual(e, a) {
t.Errorf("expected %v, got %v", e, a)
}

typeAccessor.SetAPIVersion("d")
typeAccessor.SetKind("e")
Expand All @@ -143,6 +154,7 @@ func TestGenericTypeMeta(t *testing.T) {
Labels map[string]string `json:"labels,omitempty"`
Annotations map[string]string `json:"annotations,omitempty"`
OwnerReferences []api.OwnerReference `json:"ownerReferences,omitempty"`
Finalizers []string `json:"finalizers,omitempty"`
}
type Object struct {
TypeMeta `json:",inline"`
Expand All @@ -159,6 +171,7 @@ func TestGenericTypeMeta(t *testing.T) {
SelfLink: "some/place/only/we/know",
Labels: map[string]string{"foo": "bar"},
Annotations: map[string]string{"x": "y"},
Finalizers: []string{"finalizer.1", "finalizer.2"},
},
}
accessor, err := meta.Accessor(&j)
Expand All @@ -183,6 +196,9 @@ func TestGenericTypeMeta(t *testing.T) {
if e, a := "some/place/only/we/know", accessor.GetSelfLink(); e != a {
t.Errorf("expected %v, got %v", e, a)
}
if e, a := []string{"finalizer.1", "finalizer.2"}, accessor.GetFinalizers(); !reflect.DeepEqual(e, a) {
t.Errorf("expected %v, got %v", e, a)
}

typeAccessor, err := meta.TypeAccessor(&j)
if err != nil {
Expand All @@ -203,6 +219,7 @@ func TestGenericTypeMeta(t *testing.T) {
typeAccessor.SetKind("d")
accessor.SetResourceVersion("2")
accessor.SetSelfLink("google.com")
accessor.SetFinalizers([]string{"finalizer.3"})

// Prove that accessor changes the original object.
if e, a := "baz", j.Namespace; e != a {
Expand All @@ -229,6 +246,9 @@ func TestGenericTypeMeta(t *testing.T) {
if e, a := "google.com", j.SelfLink; e != a {
t.Errorf("expected %v, got %v", e, a)
}
if e, a := []string{"finalizer.3"}, j.Finalizers; !reflect.DeepEqual(e, a) {
t.Errorf("expected %v, got %v", e, a)
}

typeAccessor.SetAPIVersion("d")
typeAccessor.SetKind("e")
Expand All @@ -252,6 +272,7 @@ type InternalTypeMeta struct {
APIVersion string `json:"apiVersion,omitempty"`
Labels map[string]string `json:"labels,omitempty"`
Annotations map[string]string `json:"annotations,omitempty"`
Finalizers []string `json:"finalizers,omitempty"`
OwnerReferences []api.OwnerReference `json:"ownerReferences,omitempty"`
}

Expand Down Expand Up @@ -435,6 +456,7 @@ func TestGenericObjectMeta(t *testing.T) {
ResourceVersion string `json:"resourceVersion,omitempty"`
Labels map[string]string `json:"labels,omitempty"`
Annotations map[string]string `json:"annotations,omitempty"`
Finalizers []string `json:"finalizers,omitempty"`
OwnerReferences []api.OwnerReference `json:"ownerReferences,omitempty"`
}
type Object struct {
Expand All @@ -455,6 +477,10 @@ func TestGenericObjectMeta(t *testing.T) {
SelfLink: "some/place/only/we/know",
Labels: map[string]string{"foo": "bar"},
Annotations: map[string]string{"a": "b"},
Finalizers: []string{
"finalizer.1",
"finalizer.2",
},
},
}
accessor, err := meta.Accessor(&j)
Expand Down Expand Up @@ -485,6 +511,9 @@ func TestGenericObjectMeta(t *testing.T) {
if e, a := 1, len(accessor.GetAnnotations()); e != a {
t.Errorf("expected %v, got %v", e, a)
}
if e, a := []string{"finalizer.1", "finalizer.2"}, accessor.GetFinalizers(); !reflect.DeepEqual(e, a) {
t.Errorf("expected %v, got %v", e, a)
}

typeAccessor, err := meta.TypeAccessor(&j)
if err != nil {
Expand All @@ -507,6 +536,7 @@ func TestGenericObjectMeta(t *testing.T) {
accessor.SetSelfLink("google.com")
accessor.SetLabels(map[string]string{"other": "label"})
accessor.SetAnnotations(map[string]string{"c": "d"})
accessor.SetFinalizers([]string{"finalizer.3"})

// Prove that accessor changes the original object.
if e, a := "baz", j.Namespace; e != a {
Expand Down Expand Up @@ -539,6 +569,9 @@ func TestGenericObjectMeta(t *testing.T) {
if e, a := map[string]string{"c": "d"}, j.Annotations; !reflect.DeepEqual(e, a) {
t.Errorf("expected %#v, got %#v", e, a)
}
if e, a := []string{"finalizer.3"}, j.Finalizers; !reflect.DeepEqual(e, a) {
t.Errorf("expected %v, got %v", e, a)
}
}

func TestGenericListMeta(t *testing.T) {
Expand Down
1 change: 1 addition & 0 deletions pkg/api/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2084,6 +2084,7 @@ type FinalizerName string
// These are internal finalizer values to Kubernetes, must be qualified name unless defined here
const (
FinalizerKubernetes FinalizerName = "kubernetes"
FinalizerOrphan string = "orphan"
)

// NamespaceStatus is information about the current status of a Namespace.
Expand Down
5 changes: 3 additions & 2 deletions pkg/api/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,9 @@ func ValidateObjectMeta(meta *api.ObjectMeta, requiresNamespace bool, nameFn Val
allErrs = append(allErrs, unversionedvalidation.ValidateLabels(meta.Labels, fldPath.Child("labels"))...)
allErrs = append(allErrs, ValidateAnnotations(meta.Annotations, fldPath.Child("annotations"))...)
allErrs = append(allErrs, ValidateOwnerReferences(meta.OwnerReferences, fldPath.Child("ownerReferences"))...)

for _, finalizer := range meta.Finalizers {
allErrs = append(allErrs, validateFinalizerName(finalizer, fldPath.Child("finalizers"))...)
}
return allErrs
}

Expand Down Expand Up @@ -373,7 +375,6 @@ func ValidateObjectMetaUpdate(newMeta, oldMeta *api.ObjectMeta, fldPath *field.P
allErrs = append(allErrs, ValidateImmutableField(newMeta.Namespace, oldMeta.Namespace, fldPath.Child("namespace"))...)
allErrs = append(allErrs, ValidateImmutableField(newMeta.UID, oldMeta.UID, fldPath.Child("uid"))...)
Copy link
Member

Choose a reason for hiding this comment

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

I thought you are only able to remove items from a finalizer list post creation. If this is handled elsewhere, ignore but noting as I review.

Copy link
Member

Choose a reason for hiding this comment

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

Post "deletion" they are remove-only.

On Wed, May 18, 2016 at 4:22 PM, Derek Carr notifications@github.com
wrote:

In pkg/api/validation/validation.go
#25599 (comment)
:

@@ -393,7 +393,6 @@ func ValidateObjectMetaUpdate(newMeta, oldMeta *api.ObjectMeta, fldPath *field.P
allErrs = append(allErrs, ValidateImmutableField(newMeta.Namespace, oldMeta.Namespace, fldPath.Child("namespace"))...)
allErrs = append(allErrs, ValidateImmutableField(newMeta.UID, oldMeta.UID, fldPath.Child("uid"))...)
allErrs = append(allErrs, ValidateImmutableField(newMeta.CreationTimestamp, oldMeta.CreationTimestamp, fldPath.Child("creationTimestamp"))...)

  • allErrs = append(allErrs, ValidateImmutableField(newMeta.Finalizers, oldMeta.Finalizers, fldPath.Child("finalizers"))...)

I thought you are only able to remove items from a finalizer list post
creation. If this is handled elsewhere, ignore but noting as I review.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25599/files/7b7917ddcb2f2ab0d8c22748df3668346ea69505#r63800258

Copy link
Member Author

Choose a reason for hiding this comment

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

@lavalamp's comment is right. Do you have any concerns?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, I was confused when making my original comment. no issue.

Copy link
Member

@liggitt liggitt May 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

they still should only be removable via a finalize subresource, right? what is preventing them from being removed via regular update operations?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think your new Update admission code can tell if the finalizers are modified and do the correct admission control?

allErrs = append(allErrs, ValidateImmutableField(newMeta.CreationTimestamp, oldMeta.CreationTimestamp, fldPath.Child("creationTimestamp"))...)
allErrs = append(allErrs, ValidateImmutableField(newMeta.Finalizers, oldMeta.Finalizers, fldPath.Child("finalizers"))...)

allErrs = append(allErrs, unversionedvalidation.ValidateLabels(newMeta.Labels, fldPath.Child("labels"))...)
allErrs = append(allErrs, ValidateAnnotations(newMeta.Annotations, fldPath.Child("annotations"))...)
Expand Down
15 changes: 0 additions & 15 deletions pkg/api/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,21 +267,6 @@ func TestValidateObjectMetaTrimsTrailingSlash(t *testing.T) {
}
}

// Ensure updating finalizers is disallowed
func TestValidateObjectMetaUpdateDisallowsUpdatingFinalizers(t *testing.T) {
errs := ValidateObjectMetaUpdate(
&api.ObjectMeta{Name: "test", ResourceVersion: "1", Finalizers: []string{"orphaning"}},
&api.ObjectMeta{Name: "test", ResourceVersion: "1"},
field.NewPath("field"),
)
if len(errs) != 1 {
t.Fatalf("unexpected errors: %v", errs)
}
if !strings.Contains(errs[0].Error(), "field is immutable") {
t.Errorf("unexpected error message: %v", errs)
}
}

func TestValidateAnnotations(t *testing.T) {
successCases := []map[string]string{
{"simple": "bar"},
Expand Down
2 changes: 1 addition & 1 deletion pkg/client/cache/delta_fifo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func NewDeltaFIFO(keyFunc KeyFunc, compressor DeltaCompressor, knownObjects KeyL
// A note on the KeyLister used by the DeltaFIFO: It's main purpose is
// to list keys that are "known", for the purpose of figuring out which
// items have been deleted when Replace() or Delete() are called. The deleted
// objet will be included in the DeleteFinalStateUnknown markers. These objects
// object will be included in the DeleteFinalStateUnknown markers. These objects
// could be stale.
//
// You may provide a function to compress deltas (e.g., represent a
Expand Down
Loading