Skip to content

Comments

Bug 1390467 idling services#3312

Merged
bfallonf merged 1 commit intoopenshift:masterfrom
bfallonf:idling_1390467
Dec 7, 2016
Merged

Bug 1390467 idling services#3312
bfallonf merged 1 commit intoopenshift:masterfrom
bfallonf:idling_1390467

Conversation

@bfallonf
Copy link

@bfallonf bfallonf commented Dec 2, 2016

As per: https://bugzilla.redhat.com/show_bug.cgi?id=1390467

Added a new "Idling and unidling" file to the dev guide.

@DirectXMan12 would you agree that the developer guide is the correct audience? It does sounds like something the admin would be doing, but it's using the "oc" command.

Also, if there's anything that's incorrect in the PR, please let me know.

@DirectXMan12
Copy link

honestly, I'd put it in the admin guide, and maybe make a note in the dev guide

Copy link

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple of tweaks to wording

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line reads a bit strangely, IMO.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say "you can idle applications fronted by services" (since the oc idle command takes in service names, but the idling action involves the whole "application" (service + dc/rc))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I'd probably call this "Idling Applications" (see comment below).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is true, it's more of "this is useful for bulk-idling applications as part of a script"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... state. This works with both traffic going through services, as well as traffic passed through routes, when using the HAProxy router in it's default configuration. Other routers will need to be configured to pass traffic through the service when they detect idling if they wish to unidle applications.

@bfallonf
Copy link
Author

bfallonf commented Dec 5, 2016

@DirectXMan12 Thanks for the look-over.

I switched it to the Adming Guide, but thought about where there could be a note in the Dev Guide, but came up short. I don't think the use case we're talking about here really fits into that.

I made the rest of your suggestions though. I think it's explaining the process better. Can I get a final ack that I haven't got any of the info wrong though?

Thanks again.

Copy link

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple of small comments. Otherwise, this looks good.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this supposed to say admin-guide-idling-applications?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Good eye!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"as well as scalable resources" (services themselves are also a type of resource ;-) )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"marks it as idled, scaling down" (remove the "by", since we don't want to give the impression that just scaling down a resource will mark it as idled)

@bfallonf
Copy link
Author

bfallonf commented Dec 5, 2016

@DirectXMan12 Thanks for the review again. If there's nothing else, I'll move this along.

@gaurav-nelson @ahardin-rh @adellape Peer review plz?

@bfallonf bfallonf added this to the Next Release milestone Dec 5, 2016
@bfallonf
Copy link
Author

bfallonf commented Dec 6, 2016

[rev_history]
|xref:../admin_guide/idling_applications.adoc#admin-guide-idling-applications[Idling Applications]
|Added the Idling Applications topic.
%

@DirectXMan12
Copy link

LGTM 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S/accosiated/associated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can change 'as well as' to 'and' because of preceding 'both'.

@gaurav-nelson
Copy link
Contributor

minor changes. Rest is all good! 👍

@bfallonf
Copy link
Author

bfallonf commented Dec 7, 2016

Thanks! I'll merge this.

@bfallonf bfallonf merged commit fd3d71d into openshift:master Dec 7, 2016
@bfallonf bfallonf deleted the idling_1390467 branch December 7, 2016 01:46
@adellape adellape modified the milestones: Next Release, Staging Dec 13, 2016
File: diagnostics_tool
Distros: openshift-origin,openshift-enterprise
- Name: Idling Applications
File: idling_applications.adoc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bfallonf Just noticed the File has an .adoc on the end here. Looks like this is building in asciidoctor but not sure how it will fare with Portal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, yea, Portal build immediately failed. Gonna fix this real quick.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xref: #3381

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adellape oops. thanks.

@adellape adellape mentioned this pull request Dec 13, 2016
@ahardin-rh ahardin-rh modified the milestones: Staging, Published - 12/13/16 Dec 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants