Add previous-scale annotation for idled resources by DirectXMan12 · Pull Request #10421 · openshift/origin

DirectXMan12 · 2016-08-15T18:01:58Z

This records the previous scale of an idled resource on idling.
This is useful in situations where finding and loading the associated
endpoints object would non-ideal (e.g. the web console).

DirectXMan12 · 2016-08-15T18:02:13Z

cc @smarterclayton @jwforres

knobunc · 2016-08-15T20:10:50Z

LGTM

sjenning · 2016-08-15T20:43:32Z

lgtm

ncdc · 2016-08-16T18:57:46Z

@smarterclayton who is doing the api review? you / @deads2k / @liggitt / @jwforres ?

deads2k · 2016-08-16T20:22:38Z

pkg/unidling/api/types.go


+	// PreviousScaleAnnotation contains the previous scale of a scalable object
+	// (currently only applied by the idler)
+	PreviousScaleAnnotation = "scale.openshift.io/previous-scale"


This name doesn't match what this is doing. When I scale an object, I don't get this value filled in. This looks tightly coupled to idling.

Yeah, I would probably call this idling.alpha.openshift.io/previous-scale or something like that

I can, but when I was talking to @smarterclayton, we were discussing the idea of making something like this automatically applied on scales in the future (and then have scale.openshift.io/xyz passed through the scale subresource so that it can act like a prototype API field, just like other objects), so using something like this would future-proof that somewhat.

(although using scale.openshift.io instead of scale.kubernetes.io might complicate that somewhat)

I can, but when I was talking to @smarterclayton, we were discussing the idea of making something like this automatically applied on scales in the future (and then have scale.openshift.io/xyz passed through the scale subresource so that it can act like a prototype API field, just like other objects), so using something like this would future-proof that somewhat.

I'm not really convinced that I'd like to use an annotation to remember previous state (see problems with the other kube attempts to use annotations to store history). I'm also not seeing a lot of value for "undo" in the general case. It seems like it would open us up to a new class of privilege escalation attacks if we had a controller that attempted to reconcile this annotation automatically.

I'd rather target the information that we want to preserve for its current usage.

@smarterclayton is there some more complete design that covers how we'd resolve the problems of history tracking annotations and privilege escalation attacks based on controllers watching annotations (think DC scaling existing RC, not scaling the current object)? Are you ok with scoping this to address our current, narrow need first?

deads2k · 2016-08-16T20:29:30Z

Name needs updating and it would be nice to see a CLI usage demonstrating your intent, which I think is something like:

create thing
scale thing up
idle thing
knowing only the thing.Name, scale it back to the value from step 2.

Other than the name, I'm fine with this.

DirectXMan12 · 2016-08-16T21:08:07Z

CLI usage

ack, will add tomorrow

deads2k · 2016-08-16T21:18:21Z

ack, will add tomorrow

Thanks. There's probably a test/cmd/*.sh script that would fit.

smarterclayton · 2016-08-16T21:56:52Z

Edit on a resource is edit on scale, so what's the privilege escalation?

Preserving the user's previous scale value is important - if the user has
an idled pet set, restoring scale to 1 on unidle is completely wrong,
likewise an app that is supposed to be HA (2 pods) that gets idled back to
1, but no human notices.

On Tue, Aug 16, 2016 at 5:18 PM, David Eads notifications@github.com
wrote:

ack, will add tomorrow

Thanks. There's probably a test/cmd/*.sh script that would fit.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10421 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p89IQC-PXl_Hw_ft_SOtFtdzwdrNks5qgikfgaJpZM4JknXz
.

deads2k · 2016-08-16T22:02:23Z

Preserving the user's previous scale value is important - if the user has
an idled pet set, restoring scale to 1 on unidle is completely wrong,

For idling, sure. No issue with this for idling. In the general case though, I don't see why I would try to build a scaling history

smarterclayton · 2016-08-16T22:04:28Z

Maybe I missed the context, but some status displaying recent scaling might
actually be useful for automated controllers. Hard to say, knowing "why"
someone last scaled (human, or automated) might be relevant. And is
relevant for idling in some contexts.

On Tue, Aug 16, 2016 at 6:02 PM, David Eads notifications@github.com
wrote:

Preserving the user's previous scale value is important - if the user has
an idled pet set, restoring scale to 1 on unidle is completely wrong,

For idling, sure. No issue with this for idling. In the general case
though, I don't see why I would try to build a scaling history

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10421 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p96lK1xzvY3f9NPO4gF78suoRU7Zks5qgjNxgaJpZM4JknXz
.

deads2k · 2016-08-16T22:16:53Z

Maybe I missed the context, but some status displaying recent scaling might
actually be useful for automated controllers.

Maybe, but I'd rather add it for a concrete reason, set in a way that makes sense, and get a name that makes sense. To be generally useful, you need to have the contract on the polymorphic scale endpoint change the annotation. This pull does none of those things.

To be clear, we're arguing over an annotation name here, not the use in this narrow case. The annotation is set only by idling for a narrow reason, I think it deserves a narrowly scoped name.

deads2k · 2016-08-16T22:19:45Z

Also, scale on a DC does not edit the DC and without access to the rcs you can never reproduce the effect since you don't know the shape of the RC you're scaling by touching the DC. It's like a dc status. It's actually mucking directly with rcs.

DirectXMan12 · 2016-08-17T01:22:16Z

Also, scale on a DC does not edit the DC

I believe it does, unless someone changed it since I added support for the /scale subresource on DCs. oc scale didn't used to, but when I added the scale endpoint, we changed it to edit the DC and reconcile the RCs with the DCs, so unless someone changed it after that, the /scale subresource modifies DCs.

smarterclayton · 2016-08-17T01:26:56Z

Scale on a DC does edit the DC

DirectXMan12 · 2016-08-17T01:38:40Z

To be generally useful, you need to have the contract on the polymorphic scale endpoint change the annotation. This pull does none of those things.

Sure, that's a thing that requires an update in the future (and it's something I intend to work on, as it would be generally useful to be able to prototype scale API fields the same way we prototype the addition of fields to other API objects), but I mainly wanted to have something that was forwards compatible, so to speak.

deads2k · 2016-08-17T02:01:33Z

but I mainly wanted to have something that was forwards compatible, so to speak.

Narrow name for a narrow case. The general solution to a theoretical general problem that requires upstream API changes we won't pick for a general need we don't have is probably more than is worth picking up here.

deads2k · 2016-08-17T11:41:36Z

Narrow name for a narrow case. The general solution to a theoretical general problem that requires upstream API changes we won't pick for a general need we don't have is probably more than is worth picking up here.

"idling.alpha.openshift.io/previous-scale" is perfect. It's scoped to its use, the name identifies who is setting it and it matches the behavior you'd expect when idling and unidling.

"scale.openshift.io/previous-scale" is overly broad. It suggests that the scale operation is controlling this (it isn't). To make it behave as expected, you'd have to change the contract on the polymorphic /scale endpoint. Even if you did that, such an annotation wouldn't be scoped to openshift. And, if you want this power out of scale, then retrieving via scale makes sense and this doesn't do that either.

ncdc · 2016-08-17T15:38:30Z

We can always deprecate idling.alpha.openshift.io/previous-scale and replace it with something else that's more appropriate for a broader scope when the time comes. I suggest we just use idling.alpha.openshift.io/previous-scale for now.

DirectXMan12 · 2016-08-17T15:50:31Z

yeah, for the sake of moving this forward I'll just change it to the annotation suggested by @deads2k

deads2k · 2016-08-17T17:04:24Z

lgtm [merge]

openshift-bot · 2016-08-17T17:16:17Z

[Test]ing while waiting on the merge queue

DirectXMan12 · 2016-08-17T19:46:16Z

Missing quote in the cmd test. Fixed.

smarterclayton · 2016-08-17T21:12:05Z

[merge]

openshift-bot · 2016-08-17T22:27:26Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8206/) (Image: devenv-rhel7_4873)

This records the previous scale of an idled resource on idling. This is useful in situations where finding and loading the associated endpoints object would non-ideal (e.g. the web console).

deads2k · 2016-08-18T14:37:11Z

@DirectXMan12 any real changes?

DirectXMan12 · 2016-08-18T14:47:02Z

@deads2k no, just rebased b/c there was a weird error in the console log of the tests about one of the files needing a merge.

deads2k · 2016-08-18T14:49:18Z

[merge]

openshift-bot · 2016-08-18T14:52:37Z

Evaluated for origin merge up to 20ab280

ncdc · 2016-08-18T22:19:28Z

[test] for fun

openshift-bot · 2016-08-18T22:22:23Z

Evaluated for origin test up to 20ab280

openshift-bot · 2016-08-19T01:38:20Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8208/)

smarterclayton added the needs-api-review label Aug 15, 2016

deads2k reviewed Aug 16, 2016
View reviewed changes

DirectXMan12 force-pushed the feature/idling-prev-scale branch from 36afd51 to 53aa583 Compare August 17, 2016 15:57

deads2k added api-approved and removed needs-api-review labels Aug 17, 2016

jwforres mentioned this pull request Aug 17, 2016

Unidle to previous scale annotation when it exists openshift/origin-web-console#406

Merged

DirectXMan12 force-pushed the feature/idling-prev-scale branch from 53aa583 to 13d4018 Compare August 17, 2016 19:34

DirectXMan12 force-pushed the feature/idling-prev-scale branch from 13d4018 to 20ab280 Compare August 18, 2016 14:31

Add previous-scale annotation for idled resources

20ab280

This records the previous scale of an idled resource on idling. This is useful in situations where finding and loading the associated endpoints object would non-ideal (e.g. the web console).

openshift-bot merged commit d4a64c0 into openshift:master Aug 19, 2016

DirectXMan12 deleted the feature/idling-prev-scale branch August 19, 2016 01:59

Conversation

DirectXMan12 commented Aug 15, 2016

Uh oh!

DirectXMan12 commented Aug 15, 2016

Uh oh!

knobunc commented Aug 15, 2016

Uh oh!

sjenning commented Aug 15, 2016

Uh oh!

ncdc commented Aug 16, 2016

Uh oh!

deads2k Aug 16, 2016

Choose a reason for hiding this comment

Uh oh!

ncdc Aug 16, 2016

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Aug 16, 2016

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Aug 16, 2016

Choose a reason for hiding this comment

Uh oh!

deads2k Aug 16, 2016

Choose a reason for hiding this comment

Uh oh!

deads2k commented Aug 16, 2016

Uh oh!

DirectXMan12 commented Aug 16, 2016

Uh oh!

deads2k commented Aug 16, 2016

Uh oh!

smarterclayton commented Aug 16, 2016

Uh oh!

deads2k commented Aug 16, 2016

Uh oh!

smarterclayton commented Aug 16, 2016

Uh oh!

deads2k commented Aug 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k commented Aug 16, 2016

Uh oh!

DirectXMan12 commented Aug 17, 2016

Uh oh!

smarterclayton commented Aug 17, 2016 via email

Uh oh!

DirectXMan12 commented Aug 17, 2016

Uh oh!

deads2k commented Aug 17, 2016

Uh oh!

deads2k commented Aug 17, 2016

Uh oh!

ncdc commented Aug 17, 2016

Uh oh!

DirectXMan12 commented Aug 17, 2016

Uh oh!

deads2k commented Aug 17, 2016

Uh oh!

openshift-bot commented Aug 17, 2016

Uh oh!

DirectXMan12 commented Aug 17, 2016

Uh oh!

smarterclayton commented Aug 17, 2016

Uh oh!

openshift-bot commented Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k commented Aug 18, 2016

Uh oh!

DirectXMan12 commented Aug 18, 2016

Uh oh!

deads2k commented Aug 18, 2016

Uh oh!

openshift-bot commented Aug 18, 2016

Uh oh!

ncdc commented Aug 18, 2016

Uh oh!

openshift-bot commented Aug 18, 2016

Uh oh!

deads2k commented Aug 16, 2016 •

edited

Loading

openshift-bot commented Aug 17, 2016 •

edited

Loading