Convert EndpointSlice Reconciler to a library in staging. #118953

mskrocki · 2023-06-28T23:13:55Z

What type of PR is this?

/kind feature

Does this PR introduce a user-facing change?

New staging repo has been created for the EndpointSlice reconciler.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/pull/3686/commits/f0e118c01e95b335a2ed8101f0b6a935232d3b8f

k8s-ci-robot · 2023-06-28T23:14:04Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

staging/src/k8s.io/endpointslice/utils.go

akhilles · 2023-06-28T23:53:15Z

staging/src/k8s.io/endpointslice/util/endpoint/controller_utils.go

@@ -85,7 +85,7 @@ func NewPortMapKey(endpointPorts []discovery.EndpointPort) PortMapKey {
 // DeepHashObjectToString creates a unique hash string from a go object.
 func DeepHashObjectToString(objectToWrite interface{}) string {


The only other consumer of this is util/endpointslice. I wonder if it makes sense to combine k8s.io/endpointslice/util/endpoint and k8s.io/endpointslice/util/endpointslice into k8s.io/endpointslice/util to reduce the public API surface.

Maybe even flatten k8s.io/endpointslice/util into k8s.io/endpointslice? Looking at some of the functions like ShouldPodBeInEndpoints, I think they belong in the main pkg. See https://www.kubernetes.dev/docs/guide/coding-convention/#directory-and-file-conventions:

Avoid general utility packages. Packages called “util” are suspect.

yeah, it does not make have much sense have so many utils folders, in theory k8s.io/endpointslice/util/endpoint should only be used for the endpoints controller, but I'm not surprised we ended mixing things

I think we have to limit ourselves to k8s.io/endpointslice/util, since k8s.io/endpointslice/topologycache and k8s.io/endpointslice/metrics import utils as well, and if not mistaken, we will end up with cyclic imports

It looks like k8s.io/endpointslice/topologycache no longer depends on k8s.io/endpointslice/util, and only k8s.io/endpointslice/metrics does. But now I'm wondering whether metrics should be flattened into k8s.io/endpointslice as well ..

Is there any value in having metrics be a separate module? It's only really used by the reconciler and doesn't seem like it can be reused anywhere else. This would let us reduce the API surface substantially.

There are a bunch of /util functions that are only used within k8s.io/endpointslice (incl. metrics and topologycache) but they're part of the public API. Instead of flattening, maybe a better approach is to move those functions into /internal instead?

metrics is used in the controller e.g. https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/endpointslice/endpointslice_controller.go#L564

staging/src/k8s.io/endpointslice/util/endpointslice/utils.go

staging/src/k8s.io/endpointslice/util/endpoint/controller_utils.go

akhilles · 2023-06-29T00:24:31Z

staging/src/k8s.io/endpointslice/util/endpoint/controller_utils.go

+	printer.Fprintf(hasher, "%#v", objectToWrite)
+}
+
+func IsPodReady(pod *v1.Pod) bool {


If we flatten the relevant utils into k8s.io/endpointslice, then this can be made private.

staging/src/k8s.io/endpointslice/util/endpointslice/errors.go

robscott

Thanks @mskrocki! Agree with all of the comments from @akhilles, added a few tiny ones as well. One other note, I think this PR would benefit from a release note. I think it's worth highlighting that a new staging repo has been created for the EndpointSlice reconciler.

staging/src/k8s.io/endpointslice/util/endpoint/controller_utils.go

staging/src/k8s.io/endpointslice/OWNERS

robscott · 2023-06-29T01:01:32Z

pkg/controller/endpointslicemirroring/endpointslicemirroring_controller.go

@@ -38,10 +38,10 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/workqueue"
+	endpointsliceutil "k8s.io/endpointslice/util/endpointslice"


It's unfortunate that the EndpointSlice reconciler library is not sufficiently generic to be used by the EndpointSlice Mirroring controller. Although it's out of scope for this PR, it would probably be good to document somewhere, either in the EndpointSlice mirroring controller or in the new EndpointSlice staging lib why that is. I believe it comes down to the fact that the reconciler in the EndpointSlice mirroring controller has a 1:1 mapping between Service/Endpoints and EndpointSlice, which results in a simpler implementation that the EndpointSlice staging lib.

Now that the code is in a place that it could be shared, we probably want to at least describe why it's not currently shared, and state that contributions to move towards the shared code being used by the mirroring controller would be welcome.

not sure it is the right place, but added above to the README.

aojea · 2023-06-30T08:41:49Z

staging/src/k8s.io/endpointslice/util/controller_utils.go

@@ -62,7 +61,7 @@ func GetPodServiceMemberships(serviceLister v1listers.ServiceLister, pod *v1.Pod
 			// if the service has a nil selector this means selectors match nothing, not everything.
 			continue
 		}
-		key, err := controller.KeyFunc(service)
+		key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(service)


Interesting, I think that this is not needed at all, but I leave this just as a comment to not forget and revisit later https://github.com/kubernetes/kubernetes/pull/84280/files#r1247603063, no action required

aojea · 2023-06-30T08:48:57Z

staging/src/k8s.io/endpointslice/util/controller_utils.go

+}
+
+// IsPodReady returns true if Pods Ready condition is true
+// copied from k8s.io/kubernetes/pkg/api/v1/pod


@thockin @BenTheElder do you know some magic to keep these things in sync?
some linter maybe?

no, I don't have a good answer. Most of them are API based, and we won't intentionally break that, so it's another case "a little copy is better than a little dep", probably.

aojea · 2023-06-30T08:59:17Z

pull-kubernetes-verify
pull-publishing-bot-validate

jobs failures are legit, something with the rules and restrictions, I'm not very familiar with this to be able to pinpoint the problem

2023/06/30 02:48:09 Inspecting imports under ./vendor/k8s.io/endpointslice/...
2023/06/30 02:48:10 - validating imports for 4 packages in the tree
2023/06/30 02:48:10 -- found forbidden imports for k8s.io/kubernetes/vendor/k8s.io/endpointslice:
2023/06/30 02:48:10 --- k8s.io/endpointslice/metrics
2023/06/30 02:48:10 --- k8s.io/endpointslice/topologycache
2023/06/30 02:48:10 --- k8s.io/endpointslice/util
2023/06/30 02:48:10 -- found forbidden imports for k8s.io/kubernetes/vendor/k8s.io/endpointslice/metrics:
2023/06/30 02:48:10 --- k8s.io/endpointslice/util

I0630 02:05:23.138769    1629 rules.go:98] loading rules file : /home/prow/go/src/k8s.io/kubernetes/staging/publishing/rules.yaml
I0[6](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/118953/pull-publishing-bot-validate/1674599288581132288#1:build-log.txt%3A6)30 02:05:23.145222    1629 rules.go:134] validating repository order
I0630 02:05:23.145436    1629 rules.go:156] validating go versions
F0630 02:05:23.14560[7](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/118953/pull-publishing-bot-validate/1674599288581132288#1:build-log.txt%3A7)    1629 main.go:41] Invalid rules file "/home/prow/go/src/k[8](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/118953/pull-publishing-bot-validate/1674599288581132288#1:build-log.txt%3A8)s.io/kubernetes/staging/publishing/rules.yaml": validation errors:
- unknown dependency "klog" in repository rules of "endpointslice"
- unknown dependency "utils" in repository rules of "endpointslice"

/test pull-kubernetes-node-e2e-containerd

the rest looks pretty nice, right?

akhilles · 2023-07-06T04:51:25Z

staging/src/k8s.io/endpointslice/topologycache/utils.go

+
+// EndpointReady returns true if an Endpoint has the Ready condition set to
+// true.
+func EndpointReady(endpoint discovery.Endpoint) bool {


Can this be a private function? This shouldn't really be part of the public API.

it is used in the reconciler tests.

Can it be private and copied to the call sites? It's a fairly trivial function. EndpointReady really has nothing to do with topologycache.

pkg/controller/endpointslice/endpointslice_controller.go

akhilles · 2023-07-06T05:10:36Z

staging/src/k8s.io/endpointslice/util/endpoint/controller_utils.go

@@ -85,7 +85,7 @@ func NewPortMapKey(endpointPorts []discovery.EndpointPort) PortMapKey {
 // DeepHashObjectToString creates a unique hash string from a go object.
 func DeepHashObjectToString(objectToWrite interface{}) string {


It looks like k8s.io/endpointslice/topologycache no longer depends on k8s.io/endpointslice/util, and only k8s.io/endpointslice/metrics does. But now I'm wondering whether metrics should be flattened into k8s.io/endpointslice as well ..

Is there any value in having metrics be a separate module? It's only really used by the reconciler and doesn't seem like it can be reused anywhere else. This would let us reduce the API surface substantially.

pkg/controller/util/endpoint/OWNERS

akhilles · 2023-07-06T05:17:17Z

pkg/controller/endpointslice/endpointslice_controller.go

@@ -354,7 +355,7 @@ func (c *Controller) syncService(key string) error {

 	esLabelSelector := labels.Set(map[string]string{
 		discovery.LabelServiceName: service.Name,
-		discovery.LabelManagedBy:   controllerName,
+		discovery.LabelManagedBy:   c.reconciler.GetControllerName(),


Can use controllerName directly here. Would eliminate the need for the GetControllerName function.

I want to expose controllerName.

akhilles · 2023-07-06T05:19:02Z

staging/src/k8s.io/endpointslice/reconciler.go

+
+// ManagedByChanged returns true if one of the provided EndpointSlices is
+// managed by the EndpointSlice controller while the other is not.
+func (r *Reconciler) ManagedByChanged(endpointSlice1, endpointSlice2 *discovery.EndpointSlice) bool {


Can be moved to pkg/controller/endpointslice/endpointslice_controller.go and made private. It's only used there.

you want to have that function as library, to identify if the particular object is managed by this controller.

yeah, I can see how this can be useful for external consumers

akhilles · 2023-07-06T05:20:12Z

staging/src/k8s.io/endpointslice/reconciler.go

+
+// ManagedByController returns true if the controller of the provided
+// EndpointSlices is the EndpointSlice controller.
+func (r *Reconciler) ManagedByController(endpointSlice *discovery.EndpointSlice) bool {


Same as above. Can be moved to pkg/controller/endpointslice/endpointslice_controller.go and made private.

same comment as above, you want to have that function as library, to identify if the particular object is managed by this controller.

agree with Maciej

mskrocki · 2023-07-10T18:53:56Z

/test pull-kubernetes-integration

mskrocki · 2023-07-11T15:47:58Z

/assign @robscott

mskrocki · 2023-07-11T16:30:44Z

/test pull-kubernetes-unit

mskrocki · 2023-07-11T20:00:07Z

/test pull-kubernetes-unit

aojea · 2023-07-13T09:29:51Z

failure is legit

go: downloading gopkg.in/yaml.v2 v2.4.0
I0711 18:12:19.881974    1614 rules.go:98] loading rules file : /home/prow/go/src/k8s.io/kubernetes/staging/publishing/rules.yaml
I0711 18:12:19.888[5](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/118953/pull-publishing-bot-validate/1678828898579648512#1:build-log.txt%3A5)07    1[6](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/118953/pull-publishing-bot-validate/1678828898579648512#1:build-log.txt%3A6)14 rules.go:134] validating repository order
I0[7](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/118953/pull-publishing-bot-validate/1678828898579648512#1:build-log.txt%3A7)11 1[8](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/118953/pull-publishing-bot-validate/1678828898579648512#1:build-log.txt%3A8):12:19.888742    1614 rules.go:156] validating go versions
I0711 18:12:19.888919    1614 validate.go:37] validating directories exist in the kubernetes branch
E0711 18:12:20.540731    1614 main.go:46] Error : endpointslice not found in branch master
F0711 18:12:20.54132[9](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/118953/pull-publishing-bot-validate/1678828898579648512#1:build-log.txt%3A9)    1614 main.go:48] Invalid rules file "/home/prow/go/src/k8s.io/kubernetes/staging/publishing/rules.yaml"

aojea · 2023-07-13T11:25:38Z

/lgtm
/approve

k8s-ci-robot · 2023-07-13T11:25:45Z

LGTM label has been added.

Git tree hash: b97ff1a21421fe675f898cf34e46eaa9e6534853

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2023-07-13T21:50:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, mskrocki, thockin

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:

~~OWNERS~~ [thockin]
~~pkg/controller/endpoint/OWNERS~~ [aojea,thockin]
~~pkg/controller/endpointslice/OWNERS~~ [aojea,thockin]
~~pkg/controller/endpointslicemirroring/OWNERS~~ [aojea,thockin]
~~staging/OWNERS~~ [thockin]
~~vendor/OWNERS~~ [thockin]

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

k8s-triage-robot · 2023-07-14T00:08:16Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot · 2023-07-14T00:11:29Z

@mskrocki: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-publishing-bot-validate	`43b509d`	link	false	`/test pull-publishing-bot-validate`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-ci-robot requested review from andrewsykim, derekwaynecarr and a team June 28, 2023 23:14

akhilles reviewed Jun 29, 2023

View reviewed changes

robscott reviewed Jun 29, 2023

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 30, 2023

mskrocki force-pushed the escLib branch from fd21658 to e8aa6b2 Compare June 30, 2023 02:01

mskrocki force-pushed the escLib branch from e8aa6b2 to acd7623 Compare June 30, 2023 02:02

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2023

aojea reviewed Jun 30, 2023

View reviewed changes

akhilles reviewed Jul 6, 2023

View reviewed changes

mskrocki mentioned this pull request Jul 10, 2023

REQUEST: Create endpointslice repo kubernetes/org#4313

Closed

mskrocki force-pushed the escLib branch from 9fce2da to c5913e6 Compare July 10, 2023 16:31

staging: add endpointslice repo

22c6678

mskrocki force-pushed the escLib branch from c5913e6 to 02e9d3e Compare July 11, 2023 15:46

k8s-ci-robot assigned robscott Jul 11, 2023

mskrocki added 3 commits July 11, 2023 18:08

move endpointslice reconciler to staging endpointslice repo

29fad38

Convert controller name to reconciler variable.

7c87332

staging: Add endpointslice to publishing data.

43b509d

mskrocki force-pushed the escLib branch from 02e9d3e to 43b509d Compare July 11, 2023 18:09

k8s-ci-robot assigned aojea Jul 13, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2023

thockin reviewed Jul 13, 2023

View reviewed changes

k8s-ci-robot assigned thockin Jul 13, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2023

k8s-ci-robot merged commit 5c72df7 into kubernetes:master Jul 14, 2023
13 of 14 checks passed

k8s-ci-robot added this to the v1.28 milestone Jul 14, 2023

Atharva-Shinde mentioned this pull request Jul 15, 2023

Move EndpointSlice Reconciler into Staging kubernetes/enhancements#3685

Closed

5 tasks

Miciah mentioned this pull request Jul 18, 2023

TopologyAwareHints: Take lock in HasPopulatedHints #118189

Merged

		@@ -85,7 +85,7 @@ func NewPortMapKey(endpointPorts []discovery.EndpointPort) PortMapKey {
		// DeepHashObjectToString creates a unique hash string from a go object.
		func DeepHashObjectToString(objectToWrite interface{}) string {

Convert EndpointSlice Reconciler to a library in staging. #118953

Convert EndpointSlice Reconciler to a library in staging. #118953

Conversation

mskrocki commented Jun 28, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhilles Jul 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jun 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhilles Jul 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mskrocki commented Jul 10, 2023

mskrocki commented Jul 11, 2023

mskrocki commented Jul 11, 2023

mskrocki commented Jul 11, 2023

aojea commented Jul 13, 2023

aojea commented Jul 13, 2023

k8s-ci-robot commented Jul 13, 2023

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 13, 2023

k8s-triage-robot commented Jul 14, 2023

k8s-ci-robot commented Jul 14, 2023

mskrocki commented Jun 28, 2023 •

edited

akhilles Jul 6, 2023 •

edited

aojea commented Jun 30, 2023 •

edited

akhilles Jul 6, 2023 •

edited