Add serviceUID/configUID labels to pods & deployments, serviceUID label to configs #10539

duglin · 2021-01-13T15:36:28Z

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 [email protected]

vagababov · 2021-01-13T19:31:19Z

Why would there be pods from previous incarnation? Or this is about race condition while old one is still being deleted and new ones already online?
This seems to be a problem in a different fashion as well, where we create lots of resources with the same name, that probably won't be created (e.g. deployment) and deployment will probably own those pods too due to service label. No?

duglin · 2021-01-13T20:19:49Z

Yes exactly. If the old pods are in Terminating state a query for all pods related to a ksvc will return both sets of pods because all we can query on (in a single query) is the ksvc name (or config name). By adding the serviceUID we can ensure we get just the pods for the correct ksvc.

duglin · 2021-01-13T20:27:32Z

/retest

vagababov · 2021-01-13T20:40:24Z

pkg/reconciler/configuration/resources/revision_test.go

@@ -78,6 +78,7 @@ func TestMakeRevisions(t *testing.T) {
 					serving.ConfigurationLabelKey:           "build",


you don't seem to have any positive tests here.

yep - on my todo list - wanted to get the core of this out there for review ASAP. Coming soon...

duglin · 2021-01-15T19:10:41Z

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: ""}; !cmp.Equal(got, want) {


pkg/apis/serving/register.go

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 <[email protected]>

codecov · 2021-01-15T19:19:38Z

Codecov Report

Merging #10539 (57232dc) into master (80321c4) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #10539      +/-   ##
==========================================
- Coverage   88.09%   88.08%   -0.02%     
==========================================
  Files         186      186              
  Lines        8831     8839       +8     
==========================================
+ Hits         7780     7786       +6     
- Misses        810      811       +1     
- Partials      241      242       +1

Impacted Files	Coverage Δ
pkg/apis/serving/v1/revision_validation.go	`88.42% <ø> (ø)`
pkg/apis/serving/v1/configuration_validation.go	`100.00% <100.00%> (ø)`
pkg/reconciler/configuration/resources/revision.go	`88.63% <100.00%> (+1.79%)`	⬆️
pkg/reconciler/service/resources/configuration.go	`100.00% <100.00%> (ø)`
pkg/reconciler/autoscaling/kpa/scaler.go	`88.57% <0.00%> (-1.43%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80321c4...ba4938a. Read the comment docs.

vagababov

/lgtm

dprotaso · 2021-01-15T20:11:26Z

/approve

/hold can you update the title to reflect what's happening now - ie. service & config uid added to revision/deployment

knative-prow-robot · 2021-01-15T20:11:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, duglin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [dprotaso]
~~pkg/reconciler/OWNERS~~ [dprotaso]
~~pkg/testing/OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

duglin · 2021-01-15T21:37:33Z

@dprotaso done!

dprotaso · 2021-01-15T21:43:46Z

/hold cancel

duglin · 2021-01-15T23:36:55Z

/retest

While exporting the service using `service export` command, ignore the service labels with keys 'configUID' and `configUID` see knative/serving#10539 for further details.

…#1194) * Ignore serviceUID and configUID labels in service export result While exporting the service using `service export` command, ignore the service labels with keys 'configUID' and `configUID` see knative/serving#10539 for further details. * Fix the var names per golint recommendation * Ignore the serviceUID and configUID labels from revision labels * Add e2e for verifying configUID and serviceUID labels are absent * Add CHANGELOG

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 13, 2021

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/API API objects and controllers labels Jan 13, 2021

knative-prow-robot requested review from dprotaso and markusthoemmes January 13, 2021 15:36

duglin force-pushed the addServiceUID branch from 8f45cc0 to 59e91be Compare January 13, 2021 16:51

vagababov reviewed Jan 13, 2021

View reviewed changes

dprotaso requested changes Jan 14, 2021

View reviewed changes

duglin force-pushed the addServiceUID branch from 59e91be to a853c21 Compare January 15, 2021 19:10

duglin force-pushed the addServiceUID branch from a853c21 to ba4938a Compare January 15, 2021 19:14

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2021

duglin requested a review from dprotaso January 15, 2021 19:39

vagababov reviewed Jan 15, 2021

View reviewed changes

knative-prow-robot assigned vagababov Jan 15, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2021

duglin changed the title ~~Add a serviceUID label to ksvc (and implicitly to config)~~ Add serviceUID/configUID labels to pods & deployments, serviceUID label to configs Jan 15, 2021

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2021

knative-prow-robot merged commit 3177cc7 into knative:master Jan 16, 2021

navidshaikh mentioned this pull request Jan 19, 2021

Do not print serviceUID and configUID labels in service export result knative/client#1194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add serviceUID/configUID labels to pods & deployments, serviceUID label to configs #10539

Add serviceUID/configUID labels to pods & deployments, serviceUID label to configs #10539

duglin commented Jan 13, 2021

vagababov commented Jan 13, 2021

duglin commented Jan 13, 2021

duglin commented Jan 13, 2021

vagababov Jan 13, 2021

duglin Jan 14, 2021

duglin Jan 15, 2021

This comment was marked as resolved.

duglin Jan 15, 2021

codecov bot commented Jan 15, 2021

vagababov left a comment

dprotaso commented Jan 15, 2021

knative-prow-robot commented Jan 15, 2021

duglin commented Jan 15, 2021

dprotaso commented Jan 15, 2021

duglin commented Jan 15, 2021

		@@ -78,6 +78,7 @@ func TestMakeRevisions(t *testing.T) {
		serving.ConfigurationLabelKey: "build",

Add serviceUID/configUID labels to pods & deployments, serviceUID label to configs #10539

Add serviceUID/configUID labels to pods & deployments, serviceUID label to configs #10539

Conversation

duglin commented Jan 13, 2021

vagababov commented Jan 13, 2021

duglin commented Jan 13, 2021

duglin commented Jan 13, 2021

vagababov Jan 13, 2021

Choose a reason for hiding this comment

duglin Jan 14, 2021

Choose a reason for hiding this comment

duglin Jan 15, 2021

Choose a reason for hiding this comment

This comment was marked as resolved.

duglin Jan 15, 2021

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2021

Codecov Report

vagababov left a comment

Choose a reason for hiding this comment

dprotaso commented Jan 15, 2021

knative-prow-robot commented Jan 15, 2021

duglin commented Jan 15, 2021

dprotaso commented Jan 15, 2021

duglin commented Jan 15, 2021