From 3177cc7a208e33c07872b8ec6c70cb8edcb8ccbf Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Fri, 15 Jan 2021 21:25:19 -0500 Subject: [PATCH] Add a serviceUID label to ksvc (and implicitly to config) (#10539) This will allow people to find all pods for a service w/o running into the problem of finding pods associated with an old ksvc that just happens to share the same name. With this the user can search for pods by the ksvc UID via the new label. Rather than having the code go and lookup the ksvc from the revision, I decided to just add this label to the config as well. If people would prefer to not have this extra label on the config (and do it via a ksvc lookup) I can look into that change. Signed-off-by: Doug Davis --- pkg/apis/serving/register.go | 8 ++++ .../serving/v1/configuration_validation.go | 6 ++- pkg/apis/serving/v1/revision_validation.go | 2 + .../configuration/resources/revision.go | 6 +++ .../configuration/resources/revision_test.go | 45 ++++++++++++++++--- .../service/resources/configuration.go | 5 ++- .../service/resources/configuration_test.go | 2 +- .../service/resources/shared_test.go | 2 +- pkg/testing/v1/service.go | 1 + 9 files changed, 67 insertions(+), 10 deletions(-) diff --git a/pkg/apis/serving/register.go b/pkg/apis/serving/register.go index 66c01b5d4389..b2dd7d867b57 100644 --- a/pkg/apis/serving/register.go +++ b/pkg/apis/serving/register.go @@ -77,6 +77,14 @@ const ( // its unique identifier RevisionUID = GroupName + "/revisionUID" + // ConfigUIDLabelKey is the label key attached to a pod to reference its + // Knative Configuration by its unique UID + ConfigUIDLabelKey = GroupName + "/configUID" + + // ServiceUIDLabelKey is the label key attached to a pod to reference its + // Knative Service by its unique UID + ServiceUIDLabelKey = GroupName + "/serviceUID" + // ServiceLabelKey is the label key attached to a Route and Configuration indicating by // which Service they are created. ServiceLabelKey = GroupName + "/service" diff --git a/pkg/apis/serving/v1/configuration_validation.go b/pkg/apis/serving/v1/configuration_validation.go index f02dd2b94de8..b098f14817bb 100644 --- a/pkg/apis/serving/v1/configuration_validation.go +++ b/pkg/apis/serving/v1/configuration_validation.go @@ -65,8 +65,10 @@ func (cs *ConfigurationSpec) Validate(ctx context.Context) *apis.FieldError { func (c *Configuration) validateLabels() (errs *apis.FieldError) { for key, val := range c.GetLabels() { switch key { - case serving.RouteLabelKey: - // Known valid labels. + case serving.RouteLabelKey, + serving.ConfigUIDLabelKey, + serving.ServiceUIDLabelKey: + // Known valid labels - so just skip them case serving.ServiceLabelKey: errs = errs.Also(verifyLabelOwnerRef(val, serving.ServiceLabelKey, "Service", c.GetOwnerReferences())) default: diff --git a/pkg/apis/serving/v1/revision_validation.go b/pkg/apis/serving/v1/revision_validation.go index 4bdf6bca4405..bbe8ed25a927 100644 --- a/pkg/apis/serving/v1/revision_validation.go +++ b/pkg/apis/serving/v1/revision_validation.go @@ -128,6 +128,8 @@ func (r *Revision) ValidateLabels() (errs *apis.FieldError) { case serving.RoutingStateLabelKey, serving.RouteLabelKey, serving.ServiceLabelKey, + serving.ConfigUIDLabelKey, + serving.ServiceUIDLabelKey, serving.ConfigurationGenerationLabelKey: // Known valid labels. case serving.ConfigurationLabelKey: diff --git a/pkg/reconciler/configuration/resources/revision.go b/pkg/reconciler/configuration/resources/revision.go index 9fb516dcd627..276dda6f04ea 100644 --- a/pkg/reconciler/configuration/resources/revision.go +++ b/pkg/reconciler/configuration/resources/revision.go @@ -64,6 +64,8 @@ func updateRevisionLabels(rev, config metav1.Object) { serving.ConfigurationLabelKey, serving.ServiceLabelKey, serving.ConfigurationGenerationLabelKey, + serving.ConfigUIDLabelKey, + serving.ServiceUIDLabelKey, } { labels[key] = RevisionLabelValueForKey(key, config) } @@ -101,6 +103,10 @@ func RevisionLabelValueForKey(key string, config metav1.Object) string { return config.GetLabels()[serving.ServiceLabelKey] case serving.ConfigurationGenerationLabelKey: return fmt.Sprint(config.GetGeneration()) + case serving.ConfigUIDLabelKey: + return string(config.GetUID()) + case serving.ServiceUIDLabelKey: + return config.GetLabels()[serving.ServiceUIDLabelKey] } return "" } diff --git a/pkg/reconciler/configuration/resources/revision_test.go b/pkg/reconciler/configuration/resources/revision_test.go index aa1ab1179668..47afdac35cc9 100644 --- a/pkg/reconciler/configuration/resources/revision_test.go +++ b/pkg/reconciler/configuration/resources/revision_test.go @@ -49,6 +49,10 @@ func TestMakeRevisions(t *testing.T) { Namespace: "no", Name: "build", Generation: 10, + Labels: map[string]string{ + serving.ServiceUIDLabelKey: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", + }, + UID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", }, Spec: v1.ConfigurationSpec{ Template: v1.RevisionTemplateSpec{ @@ -72,12 +76,15 @@ func TestMakeRevisions(t *testing.T) { Name: "build", Controller: ptr.Bool(true), BlockOwnerDeletion: ptr.Bool(true), + UID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", }}, Labels: map[string]string{ - serving.RoutingStateLabelKey: "pending", serving.ConfigurationLabelKey: "build", serving.ConfigurationGenerationLabelKey: "10", + serving.ConfigUIDLabelKey: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + serving.RoutingStateLabelKey: "pending", serving.ServiceLabelKey: "", + serving.ServiceUIDLabelKey: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", }, Annotations: map[string]string{ serving.RoutingStateModifiedAnnotationKey: v1.RoutingStateModifiedString(clock), @@ -98,6 +105,10 @@ func TestMakeRevisions(t *testing.T) { Namespace: "with", Name: "labels", Generation: 100, + Labels: map[string]string{ + serving.ServiceUIDLabelKey: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", + }, + UID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", }, Spec: v1.ConfigurationSpec{ Template: v1.RevisionTemplateSpec{ @@ -130,12 +141,15 @@ func TestMakeRevisions(t *testing.T) { Name: "labels", Controller: ptr.Bool(true), BlockOwnerDeletion: ptr.Bool(true), + UID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", }}, Labels: map[string]string{ serving.ConfigurationLabelKey: "labels", serving.ConfigurationGenerationLabelKey: "100", - serving.ServiceLabelKey: "", + serving.ConfigUIDLabelKey: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", serving.RoutingStateLabelKey: "pending", + serving.ServiceLabelKey: "", + serving.ServiceUIDLabelKey: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", "foo": "bar", "baz": "blah", }, @@ -155,6 +169,10 @@ func TestMakeRevisions(t *testing.T) { Namespace: "with", Name: "annotations", Generation: 100, + Labels: map[string]string{ + serving.ServiceUIDLabelKey: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", + }, + UID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", }, Spec: v1.ConfigurationSpec{ Template: v1.RevisionTemplateSpec{ @@ -184,12 +202,15 @@ func TestMakeRevisions(t *testing.T) { Name: "annotations", Controller: ptr.Bool(true), BlockOwnerDeletion: ptr.Bool(true), + UID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", }}, Labels: map[string]string{ serving.ConfigurationLabelKey: "annotations", serving.ConfigurationGenerationLabelKey: "100", - serving.ServiceLabelKey: "", + serving.ConfigUIDLabelKey: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", serving.RoutingStateLabelKey: "pending", + serving.ServiceLabelKey: "", + serving.ServiceUIDLabelKey: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", }, Annotations: map[string]string{ "foo": "bar", @@ -217,7 +238,11 @@ func TestMakeRevisions(t *testing.T) { "serving.knative.dev/lastModifier": "someone", serving.RoutesAnnotationKey: "route", }, + Labels: map[string]string{ + serving.ServiceUIDLabelKey: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", + }, Generation: 10, + UID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", }, Spec: v1.ConfigurationSpec{ Template: v1.RevisionTemplateSpec{ @@ -246,12 +271,15 @@ func TestMakeRevisions(t *testing.T) { Name: "config", Controller: ptr.Bool(true), BlockOwnerDeletion: ptr.Bool(true), + UID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", }}, Labels: map[string]string{ serving.ConfigurationLabelKey: "config", serving.ConfigurationGenerationLabelKey: "10", - serving.ServiceLabelKey: "", + serving.ConfigUIDLabelKey: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", serving.RoutingStateLabelKey: "active", + serving.ServiceLabelKey: "", + serving.ServiceUIDLabelKey: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", }, }, Spec: v1.RevisionSpec{ @@ -272,7 +300,11 @@ func TestMakeRevisions(t *testing.T) { "serving.knative.dev/creator": "admin", "serving.knative.dev/lastModifier": "someone", }, + Labels: map[string]string{ + serving.ServiceUIDLabelKey: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", + }, Generation: 10, + UID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", }, Spec: v1.ConfigurationSpec{ Template: v1.RevisionTemplateSpec{ @@ -308,12 +340,15 @@ func TestMakeRevisions(t *testing.T) { Name: "config", Controller: ptr.Bool(true), BlockOwnerDeletion: ptr.Bool(true), + UID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", }}, Labels: map[string]string{ serving.ConfigurationLabelKey: "config", serving.ConfigurationGenerationLabelKey: "10", - serving.ServiceLabelKey: "", + serving.ConfigUIDLabelKey: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", serving.RoutingStateLabelKey: "pending", + serving.ServiceLabelKey: "", + serving.ServiceUIDLabelKey: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", }, }, Spec: v1.RevisionSpec{ diff --git a/pkg/reconciler/service/resources/configuration.go b/pkg/reconciler/service/resources/configuration.go index ba9d8864f750..133e326b197f 100644 --- a/pkg/reconciler/service/resources/configuration.go +++ b/pkg/reconciler/service/resources/configuration.go @@ -36,7 +36,10 @@ func MakeConfiguration(service *v1.Service) *v1.Configuration { // MakeConfigurationFromExisting creates a Configuration from a Service object given an existing Configuration. func MakeConfigurationFromExisting(service *v1.Service, existing *v1.Configuration) *v1.Configuration { - labels := map[string]string{serving.ServiceLabelKey: service.Name} + labels := map[string]string{ + serving.ServiceLabelKey: service.Name, + serving.ServiceUIDLabelKey: string(service.ObjectMeta.UID), + } anns := kmeta.FilterMap(service.GetAnnotations(), func(key string) bool { return key == corev1.LastAppliedConfigAnnotation }) diff --git a/pkg/reconciler/service/resources/configuration_test.go b/pkg/reconciler/service/resources/configuration_test.go index 52634a8c89c0..cf715d9449b7 100644 --- a/pkg/reconciler/service/resources/configuration_test.go +++ b/pkg/reconciler/service/resources/configuration_test.go @@ -41,7 +41,7 @@ func TestConfigurationSpec(t *testing.T) { } expectOwnerReferencesSetCorrectly(t, c.OwnerReferences) - if got, want := c.Labels, map[string]string{serving.ServiceLabelKey: testServiceName}; !cmp.Equal(got, want) { + if got, want := c.Labels, map[string]string{serving.ServiceLabelKey: testServiceName, serving.ServiceUIDLabelKey: "cccccccc-cccc-cccc-cccc-cccccccccccc"}; !cmp.Equal(got, want) { t.Errorf("Labels mismatch: diff(-want,+got):\n%s", cmp.Diff(want, got)) } if got, want := c.Annotations, map[string]string{ diff --git a/pkg/reconciler/service/resources/shared_test.go b/pkg/reconciler/service/resources/shared_test.go index c2e92909598b..0c31c914873f 100644 --- a/pkg/reconciler/service/resources/shared_test.go +++ b/pkg/reconciler/service/resources/shared_test.go @@ -49,7 +49,7 @@ func expectOwnerReferencesSetCorrectly(t *testing.T, ownerRefs []metav1.OwnerRef Kind: "Service", Name: testServiceName, }} - if diff := cmp.Diff(expectedRefs, ownerRefs, cmpopts.IgnoreFields(expectedRefs[0], "Controller", "BlockOwnerDeletion")); diff != "" { + if diff := cmp.Diff(expectedRefs, ownerRefs, cmpopts.IgnoreFields(expectedRefs[0], "Controller", "BlockOwnerDeletion", "UID")); diff != "" { t.Error("Unexpected service owner refs diff (-want +got):", diff) } } diff --git a/pkg/testing/v1/service.go b/pkg/testing/v1/service.go index 734d122f78ef..59581028dde3 100644 --- a/pkg/testing/v1/service.go +++ b/pkg/testing/v1/service.go @@ -42,6 +42,7 @@ func Service(name, namespace string, so ...ServiceOption) *v1.Service { ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, + UID: "cccccccc-cccc-cccc-cccc-cccccccccccc", }, } for _, opt := range so {