Refactors GCE backend config updates.

appengine/gce-backend/config.py

Index: appengine/gce-backend/config.py
diff --git a/appengine/gce-backend/config.py b/appengine/gce-backend/config.py
index 8294119f13c64a5eb34b5491adf30fcbec662be1..745e564c62586b1a509b8079f455cc9ce559c37c 100644
--- a/appengine/gce-backend/config.py
+++ b/appengine/gce-backend/config.py
@@ -17,14 +17,17 @@ import metrics
 
 from components import config
 from components import datastore_utils
+from components.config import validation
 
 from proto import config_pb2
 
 
+TEMPLATES_CFG_FILENAME = 'templates.cfg'
+MANAGERS_CFG_FILENAME = 'managers.cfg'
+
+
 class Configuration(datastore_utils.config.GlobalConfig):
   """Configuration for this service."""
-  # Name of the config set in the config service.
-  config_set = ndb.StringProperty()
   # Text-formatted proto.config_pb2.InstanceTemplateConfig.
   template_config = ndb.TextProperty()
   # Text-formatted proto.config_pb2.InstanceGroupManagerConfig.
@@ -32,6 +35,12 @@ class Configuration(datastore_utils.config.GlobalConfig):
   # Revision of the configs.
   revision = ndb.StringProperty()
 
+  def set_defaults(self):
+    """Sets defaults for Configuration entity."""
+    self.template_config = ''

[M-A Ruel, 2016/08/12 18:50:25]

why not use default='' on each property?

[ryanmartens, 2016/08/12 21:02:49]

Done.

+    self.manager_config = ''
+    self.revision = ''
+
   @classmethod
   def load(cls):
     """Loads text-formatted template and manager configs into message.Messages.
@@ -40,79 +49,80 @@ class Configuration(datastore_utils.config.GlobalConfig):
       A 2-tuple of (InstanceTemplateConfig, InstanceGroupManagerConfig).
     """
     configuration = cls.cached()
-    template_config = config_pb2.InstanceTemplateConfig()
-    protobuf.text_format.Merge(configuration.template_config, template_config)
-    manager_config = config_pb2.InstanceGroupManagerConfig()
-    protobuf.text_format.Merge(configuration.manager_config, manager_config)
-    return template_config, manager_config
-
-
-def update_config():
-  """Updates the local configuration from the config service."""
-  config_set = Configuration.cached().config_set
-  revision, template_config = config.get(
-      config_set,
-      'templates.cfg',
-      dest_type=config_pb2.InstanceTemplateConfig,
-  )
-  _, manager_config = config.get(
-      config_set,
-      'managers.cfg',
-      dest_type=config_pb2.InstanceGroupManagerConfig,
-      revision=revision,
-  )
-
-  # Validity of each config.
-  configs = {
-      'managers.cfg': True,
-      'templates.cfg': True,
-  }
+    template_cfg = config_pb2.InstanceTemplateConfig()
+    if configuration.template_config:
+      protobuf.text_format.Merge(configuration.template_config, template_cfg)

[M-A Ruel, 2016/08/12 18:50:25]

do you want to trap protobuf.text_format.ParseError and convert to ValueError?
That's what auth_service does.

[ryanmartens, 2016/08/12 21:02:48]

Sure. I've now loosely followed auth_service. It'd be nice to unify the two in
the future by adding an ability in the config component to fetch a set of config
files to ensure they are all in sync. Thoughts?

+    manager_cfg = config_pb2.InstanceGroupManagerConfig()
+    if configuration.manager_config:
+      protobuf.text_format.Merge(configuration.manager_config, manager_cfg)
+    return template_cfg, manager_cfg
+
+
+def update_template_configs():
+  """Updates the local template configuration from the config service.
+
+  Ensures that all config files are at the same path revision.
+  """
+  template_revision, template_config = config.get_self_config(
+    TEMPLATES_CFG_FILENAME, config_pb2.InstanceTemplateConfig,

[M-A Ruel, 2016/08/12 18:50:25]

align arguments at +4

[ryanmartens, 2016/08/12 21:02:48]

Done.

+    store_last_good=True)
+  manager_revision, manager_config = config.get_self_config(
+    MANAGERS_CFG_FILENAME, config_pb2.InstanceGroupManagerConfig,
+    store_last_good=True)
 
   context = config.validation_context.Context.logging()
-  validate_template_config(template_config, context)
+  if template_config:
+    validate_template_config(template_config, context)
   if context.result().has_errors:
     logging.error('Not updating configuration due to errors in templates.cfg')
-    configs['templates.cfg'] = False
+    return
 
   context = config.validation_context.Context.logging()
-  validate_manager_config(manager_config, context)
+  if manager_config:
+    validate_manager_config(manager_config, context)
   if context.result().has_errors:
     logging.error('Not updating configuration due to errors in managers.cfg')
-    configs['managers.cfg'] = False
-
-  for config_name, valid in configs.iteritems():
-    metrics.config_valid.set(valid, fields={'config': config_name})
+    return
 
-  if not all(configs.values()):
+  if template_revision != manager_revision:

[M-A Ruel, 2016/08/12 18:50:25]

I think this check should be done first (?)

[ryanmartens, 2016/08/12 21:02:48]

Done.

+    logging.error('Not updating configuration due to revision mismatch.')

[M-A Ruel, 2016/08/12 18:50:25]

warning?

[ryanmartens, 2016/08/12 21:02:48]

Done.

     return
 
-  stored_config = Configuration.fetch()
-  if stored_config.revision != revision:
-    logging.info('Updating configuration to %s', revision)
+  stored_config = Configuration.cached()
+  if stored_config.revision != template_revision:

[M-A Ruel, 2016/08/12 18:50:25]

No need to validate at every cron job when the stored version is the same (?)

[ryanmartens, 2016/08/12 21:02:48]

Done.

+    logging.info('Updating configuration to %s', template_revision)
     stored_config.modify(
-      manager_config=protobuf.text_format.MessageToString(manager_config),
-      revision=revision,
-      template_config=protobuf.text_format.MessageToString(template_config),
+      manager_config = protobuf.text_format.MessageToString(manager_config),

[M-A Ruel, 2016/08/12 18:50:25]

align at +4 while at it

[ryanmartens, 2016/08/12 21:02:48]

Done.

+      revision = template_revision,
+      template_config = protobuf.text_format.MessageToString(template_config),
     )
 
 
+@validation.self_rule(TEMPLATES_CFG_FILENAME, config_pb2.InstanceTemplateConfig)
 def validate_template_config(config, context):
   """Validates an InstanceTemplateConfig instance."""
   # We don't do any GCE-specific validation here. Just require globally
   # unique base name because base name is used as the key in the datastore.
   base_names = set()
+  valid = True
   for template in config.templates:
     if template.base_name in base_names:
       context.error('base_name %s is not globally unique.', template.base_name)
+      valid = False
     else:
       base_names.add(template.base_name)
 
+  metrics.config_valid.set(valid, fields={'config': TEMPLATES_CFG_FILENAME})
 
+
+@validation.self_rule(MANAGERS_CFG_FILENAME,

[M-A Ruel, 2016/08/12 18:50:25]

@validation.self_rule(
    MANAGERS_CFG_FILENAME, config_pb2.InstanceGroupManagerConfig)

[ryanmartens, 2016/08/12 21:02:49]

Done.

+  config_pb2.InstanceGroupManagerConfig)
 def validate_manager_config(config, context):
   """Validates an InstanceGroupManagerConfig instance."""
   # We don't do any GCE-specific validation here. Just require per-template
   # unique zone because template+zone is used as a key in the datastore.
   zones = collections.defaultdict(set)
+  valid = True
   for manager in config.managers:
     if manager.zone in zones[manager.template_base_name]:
       context.error(
@@ -120,5 +130,8 @@ def validate_manager_config(config, context):
           manager.zone,
           manager.template_base_name,
       )
+      valid = False
     else:
       zones[manager.template_base_name].add(manager.zone)
+
+  metrics.config_valid.set(valid, fields={'config': MANAGERS_CFG_FILENAME})