Include zone in GCE instance base name

Include zone in GCE instance base name

To prevent ending up with a same-named instance in two different zones,
since Machine Provider expects to key on the combination of backend:hostname.

Bug: 753079
Review-Url: https://codereview.chromium.org/2996623002
Committed: https://github.com/luci/luci-py/commit/623d6fc93ee051a7b7cd9b92670f89280da7ad6e

[smut, 2017-08-07 21:56:32]

smut@google.com changed reviewers:
+ vadimsh@chromium.org

[smut, 2017-08-07 21:56:34]

The instance group manager's name remains the same, but this will ensure the
instances created from two different same-named managers will have different
names.

https://codereview.chromium.org/2996623002/diff/1/appengine/gce-backend/insta...
File appengine/gce-backend/instance_group_managers.py (right):

https://codereview.chromium.org/2996623002/diff/1/appengine/gce-backend/insta...
appengine/gce-backend/instance_group_managers.py:76: )
n.b. This gives us a longest instance name of 42 chars in practice, well below
the limit. But theoretically we need to enforce the limit by rejecting values of
instance_group_manager.key.parent().parent().id() that are too long, which we
should do when validating the config.

[Vadim Sh., 2017-08-07 22:49:02]

1. I'm worried by a lack of any unit test changes.
2. What will happen when we deploy this change? Will it gracefully drain old IGM
and populate new IGM?

[smut, 2017-08-07 23:43:26]

Patchset #2 (id:20001) has been deleted

[smut, 2017-08-07 23:43:45]

On 2017/08/07 22:49:02, Vadim Sh. wrote:
> 1. I'm worried by a lack of any unit test changes.

The unit tests test the internals of the GCE Backend, e.g. ensuring that
database entities are correctly created and updated, and RPCs are only made when
things are in a correct state, in other words that the app is algorithmically
correct. The value returned by get_base_name() is never stored, all this is is a
change to the value passed to one parameter of an external API call, testing it
seemed like overkill until now so it had no unit test. However, I've added an
assertion in the newest patchset.

> 2. What will happen when we deploy this change? Will it gracefully drain old
IGM
> and populate new IGM?

No. Only config changes do that. We will need to make a change in templates.cfg,
which is impending anyway because I need to move these to us-west.

[Vadim Sh., 2017-08-07 23:46:51]

> > 2. What will happen when we deploy this change? Will it gracefully drain old
> IGM
> > and populate new IGM?
> 
> No. Only config changes do that. We will need to make a change in
templates.cfg,
> which is impending anyway because I need to move these to us-west.

But what will happen when we deploy this change? Will everything continue to
work as before until we make a config change? I'm worried it may go "crazy" and
create new IGM group with new name (that includes the zone), "forgetting" about
old one (or something like that).

lgtm assuming you'll verify on -dev that nothing bad is happening

[smut, 2017-08-07 23:49:47]

On 2017/08/07 23:46:51, Vadim Sh. wrote:
> > > 2. What will happen when we deploy this change? Will it gracefully drain
old
> > IGM
> > > and populate new IGM?
> > 
> > No. Only config changes do that. We will need to make a change in
> templates.cfg,
> > which is impending anyway because I need to move these to us-west.
> 
> But what will happen when we deploy this change? Will everything continue to
> work as before until we make a config change? I'm worried it may go "crazy"
and
> create new IGM group with new name (that includes the zone), "forgetting"
about
> old one (or something like that).
> 
> lgtm assuming you'll verify on -dev that nothing bad is happening

Nothing will happen. I didn't make any change to the IGM name. This changes the
"create-new-IGM" RPC. Until we make a config change, GCE Backend will see that
it doesn't need to make a new IGM because IGMs matching the config already
exist. On config change, it will behave as normal except sending a slightly
different parameter to GCE in the "create-new-IGM" RPC.

[smut, 2017-08-07 23:50:20]

The CQ bit was checked by smut@google.com

[commit-bot: I haz the power, 2017-08-07 23:50:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-07 23:52:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1502149820320720,
"parent_rev": "55e6a2a93ab96fb928638aca733ae399eec4d8e0", "commit_rev":
"623d6fc93ee051a7b7cd9b92670f89280da7ad6e"}

[commit-bot: I haz the power, 2017-08-07 23:52:42]

Description was changed from

==========
Include zone in GCE instance base name

To prevent ending up with a same-named instance in two different zones,
since Machine Provider expects to key on the combination of backend:hostname.

Bug: 753079
==========

to

==========
Include zone in GCE instance base name

To prevent ending up with a same-named instance in two different zones,
since Machine Provider expects to key on the combination of backend:hostname.

Bug: 753079
Review-Url: https://codereview.chromium.org/2996623002
Committed:
https://github.com/luci/luci-py/commit/623d6fc93ee051a7b7cd9b92670f89280da7ad6e
==========

[commit-bot: I haz the power, 2017-08-07 23:52:43]

Committed patchset #2 (id:40001) as
https://github.com/luci/luci-py/commit/623d6fc93ee051a7b7cd9b92670f89280da7ad6e

[Vadim Sh., 2017-08-07 23:54:17]

On 2017/08/07 23:49:47, smut wrote:
> On 2017/08/07 23:46:51, Vadim Sh. wrote:
> > > > 2. What will happen when we deploy this change? Will it gracefully drain
> old
> > > IGM
> > > > and populate new IGM?
> > > 
> > > No. Only config changes do that. We will need to make a change in
> > templates.cfg,
> > > which is impending anyway because I need to move these to us-west.
> > 
> > But what will happen when we deploy this change? Will everything continue to
> > work as before until we make a config change? I'm worried it may go "crazy"
> and
> > create new IGM group with new name (that includes the zone), "forgetting"
> about
> > old one (or something like that).
> > 
> > lgtm assuming you'll verify on -dev that nothing bad is happening
> 
> Nothing will happen. I didn't make any change to the IGM name. This changes
the
> "create-new-IGM" RPC. Until we make a config change, GCE Backend will see that
> it doesn't need to make a new IGM because IGMs matching the config already
> exist. On config change, it will behave as normal except sending a slightly
> different parameter to GCE in the "create-new-IGM" RPC.

Thanks for explanation :) Makes sense now.