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.

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

Initial version of GCE timeout sample #1

Merged
merged 27 commits into from Dec 31, 2012

Conversation

Projects
None yet
4 participants
@briandorsey
Contributor

briandorsey commented Dec 27, 2012

Compute folks, please review.

README.md Outdated
------

> As long as PRETEND_MODE is set to `True` (the default) in `main.py`, the application will only log deletes.

This comment has been minimized.

@fredsa

fredsa Dec 28, 2012

Contributor

Suggest DRY_RUN instead of PRETEND_MODE

main.py Outdated
SAMPLE_NAME = 'Instance timeout helper'
# Be careful, this application will delete instances unless they're tagged
# with one of the SAFE_TAGS below.
GCE_PROJECT_ID = 'briandpe-api'

This comment has been minimized.

@fredsa

fredsa Dec 28, 2012

Contributor

perhaps this should be 'replace-with-your-compute-engine-project-id'

app.yaml Outdated
@@ -0,0 +1,21 @@
application: REPLACE-WITH-YOUR-APP-ID

This comment has been minimized.

@fredsa

fredsa Dec 28, 2012

Contributor

Suggest 'replace-with-your-app-id' to reinforce the fact that app ids are lower case

app.yaml Outdated

libraries:
- name: webapp2
version: "2.5.2"

This comment has been minimized.

@fredsa

fredsa Dec 28, 2012

Contributor

using 'latest' here might be better for a sample app

cron.yaml Outdated
cron:
- description: check for expired Compute Engine instances to delete
url: /cron/delete
schedule: every 1 hours from 00:55 to 23:59

This comment has been minimized.

@fredsa

fredsa Dec 28, 2012

Contributor

Can we make start/end times less random?

main.py Outdated
creation = parse_iso8601tz(instance['creationTimestamp'])
now = datetime.datetime.now()
delta = now - creation
instance['_age'] = delta.seconds / 60

This comment has been minimized.

@fredsa

fredsa Dec 28, 2012

Contributor

suggest _age_hours instead

main.py Outdated
SAFE_TAGS = "production safetag".lower().split()
# In pretend mode, deletes are only logged. Set this to False after you've
# double-checked the status page and you're ready to enable the deletes.
PRETEND_MODE = True

This comment has been minimized.

@fredsa

fredsa Dec 28, 2012

Contributor

Recommend moving all these config items into settings.py and then pulling them in via google.appengine.api.lib_config:
https://developers.google.com/appengine/docs/python/tools/appengineconfig?hl=en#Configuring_Your_Own_Modules

This comment has been minimized.

@briandorsey

briandorsey Dec 28, 2012

Contributor

Thanks for the pointer to lib_config. It's very nice.

I tried a version with lib_config (08c0ab9) , but I think the extra layer of abstraction is a bit too much for a sample. Also, it becomes attractive to change the defaults right in main.py, but have them overwritten by the values in settings.py.

This comment has been minimized.

@fredsa

fredsa Dec 29, 2012

Contributor

You could have None as the default in main.py, and explicitly put defaults
in settings.py only.

Fred on my Android
On Dec 28, 2012 3:39 PM, "Brian Dorsey" notifications@github.com wrote:

In main.py:

+import jinja2
+import webapp2
+from apiclient.discovery import build
+from google.appengine.api import memcache
+from oauth2client.appengine import AppAssertionCredentials
+
+SAMPLE_NAME = 'Instance timeout helper'
+# Be careful, this application will delete instances unless they're tagged
+# with one of the SAFE_TAGS below.
+GCE_PROJECT_ID = 'briandpe-api'
+TIMEOUT = 60 * 8 # in minutes, defaulting to 8 hours
+SAFE_TAGS = "production safetag".lower().split()
+# In pretend mode, deletes are only logged. Set this to False after you've
+# double-checked the status page and you're ready to enable the deletes.
+PRETEND_MODE = True

Thanks for the pointer to lib_config. It's very nice.

I tried a version with lib_config (08c0ab908c0ab9)
, but I think the extra layer of abstraction is a bit too much for a
sample. Also, it becomes attractive to change the defaults right in
main.py, but have them overwritten by the values in settings.py.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1/files#r2518886.

main.py Outdated
def get(self):
instances = list_instances()

config = {}
main.py Outdated


app = webapp2.WSGIApplication([
('/', MainHandler),

This comment has been minimized.

@fredsa

fredsa Dec 28, 2012

Contributor

recommend putting '/' after the more specific handlers

main.py Outdated
scope='https://www.googleapis.com/auth/compute')
HTTP = credentials.authorize(httplib2.Http(memcache))

compute = build('compute', 'v1beta13', http=HTTP)

This comment has been minimized.

@fredsa

fredsa Dec 28, 2012

Contributor

Can you explain v1beta13 in a comment?

README.md Outdated
$ git clone REPOSITORY-URL

Install [Goolge API Python Client with dependencies for App Engine](http://code.google.com/p/google-api-python-client/downloads/list).

This comment has been minimized.

@dhermes

dhermes Dec 28, 2012

Typo "Goolge API"

index.yaml Outdated
@@ -0,0 +1,12 @@
indexes:

This comment has been minimized.

@dhermes

dhermes Dec 28, 2012

Does this really need to be included if no indexes are created?

main.py Outdated
GCE_PROJECT_ID = 'replace-with-your-compute-engine-project-id'
TIMEOUT = 60 * 8 # in minutes, defaulting to 8 hours
SAFE_TAGS = "production safetag".lower().split()
# In pretend mode, deletes are only logged. Set this to False after you've

This comment has been minimized.

@dhermes

dhermes Dec 28, 2012

Should change this comment "In pretend mode" to reflect the change in the constant name to DRY_RUN

main.py Outdated
# filter instances, keep only expired instances
instances = [i for i in instances if i['_timeout_expired']]

logging.info("delete cron: %s instance%s to delete",

This comment has been minimized.

@dhermes

dhermes Dec 28, 2012

Inconsistent use of single quote and double quote.

main.py Outdated
# parse the timezone offset separately
delta = datetime.timedelta(minutes=int(date_string[-2:]),
hours=int(date_string[-5:-3]))
if date_string[-6:-5] == u'-':

This comment has been minimized.

@dhermes

dhermes Dec 28, 2012

You don't need a slice here, nor a unicode u'-' for comparison.

@dhermes

This comment has been minimized.

dhermes commented on main.py in 08c0ab9 Dec 28, 2012

Using lowercase as global is a bit scary, especially because you end up using it in MainHandler.get

briandorsey added some commits Dec 28, 2012

convert from lib_config to a config dictionary
lib_config is nice, but I think the extra layer of abstraction is too
much for a sample. Also, it's too attractive to change the defaults right
in main.py, but have them overwritten by the values in settings.py.

@ghost ghost assigned marcacohen Dec 28, 2012

main.py Outdated
# double-checked the status page and you're ready to enable the deletes.
'DRY_RUN': True,

# Be careful, this application will delete all instances in the project

This comment has been minimized.

@marcacohen

marcacohen Dec 30, 2012

Contributor

This comment seems to apply to the next dict key (SAFE_TAGS). Perhaps this should be moved to be adjacent to that def and a comment could be added here re: where to find the project id?


# Build object for the 'v1beta13' version of the GCE API.
# https://developers.google.com/compute/docs/reference/v1beta13/
compute = build('compute', 'v1beta13', http=HTTP)

This comment has been minimized.

@marcacohen

marcacohen Dec 30, 2012

Contributor

The API version is likely to change often. Should that be pulled out into a separate constant for easy maintenance?

This comment has been minimized.

@briandorsey

briandorsey Dec 31, 2012

Contributor

This is a good idea... I'd like to take it bit further and say that we should have a variable name we agree to use in all samples, so it's easy to update them all at once. Something like GCE_API_VERSION? I'll make a separate proposal about this.

main.py Outdated
if tag.lower() in CONFIG['SAFE_TAGS']:
excluded = True
break
instance['_excluded'] = excluded

This comment has been minimized.

@marcacohen

marcacohen Dec 30, 2012

Contributor

I've seen the underscore convention for instance variables but not for dictionary keys. Is that a standard, or just a personal preference? Not a problem either way, just curious.

This comment has been minimized.

@briandorsey

briandorsey Dec 31, 2012

Contributor

It's not a standard. Or even personal preference in general... but in this case I wanted it to be obvious which keys were added by the sample when looking at the overview web page. Could also use a sample-specific prefix, I suppose.

main.py Outdated
now = datetime.datetime.now()
delta = now - creation
instance['_age_minutes'] = delta.seconds / 60
if delta.seconds > CONFIG['TIMEOUT'] * 60 and not instance['_excluded']:

This comment has been minimized.

@marcacohen

marcacohen Dec 30, 2012

Contributor

This might just be me but I prefer parenthesizing (CONFIG['TIMEOUT'] * 60), not because it's needed (it's not) but because I find it more readabie.

This comment has been minimized.

@marcacohen

marcacohen Dec 30, 2012

Contributor

Consider reversing and operands to avoid doing calculation on excluded instances?

briandorsey added a commit that referenced this pull request Dec 31, 2012

Merge pull request #1 from GoogleCloudPlatform/initial
Initial version of GCE timeout sample

@briandorsey briandorsey merged commit b59a5ad into master Dec 31, 2012

@briandorsey briandorsey deleted the initial branch Dec 31, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment