Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[release-4.6] Bug 1900991: unidling: switch away from endpoints to the service #167

Merged
merged 2 commits into from Feb 4, 2021
Merged

[release-4.6] Bug 1900991: unidling: switch away from endpoints to the service #167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b79335
Switch unidle to remove annotations from the service
Jan 28, 2021
0a95c56
unidling: remove idle annotations from endpoints
Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/cmd/controller/unidling.go
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@ func RunUnidlingController(ctx *ControllerContext) (bool, error) {
ctx.RestMapper,
coreClient,
coreClient,
coreClient,
appsClient.AppsV1(),
coreClient,
resyncPeriod,
Expand Down
70 changes: 56 additions & 14 deletions pkg/unidling/controller/unidling_controller.go
View file
Open in desktop
Expand Up @@ -69,6 +69,7 @@ type UnidlingController struct {
scaleNamespacer scale.ScalesGetter
mapper meta.RESTMapper
endpointsNamespacer corev1client.EndpointsGetter
servicesNamespacer corev1client.ServicesGetter
queue workqueue.RateLimitingInterface
lastFiredCache *lastFiredCache

Expand All @@ -77,7 +78,7 @@ type UnidlingController struct {
rcNamespacer corev1client.ReplicationControllersGetter
}

func NewUnidlingController(scaleNS scale.ScalesGetter, mapper meta.RESTMapper, endptsNS corev1client.EndpointsGetter, evtNS corev1client.EventsGetter,
func NewUnidlingController(scaleNS scale.ScalesGetter, mapper meta.RESTMapper, endptsNS corev1client.EndpointsGetter, servicesNS corev1client.ServicesGetter, evtNS corev1client.EventsGetter,
dcNamespacer appstypedclient.DeploymentConfigsGetter, rcNamespacer corev1client.ReplicationControllersGetter,
resyncPeriod time.Duration) *UnidlingController {
fieldSet := fields.Set{}
Expand All @@ -88,6 +89,7 @@ func NewUnidlingController(scaleNS scale.ScalesGetter, mapper meta.RESTMapper, e
scaleNamespacer: scaleNS,
mapper: mapper,
endpointsNamespacer: endptsNS,
servicesNamespacer: servicesNS,
queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "unidling"),
lastFiredCache: &lastFiredCache{
items: make(map[types.NamespacedName]time.Time),
Expand Down Expand Up @@ -257,20 +259,20 @@ func (c *UnidlingController) awaitRequest() bool {
}

// handleRequest handles a single request to unidle. After checking the validity of the request,
// it will examine the endpoints in question to determine which scalables to scale, and will scale
// them and remove them from the endpoints' list of idled scalables. If it is unable to properly
// it will examine the service in question to determine which scalables to scale, and will scale
// them and remove them from the services' list of idled scalables. If it is unable to properly
// process the request, it will return a boolean indicating whether or not we should retry later,
// as well as an error (e.g. if we're unable to parse an annotation, retrying later won't help,
// so it will return false).
func (c *UnidlingController) handleRequest(info types.NamespacedName, lastFired time.Time) (bool, error) {
// fetch the endpoints associated with the service in question
targetEndpoints, err := c.endpointsNamespacer.Endpoints(info.Namespace).Get(context.TODO(), info.Name, metav1.GetOptions{})
// fetch the service in question
targetService, err := c.servicesNamespacer.Services(info.Namespace).Get(context.TODO(), info.Name, metav1.GetOptions{})
if err != nil {
return true, fmt.Errorf("unable to retrieve endpoints: %v", err)
return true, fmt.Errorf("unable to retrieve service: %v", err)
}

// make sure we actually were idled...
idledTimeRaw, wasIdled := targetEndpoints.Annotations[unidlingapi.IdledAtAnnotation]
idledTimeRaw, wasIdled := targetService.Annotations[unidlingapi.IdledAtAnnotation]
if !wasIdled {
klog.V(5).Infof("UnidlingController received a NeedPods event for a service that was not idled, ignoring")
return false, nil
Expand All @@ -289,7 +291,7 @@ func (c *UnidlingController) handleRequest(info types.NamespacedName, lastFired

// TODO: ew, this is metav1. Such is life when working with annotations.
var targetScalables []unidlingapi.RecordedScaleReference
if targetScalablesStr, hasTargetScalables := targetEndpoints.Annotations[unidlingapi.UnidleTargetAnnotation]; hasTargetScalables {
if targetScalablesStr, hasTargetScalables := targetService.Annotations[unidlingapi.UnidleTargetAnnotation]; hasTargetScalables {
if err = json.Unmarshal([]byte(targetScalablesStr), &targetScalables); err != nil {
// retrying here won't help, we're just stuck as idled since we can't parse the idled scalables list
return false, fmt.Errorf("unable to unmarshal target scalable references: %v", err)
Expand Down Expand Up @@ -355,25 +357,65 @@ func (c *UnidlingController) handleRequest(info types.NamespacedName, lastFired
}

if len(newAnnotationList) == 0 {
delete(targetEndpoints.Annotations, unidlingapi.UnidleTargetAnnotation)
delete(targetEndpoints.Annotations, unidlingapi.IdledAtAnnotation)
delete(targetService.Annotations, unidlingapi.UnidleTargetAnnotation)
delete(targetService.Annotations, unidlingapi.IdledAtAnnotation)
} else {
var newAnnotationBytes []byte
newAnnotationBytes, err = json.Marshal(newAnnotationList)
if err != nil {
utilruntime.HandleError(fmt.Errorf("unable to update/remove idle annotations from %s/%s: unable to marshal list of remaining scalables, removing list entirely: %v", info.Namespace, info.Name, err))

delete(targetEndpoints.Annotations, unidlingapi.UnidleTargetAnnotation)
delete(targetEndpoints.Annotations, unidlingapi.IdledAtAnnotation)
delete(targetService.Annotations, unidlingapi.UnidleTargetAnnotation)
delete(targetService.Annotations, unidlingapi.IdledAtAnnotation)
} else {
targetEndpoints.Annotations[unidlingapi.UnidleTargetAnnotation] = string(newAnnotationBytes)
targetService.Annotations[unidlingapi.UnidleTargetAnnotation] = string(newAnnotationBytes)
}
}

if _, err = c.endpointsNamespacer.Endpoints(info.Namespace).Update(context.TODO(), targetEndpoints, metav1.UpdateOptions{}); err != nil {
if _, err = c.servicesNamespacer.Services(info.Namespace).Update(context.TODO(), targetService, metav1.UpdateOptions{}); err != nil {
return true, fmt.Errorf("unable to update/remove idle annotations from %s/%s: %v", info.Namespace, info.Name, err)
}

// oc idle still annotates endpoints for backwards
// compatibilty. We need to remove any idled annotations on
// the endpoints.

// fetch the endpoints in question
targetEndpoints, err := c.endpointsNamespacer.Endpoints(info.Namespace).Get(context.TODO(), info.Name, metav1.GetOptions{})
if err != nil {
return true, fmt.Errorf("unable to retrieve endpoints: %v", err)
}

// make sure we actually were idled...
idledTimeRaw, wasIdled = targetEndpoints.Annotations[unidlingapi.IdledAtAnnotation]
if !wasIdled {
klog.V(5).Infof("UnidlingController received a NeedPods event for a service that was not idled, ignoring")
return false, nil
}

// ...and make sure this request was to wake up from the most recent idling, and not a previous one
idledTime, err = time.Parse(time.RFC3339, idledTimeRaw)
if err != nil {
// retrying here won't help, we're just stuck as idle since we can't get parse the idled time
return false, fmt.Errorf("unable to check idled-at time: %v", err)
}
if lastFired.Before(idledTime) {
klog.V(5).Infof("UnidlingController received an out-of-date NeedPods event, ignoring")
return false, nil
}

_, unidledAnnotation := targetEndpoints.Annotations[unidlingapi.UnidleTargetAnnotation]
_, idledAtAnnotation := targetEndpoints.Annotations[unidlingapi.IdledAtAnnotation]

if unidledAnnotation || idledAtAnnotation {
delete(targetEndpoints.Annotations, unidlingapi.UnidleTargetAnnotation)
delete(targetEndpoints.Annotations, unidlingapi.IdledAtAnnotation)

if _, err = c.endpointsNamespacer.Endpoints(info.Namespace).Update(context.TODO(), targetEndpoints, metav1.UpdateOptions{}); err != nil {
return true, fmt.Errorf("unable to update/remove idle annotations from %s/%s: %v", info.Namespace, info.Name, err)
}
}

return false, nil
}

Expand Down
95 changes: 50 additions & 45 deletions pkg/unidling/controller/unidling_controller_test.go
View file
Open in desktop
Expand Up @@ -24,8 +24,8 @@ import (
)

type fakeResults struct {
resMap map[unidlingapi.CrossGroupObjectReference]autoscalingv1.Scale
resEndpoints *corev1.Endpoints
resMap map[unidlingapi.CrossGroupObjectReference]autoscalingv1.Scale
resService *corev1.Service
}

func prepFakeClient(t *testing.T, nowTime time.Time, scales ...autoscalingv1.Scale) (*kexternalfake.Clientset, *appsfake.Clientset, *scalefake.FakeScaleClient, meta.RESTMapper, *fakeResults) {
Expand All @@ -50,7 +50,7 @@ func prepFakeClient(t *testing.T, nowTime time.Time, scales ...autoscalingv1.Sca
t.Fatalf("unexpected error: %v", err)
}

endpointsObj := corev1.Endpoints{
serviceObj := corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "somesvc",
Annotations: map[string]string{
Expand All @@ -59,9 +59,10 @@ func prepFakeClient(t *testing.T, nowTime time.Time, scales ...autoscalingv1.Sca
},
},
}
fakeClient.PrependReactor("get", "endpoints", func(action clientgotesting.Action) (bool, runtime.Object, error) {
if action.(clientgotesting.GetAction).GetName() == endpointsObj.Name {
return true, &endpointsObj, nil

fakeClient.PrependReactor("get", "services", func(action clientgotesting.Action) (bool, runtime.Object, error) {
if action.(clientgotesting.GetAction).GetName() == serviceObj.Name {
return true, &serviceObj, nil
}

return false, nil, nil
Expand Down Expand Up @@ -169,13 +170,13 @@ func prepFakeClient(t *testing.T, nowTime time.Time, scales ...autoscalingv1.Sca
return true, nil, errors.NewNotFound(action.GetResource().GroupResource(), patchAction.GetName())
})

fakeClient.AddReactor("*", "endpoints", func(action clientgotesting.Action) (bool, runtime.Object, error) {
obj := action.(clientgotesting.UpdateAction).GetObject().(*corev1.Endpoints)
if obj.Name != endpointsObj.Name {
fakeClient.AddReactor("*", "services", func(action clientgotesting.Action) (bool, runtime.Object, error) {
obj := action.(clientgotesting.UpdateAction).GetObject().(*corev1.Service)
if obj.Name != serviceObj.Name {
return false, nil, nil
}

res.resEndpoints = obj
res.resService = obj

return true, obj, nil
})
Expand Down Expand Up @@ -226,6 +227,7 @@ func TestControllerHandlesStaleEvents(t *testing.T) {
controller := &UnidlingController{
mapper: mapper,
endpointsNamespacer: fakeClient.CoreV1(),
servicesNamespacer: fakeClient.CoreV1(),
rcNamespacer: fakeClient.CoreV1(),
dcNamespacer: fakeDeployClient.AppsV1(),
scaleNamespacer: fakeScaleClient,
Expand All @@ -244,8 +246,8 @@ func TestControllerHandlesStaleEvents(t *testing.T) {
t.Errorf("Did not expect to have anything scaled, but got %v", res.resMap)
}

if res.resEndpoints != nil {
t.Errorf("Did not expect to have endpoints object updated, but got %v", res.resEndpoints)
if res.resService != nil {
t.Errorf("Did not expect to have service object updated, but got %v", res.resService)
}
}

Expand Down Expand Up @@ -282,8 +284,9 @@ func TestControllerIgnoresAlreadyScaledObjects(t *testing.T) {

controller := &UnidlingController{
mapper: mapper,
scaleNamespacer: fakeScaleClient,
endpointsNamespacer: fakeClient.CoreV1(),
scaleNamespacer: fakeScaleClient,
servicesNamespacer: fakeClient.CoreV1(),
rcNamespacer: fakeClient.CoreV1(),
dcNamespacer: fakeDeployClient.AppsV1(),
}
Expand Down Expand Up @@ -325,12 +328,12 @@ func TestControllerIgnoresAlreadyScaledObjects(t *testing.T) {
}
}

if res.resEndpoints == nil {
t.Fatalf("Expected endpoints object to be updated, but it was not")
if res.resService == nil {
t.Fatalf("Expected service object to be updated, but it was not")
}

resTargetsRaw, hadTargets := res.resEndpoints.Annotations[unidlingapi.UnidleTargetAnnotation]
resIdledTimeRaw, hadIdledTime := res.resEndpoints.Annotations[unidlingapi.IdledAtAnnotation]
resTargetsRaw, hadTargets := res.resService.Annotations[unidlingapi.UnidleTargetAnnotation]
resIdledTimeRaw, hadIdledTime := res.resService.Annotations[unidlingapi.IdledAtAnnotation]

if !hadTargets {
t.Errorf("Expected targets annotation to still be present, but it was not")
Expand Down Expand Up @@ -396,6 +399,7 @@ func TestControllerUnidlesProperly(t *testing.T) {
controller := &UnidlingController{
mapper: mapper,
endpointsNamespacer: fakeClient.CoreV1(),
servicesNamespacer: fakeClient.CoreV1(),
rcNamespacer: fakeClient.CoreV1(),
dcNamespacer: fakeDeployClient.AppsV1(),
scaleNamespacer: fakeScaleClient,
Expand Down Expand Up @@ -430,12 +434,12 @@ func TestControllerUnidlesProperly(t *testing.T) {
}
}

if res.resEndpoints == nil {
t.Fatalf("Expected endpoints object to be updated, but it was not")
if res.resService == nil {
t.Fatalf("Expected service object to be updated, but it was not")
}

resTargets, hadTargets := res.resEndpoints.Annotations[unidlingapi.UnidleTargetAnnotation]
resIdledTime, hadIdledTime := res.resEndpoints.Annotations[unidlingapi.IdledAtAnnotation]
resTargets, hadTargets := res.resService.Annotations[unidlingapi.UnidleTargetAnnotation]
resIdledTime, hadIdledTime := res.resService.Annotations[unidlingapi.IdledAtAnnotation]

if hadTargets {
t.Errorf("Expected targets annotation to be removed, but it was %q", resTargets)
Expand All @@ -447,11 +451,11 @@ func TestControllerUnidlesProperly(t *testing.T) {
}

type failureTestInfo struct {
name string
endpointsGet *corev1.Endpoints
scaleGets []autoscalingv1.Scale
scaleUpdatesNotFound []bool
preventEndpointsUpdate bool
name string
servicesGet *corev1.Service
scaleGets []autoscalingv1.Scale
scaleUpdatesNotFound []bool
preventServicesUpdate bool

errorExpected bool
retryExpected bool
Expand All @@ -463,10 +467,10 @@ func prepareFakeClientForFailureTest(test failureTestInfo) (*kexternalfake.Clien
fakeDeployClient := &appsfake.Clientset{}
fakeScaleClient := &scalefake.FakeScaleClient{}

fakeClient.PrependReactor("get", "endpoints", func(action clientgotesting.Action) (bool, runtime.Object, error) {
fakeClient.PrependReactor("get", "services", func(action clientgotesting.Action) (bool, runtime.Object, error) {
objName := action.(clientgotesting.GetAction).GetName()
if test.endpointsGet != nil && objName == test.endpointsGet.Name {
return true, test.endpointsGet, nil
if test.servicesGet != nil && objName == test.servicesGet.Name {
return true, test.servicesGet, nil
}

return true, nil, errors.NewNotFound(action.GetResource().GroupResource(), objName)
Expand Down Expand Up @@ -537,14 +541,14 @@ func prepareFakeClientForFailureTest(test failureTestInfo) (*kexternalfake.Clien
return true, nil, errors.NewNotFound(action.GetResource().GroupResource(), obj.Name)
})

fakeClient.PrependReactor("update", "endpoints", func(action clientgotesting.Action) (bool, runtime.Object, error) {
obj := action.(clientgotesting.UpdateAction).GetObject().(*corev1.Endpoints)
if obj.Name != test.endpointsGet.Name {
fakeClient.PrependReactor("update", "services", func(action clientgotesting.Action) (bool, runtime.Object, error) {
obj := action.(clientgotesting.UpdateAction).GetObject().(*corev1.Service)
if obj.Name != test.servicesGet.Name {
return false, nil, nil
}

if test.preventEndpointsUpdate {
return true, nil, fmt.Errorf("some problem updating the endpoints")
if test.preventServicesUpdate {
return true, nil, fmt.Errorf("some problem updating the service")
}

return true, obj, nil
Expand Down Expand Up @@ -626,14 +630,14 @@ func TestControllerPerformsCorrectlyOnFailures(t *testing.T) {

tests := []failureTestInfo{
{
name: "retry on failed endpoints get",
endpointsGet: nil,
name: "retry on failed service get",
servicesGet: nil,
errorExpected: true,
retryExpected: true,
},
{
name: "not retry on failure to parse time",
endpointsGet: &corev1.Endpoints{
servicesGet: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "somesvc",
Annotations: map[string]string{
Expand All @@ -646,7 +650,7 @@ func TestControllerPerformsCorrectlyOnFailures(t *testing.T) {
},
{
name: "not retry on failure to unmarshal target scalables",
endpointsGet: &corev1.Endpoints{
servicesGet: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "somesvc",
Annotations: map[string]string{
Expand All @@ -660,7 +664,7 @@ func TestControllerPerformsCorrectlyOnFailures(t *testing.T) {
},
{
name: "remove a scalable from the list if it cannot be found (while getting)",
endpointsGet: &corev1.Endpoints{
servicesGet: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "somesvc",
Annotations: map[string]string{
Expand Down Expand Up @@ -689,7 +693,7 @@ func TestControllerPerformsCorrectlyOnFailures(t *testing.T) {
},
{
name: "should remove a scalable from the list if it cannot be found (while updating)",
endpointsGet: &corev1.Endpoints{
servicesGet: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "somesvc",
Annotations: map[string]string{
Expand Down Expand Up @@ -727,8 +731,8 @@ func TestControllerPerformsCorrectlyOnFailures(t *testing.T) {
},
},
{
name: "retry on failed endpoints update",
endpointsGet: &corev1.Endpoints{
name: "retry on failed service update",
servicesGet: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "somesvc",
Annotations: map[string]string{
Expand Down Expand Up @@ -758,9 +762,9 @@ func TestControllerPerformsCorrectlyOnFailures(t *testing.T) {
Spec: autoscalingv1.ScaleSpec{Replicas: 0},
},
},
preventEndpointsUpdate: true,
errorExpected: true,
retryExpected: true,
preventServicesUpdate: true,
errorExpected: true,
retryExpected: true,
},
}

Expand All @@ -769,6 +773,7 @@ func TestControllerPerformsCorrectlyOnFailures(t *testing.T) {
controller := &UnidlingController{
mapper: mapper,
endpointsNamespacer: fakeClient.CoreV1(),
servicesNamespacer: fakeClient.CoreV1(),
rcNamespacer: fakeClient.CoreV1(),
dcNamespacer: fakeDeployClient.AppsV1(),
scaleNamespacer: fakeScaleClient,
Expand Down