Consistent labels for channel and provisioner by sbezverk · Pull Request #698 · knative/eventing

sbezverk · 2019-01-03T23:52:21Z

Signed-off-by: Serguei Bezverkhi sbezverk@cisco.com

Fixes #690

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

sbezverk · 2019-01-03T23:53:04Z

/assign @vaikas-google

sbezverk · 2019-01-03T23:57:50Z

@vaikas-google @grantr I do not like this solution as labels are not defined in one place. Please advise if I could create a common package where I could define these labels and then make a func which would return them to any func where it is needed.

grantr · 2019-01-04T00:00:20Z

Thanks @sbezverk! Seems like the label keys could be constants in the provisioners package.

sbezverk · 2019-01-04T00:04:53Z

@grantr ok make sense and it looks like provisioners package gets imported in some tests.

Harwayne · 2019-01-04T00:10:54Z

We have to be careful with how we do this, it will cause duplicate VirtualServices. See #674 (comment).

We can make this backwards compatible. In this release, search based on the old labels and have the controller add the both the new and old labels to all Services and VirtualServices. Next release, only recognize the new labels (and stop writing them to existing Services and VirtualServices). This means that anyone upgrading 0.2 -> 0.3 -> 0.4 works. 0.2 -> 0.4 would have the same possible problems discussed above.

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

sbezverk · 2019-01-04T00:22:52Z

@Harwayne thank you for your comment. Could you please provide steps to reproduce messages you posted in #674. I would like to see how it could be solved. Unit testing does not catch this condition.

vaikas · 2019-01-04T00:28:46Z

@sbezverk I think @Harwayne is talking about an upgrade path. If somebody upgrades the eventing components and the new labels get applied.

sbezverk · 2019-01-04T00:40:14Z

@vaikas-google @Harwayne what about preserving old labels and add new set of labels and then at one point deprecate old labels?

grantr · 2019-01-04T01:02:50Z

Yep that seems like the right approach. For N releases we'll have both label styles applied.

vaikas · 2019-01-04T01:11:50Z

That might work but I defer to @Harwayne because I thought there were some corner cases here.

Harwayne · 2019-01-04T01:51:47Z

@vaikas-google @Harwayne what about preserving old labels and add new set of labels and then at one point deprecate old labels?

Yes, that is the general idea. I think the procedure is:

Change CreateVirtualService to compare labels, in addition to the current comparing spec. If the labels differ, update the VirtualService.
Change CreateK8sService to act like CreateVirtualService, in that it compares the existing to the desired and updates existing to match desired.
Add the new labels to all created Services and VirtualServices. But, only search for them based on the old labels (i.e. getK8sService and getVirtualService use a label selector that only uses the old labels).
- The combination of this and the previous two steps means that existing Services and VirtualServices will get the updated labels added.
Wait a full release.
No longer search for or create using the old labels.

sbezverk · 2019-01-04T14:30:15Z

@Harwayne WRT point 1 of your comment. Based on the code and updated newVirtualService, see: https://github.com/knative/eventing/blob/master/pkg/provisioners/channel_util.go#L161

new newVirtualService will return VirtualService with old and new labels, if there is already VirtualService with just old labels, the equality check will fail and the existing VirtualService will be updated with new labels. It seems nothing else is required. Please confirm or Am I missing something?

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne · 2019-01-04T15:52:12Z

@Harwayne WRT point 1 of your comment. Based on the code and updated newVirtualService, see: https://github.com/knative/eventing/blob/master/pkg/provisioners/channel_util.go#L161

new newVirtualService will return VirtualService with old and new labels, if there is already VirtualService with just old labels, the equality check will fail and the existing VirtualService will be updated with new labels. It seems nothing else is required. Please confirm or Am I missing something?

You are correct that newVirtualService now creates with both old and new labels. So point 3 is finished. However, the equality check is only on spec, so if the only change is labels, it will get ignored. I think it needs to be changed to something like:

	if !equality.Semantic.DeepDerivative(expected.Spec, virtualService.Spec) || !equality.Semantic.DeepEqual(expected.Meta.Labels, virtualService.Meta.Labels) {
		virtualService.Spec = expected.Spec
		virtualService.Meta.Labels = expected.Meta.Labels
		err := client.Update(ctx, virtualService)
		if err != nil {
			return nil, err
		}
	}

That would fix point 1.

Then we would still need to make CreateK8sService look like CreateVirtualService (point 2).

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne

I think copying over non-interfering labels is a very good idea.

And I was wrong about point 2 above, it has already been done.

Harwayne · 2019-01-04T17:05:34Z


+// checkExpectedLabels checks the presence of expected labels and its values and return true
+// if all labels are found.
+func checkExpectedLabels(actual, expected map[string]string) bool {


I believe this is equivalent to equality.Semantic.DeepDerivative(expected, actual).

@Harwayne I get your point, I was just trying to be more specific in what I check and can control. I think usage DeepDerivative in this specific case is too generic, but I do not have a strong opinion about it.

I have a slight preference towards using DeepDerivative, but it is weak considering how simple this function is. But, if we are going to keep it, please add unit tests for it.

Harwayne · 2019-01-04T17:07:16Z

+	}
+	// Second add all missing expected labels
+	for k, v := range expected {
+		if va, ok := consolidated[k]; ok {


Shouldn't this always overwrite? If the value of the label is different than expected, don't we want to overwrite that value to be what we expect?

yep, you are right, thanks for catching it.

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

vaikas · 2019-01-04T18:05:41Z

/approve

knative-prow-robot · 2019-01-04T18:05:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbezverk, vaikas-google

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vaikas-google]

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

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne · 2019-01-04T18:27:59Z

+				OldEventingProvisionerLabel: "provisioner-1",
+			},
+			shouldFail: false,
+		},


Add a test where actual has extraneous extra labels, to ensure we only return true when the expected ones are there.

{ name: "all good", actual: map[string]string{ EventingChannelLabel: "channel-1", OldEventingChannelLabel: "channel-1", EventingProvisionerLabel: "provisioner-1", OldEventingProvisionerLabel: "provisioner-1", "extra-label-that-should-be-ignored": "foo", }, expected: map[string]string{ EventingChannelLabel: "channel-1", OldEventingChannelLabel: "channel-1", EventingProvisionerLabel: "provisioner-1", OldEventingProvisionerLabel: "provisioner-1", }, shouldFail: false, },

Harwayne · 2019-01-04T18:29:33Z


+// checkExpectedLabels checks the presence of expected labels and its values and return true
+// if all labels are found.
+func checkExpectedLabels(actual, expected map[string]string) bool {


Can we change the name to expectedLabelsPresent?

sure thing.

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

knative-metrics-robot · 2019-01-04T18:37:24Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/provisioners/channel_util.go	94.0%	94.9%	1.0

Harwayne · 2019-01-04T18:41:47Z

/lgtm

grantr · 2019-01-04T21:49:58Z

/milestone v0.3

knative-prow-robot · 2019-01-04T21:49:59Z

@grantr: The provided milestone is not valid for this repository. Milestones in this repository: [v0.2.2, v0.3.0]

Use /milestone clear to clear the milestone.

Details

In response to this:

/milestone v0.3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

grantr · 2019-01-04T21:50:09Z

/milestone v0.3.0

Consistent labels

33b05f7

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 3, 2019

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 3, 2019

knative-prow-robot requested review from Harwayne and grantr January 3, 2019 23:52

knative-prow-robot assigned vaikas Jan 3, 2019

Define labels in one place

3d0eb62

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne mentioned this pull request Jan 4, 2019

Consistent names of our labels. #690

Closed

Addressing review comments

92bd4d2

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 4, 2019

Add some helper functions for labels management

4650fcb

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 4, 2019

Harwayne reviewed Jan 4, 2019

View reviewed changes

Addressing comments #2

800111d

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

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

Adding unit test

64404d1

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne reviewed Jan 4, 2019

View reviewed changes

Adding extra unit test and change func name

ad21cba

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

knative-prow-robot assigned Harwayne Jan 4, 2019

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

knative-prow-robot merged commit 44f4bc3 into knative:master Jan 4, 2019

sbezverk deleted the labels branch January 4, 2019 19:02

knative-prow-robot added this to the v0.3.0 milestone Jan 4, 2019

Harwayne mentioned this pull request Feb 7, 2019

Add Harwayne as an approver. #782

Merged

Conversation

sbezverk commented Jan 3, 2019

Uh oh!

sbezverk commented Jan 3, 2019

Uh oh!

sbezverk commented Jan 3, 2019

Uh oh!

grantr commented Jan 4, 2019

Uh oh!

sbezverk commented Jan 4, 2019

Uh oh!

Harwayne commented Jan 4, 2019

Uh oh!

sbezverk commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vaikas commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbezverk commented Jan 4, 2019

Uh oh!

grantr commented Jan 4, 2019

Uh oh!

vaikas commented Jan 4, 2019

Uh oh!

Harwayne commented Jan 4, 2019

Uh oh!

sbezverk commented Jan 4, 2019

Uh oh!

Harwayne commented Jan 4, 2019

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaikas commented Jan 4, 2019

Uh oh!

knative-prow-robot commented Jan 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Jan 4, 2019

Uh oh!

Harwayne commented Jan 4, 2019

Uh oh!

grantr commented Jan 4, 2019

Uh oh!

knative-prow-robot commented Jan 4, 2019

Uh oh!

grantr commented Jan 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sbezverk commented Jan 4, 2019 •

edited

Loading

vaikas commented Jan 4, 2019 •

edited

Loading