Refactors GCE backend config updates.

appengine/gce-backend/config.py

 # Copyright 2016 The LUCI Authors. All rights reserved.
 # Use of this source code is governed under the Apache License, Version 2.0
 # that can be found in the LICENSE file.
 
 """Utilities for reading GCE Backend configuration."""
 
 import collections
 import logging
 
 from components import utils
 utils.fix_protobuf_package()
 
 from google import protobuf
 from google.appengine.ext import ndb
 
 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.
   manager_config = ndb.TextProperty()
   # 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.
 
     Returns:
       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
+    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_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,
-  )
+def update_template_configs():
+  """Updates the local template configuration from the config service.
 
-  # Validity of each config.
-  configs = {
-      'managers.cfg': True,
-      'templates.cfg': True,
-  }
+  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})
-
-  if not all(configs.values()):
     return
 
-  stored_config = Configuration.fetch()
-  if stored_config.revision != revision:
-    logging.info('Updating configuration to %s', revision)
+  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.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(
           'zone %s is not unique in template %s.',
           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})