Bug 1900989: Move idle check from endpoints to service #225

frobware · 2020-11-27T19:09:39Z

As we have moved to endpointslices we now check the service for the
idled annotation. oc idle has been updated to annotate the service.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1900989

frobware · 2020-11-27T19:10:00Z

/hold

openshift-ci-robot · 2020-11-27T19:10:00Z

@frobware: This pull request references Bugzilla bug 1900989, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1900989: Ensure idled services work with endpointslices

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.

frobware · 2020-11-30T15:55:48Z

Installing from initial release registry.build02.ci.openshift.org/ci-op-t951pb9z/release@sha256:b0b724e3b077f637528f0ea67b196214e5f661148a7873bbdd2173730a3dc41a
level=info msg=Credentials loaded from environment variable "GOOGLE_CLOUD_KEYFILE_JSON", file "/etc/openshift-installer/gce.json"
level=warning msg=Found override for release image. Please be warned, this is not advised
level=info msg=Consuming Install Config from target directory
level=fatal msg=failed to fetch Cluster: failed to fetch dependency of "Cluster": failed to generate asset "Platform Provisioning Check": baseDomain: Internal error: failed to list DNS Zones: googleapi: Error 429: Resource has been exhausted (e.g. check quota)., rateLimitExceeded

/test e2e-upgrade

frobware · 2020-11-30T21:28:38Z

/test images

openshift-merge-robot · 2020-11-30T21:40:46Z

@frobware: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-agnostic	`25378d0`	link	`/test e2e-agnostic`

Full PR test history. Your PR dashboard.

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.

frobware · 2021-01-04T09:36:24Z

/retest

sgreene570 · 2021-01-05T20:35:02Z

An oc idle <service> will annotate the endpoint with an 'idle time'
annotation. We don't or can't add this annotation to endpointslices in
openshift/oc (idle) because the underlying endpointslices are managed
by the kube-controller-manager and that doesn't propagate annotations
as part of the mirroring service.

Is it at all possible to have oc idle pick up the endpointslice for a service and annotate that directly?

frobware · 2021-01-06T09:08:38Z

Is it at all possible to have oc idle pick up the endpointslice for a service and annotate that directly?

This was my first thought too. And this is how I initially implemented it. But it turns out that the endpointslice is managed by the endpoints/endpointslice mirroring service so although you can annotate the endpointslice and, with good luck/timing you do see that delivered, it is soon overwritten by the mirroring service. I got caught out when testing this because the first few times it worked (as in I received and endpointslice with the expected annotation) but sometimes I was stepping through the behaviour in the debugger and I noticed that sometimes the annotation had disappeared.

pkg/router/controller/factory/factory.go

pkg/router/controller/router_controller.go

frobware · 2021-01-18T09:33:07Z

We may be better off carrying kubernetes/kubernetes#98116

frobware · 2021-01-19T13:03:11Z

~~This is the fix: openshift/openshift-controller-manager#159~~

frobware · 2021-01-19T13:03:24Z

/hold

aojea · 2021-01-19T15:59:04Z

/cc

openshift-ci-robot · 2021-01-27T14:15:47Z

@frobware: This pull request references Bugzilla bug 1900989, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

[WIP] Bug 1900989: Ensure idled services work with endpointslices

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.

frobware · 2021-01-27T14:48:23Z

/hold cancel

frobware · 2021-01-27T14:51:00Z

Is it at all possible to have oc idle pick up the endpointslice for a service and annotate that directly?

We're now annotating the service; see openshift/oc#720

aojea · 2021-01-27T14:59:04Z

pkg/router/template/plugin.go

+			svcSubset := kapi.EndpointSubset{
+				Addresses: []kapi.EndpointAddress{
+					{
+						IP: service.Spec.ClusterIP,


just FYI, with dual stack this will be an slice service.Spec.ClusterIPs

Spoke with @frobware, we can address this in a follow-up

sgreene570 · 2021-01-27T18:44:54Z

pkg/router/template/plugin.go

+	for i := range subsets {
+		if len(subsets[i].Addresses) > 0 {


Suggested change

for i := range subsets {

if len(subsets[i].Addresses) > 0 {

for _, subset := range subsets {

if len(subset.Addresses) > 0 {

Why bother with the copy? It's not zero cost vis-a-vis accessing via the index.

Consistency, I guess. But you do bring up a point about the copy, so I don't feel strongly enough to contend.

pkg/router/template/plugin.go

As we have moved to endpointslices we now check the service for the idled annotation. `oc idle` has been updated to annotate the service.

sgreene570 · 2021-01-27T20:47:06Z

/lgtm

openshift-ci-robot · 2021-01-27T20:47:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, sgreene570

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~~ [frobware,sgreene570]

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

openshift-bot · 2021-01-28T04:26:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-28T04:51:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2021-01-28T06:18:32Z

@frobware: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/oc#720 is open

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with /bugzilla refresh.

Bugzilla bug 1900989 has not been moved to the MODIFIED state.

In response to this:

Bug 1900989: Move idle check from endpoints to service

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.

The router (in 4.7) has moved to using endpointslices. Rather than annotating both endpoints and endpointslices oc/idle will now annotate the service associated with the endpoints. Similarly unidling now needs to remove those idle annotations from the service. - openshift/oc#720 - openshift/router#225

oc/idle is now updating the service in addition to the endpoints when idling. Related PRs: - openshift/openshift-controller-manager#165 - openshift/oc#720 - openshift/router#225 - openshift/sdn#252

oc/idle will now additionally annotate the service. The test explicitly deletes it so removing that call so that we can verify the following PRs, all related to annotating the service when idling. - openshift/oc#720 - openshift/router#225 - openshift/sdn#252 - openshift/openshift-apiserver#180 - openshift/openshift-controller-manager#165

oc/idle is now updating the service in addition to the endpoints when idling. Related PRs: - openshift/openshift-controller-manager#165 - openshift/oc#720 - openshift/router#225 - openshift/sdn#252

The router (in 4.7) has moved to using endpointslices. Rather than annotating both endpoints and endpointslices oc/idle will now annotate the service associated with the endpoints. Similarly unidling now needs to remove those idle annotations from the service. - openshift/oc#720 - openshift/router#225

frobware · 2021-02-05T10:49:50Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2021-02-05T10:50:30Z

@frobware: new pull request created: #253

In response to this:

/cherry-pick release-4.6

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.

frobware changed the title ~~Ensure idled services work with endpointslices~~ Bug 1900989: Ensure idled services work with endpointslices Nov 27, 2020

openshift-ci-robot requested review from ironcladlou and smarterclayton November 27, 2020 19:09

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. labels Nov 27, 2020

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 27, 2020

frobware changed the title ~~Bug 1900989: Ensure idled services work with endpointslices~~ [WIP] Bug 1900989: Ensure idled services work with endpointslices Nov 30, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2020

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2020

sgreene570 reviewed Jan 14, 2021

View reviewed changes

pkg/router/controller/factory/factory.go Outdated Show resolved Hide resolved

sgreene570 reviewed Jan 14, 2021

View reviewed changes

pkg/router/controller/router_controller.go Outdated Show resolved Hide resolved

sgreene570 reviewed Jan 14, 2021

View reviewed changes

pkg/router/controller/router_controller.go Outdated Show resolved Hide resolved

openshift-ci-robot requested a review from aojea January 19, 2021 15:59

frobware force-pushed the fix-idled-services branch from 25378d0 to 5d6ea11 Compare January 19, 2021 16:59

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

frobware force-pushed the fix-idled-services branch from 5d6ea11 to 85160a4 Compare January 27, 2021 14:16

frobware changed the title ~~[WIP] Bug 1900989: Ensure idled services work with endpointslices~~ Bug 1900989: Move idle check from endpoints to service Jan 27, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 27, 2021

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

aojea reviewed Jan 27, 2021

View reviewed changes

sgreene570 reviewed Jan 27, 2021

View reviewed changes

Move idled services checks from endpoints to the service

3b24611

As we have moved to endpointslices we now check the service for the idled annotation. `oc idle` has been updated to annotate the service.

frobware force-pushed the fix-idled-services branch from b873ba1 to 3b24611 Compare January 27, 2021 20:35

openshift-ci-robot assigned sgreene570 Jan 27, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2021

openshift-merge-robot merged commit 30e2fb5 into openshift:master Jan 28, 2021

frobware mentioned this pull request Jan 28, 2021

Bug 1900989: unidling: switch away from endpoints to the service openshift/openshift-controller-manager#165

Merged

This was referenced Jan 29, 2021

Bug 1900989: policy/unidling-controller: allow get/update on services openshift/openshift-apiserver#180

Merged

debug idle annotations - DO NOT MERGE #252

Closed

frobware mentioned this pull request Jan 29, 2021

Bug 1900989: test/cmd/idle: don't delete the service during the test openshift/origin#25840

Closed

openshift-cherrypick-robot mentioned this pull request Feb 5, 2021

[release-4.6] Bug 1900991: Move idle check from endpoints to service #253

Merged

frobware deleted the fix-idled-services branch May 1, 2024 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1900989: Move idle check from endpoints to service #225

Bug 1900989: Move idle check from endpoints to service #225

frobware commented Nov 27, 2020 •

edited

Loading

frobware commented Nov 27, 2020

openshift-ci-robot commented Nov 27, 2020

frobware commented Nov 30, 2020

frobware commented Nov 30, 2020

openshift-merge-robot commented Nov 30, 2020

frobware commented Jan 4, 2021

sgreene570 commented Jan 5, 2021

frobware commented Jan 6, 2021

frobware commented Jan 18, 2021

frobware commented Jan 19, 2021 •

edited

Loading

frobware commented Jan 19, 2021

aojea commented Jan 19, 2021

openshift-ci-robot commented Jan 27, 2021

frobware commented Jan 27, 2021

frobware commented Jan 27, 2021

aojea Jan 27, 2021

sgreene570 Jan 27, 2021

sgreene570 Jan 27, 2021

frobware Jan 27, 2021

sgreene570 Jan 27, 2021

sgreene570 commented Jan 27, 2021

openshift-ci-robot commented Jan 27, 2021

openshift-bot commented Jan 28, 2021

openshift-bot commented Jan 28, 2021

openshift-ci-robot commented Jan 28, 2021

frobware commented Feb 5, 2021

openshift-cherrypick-robot commented Feb 5, 2021

Bug 1900989: Move idle check from endpoints to service #225

Bug 1900989: Move idle check from endpoints to service #225

Conversation

frobware commented Nov 27, 2020 • edited Loading

frobware commented Nov 27, 2020

openshift-ci-robot commented Nov 27, 2020

frobware commented Nov 30, 2020

frobware commented Nov 30, 2020

openshift-merge-robot commented Nov 30, 2020

frobware commented Jan 4, 2021

sgreene570 commented Jan 5, 2021

frobware commented Jan 6, 2021

frobware commented Jan 18, 2021

frobware commented Jan 19, 2021 • edited Loading

frobware commented Jan 19, 2021

aojea commented Jan 19, 2021

openshift-ci-robot commented Jan 27, 2021

frobware commented Jan 27, 2021

frobware commented Jan 27, 2021

aojea Jan 27, 2021

Choose a reason for hiding this comment

sgreene570 Jan 27, 2021

Choose a reason for hiding this comment

sgreene570 Jan 27, 2021

Choose a reason for hiding this comment

frobware Jan 27, 2021

Choose a reason for hiding this comment

sgreene570 Jan 27, 2021

Choose a reason for hiding this comment

sgreene570 commented Jan 27, 2021

openshift-ci-robot commented Jan 27, 2021

openshift-bot commented Jan 28, 2021

openshift-bot commented Jan 28, 2021

openshift-ci-robot commented Jan 28, 2021

frobware commented Feb 5, 2021

openshift-cherrypick-robot commented Feb 5, 2021

frobware commented Nov 27, 2020 •

edited

Loading

frobware commented Jan 19, 2021 •

edited

Loading