Introduce a device setting controlling allowed connection types for AU.

Introduce a device setting controlling allowed connection types for AU.

Currently, the updater just updates on WiFi and Ethernet and we'd like
to be more flexible in what connection types we allow updates on. This
only introduces the setting (which also allows policy control). A
follow-up change on the Chrome OS side is needed to wire up
update_engine.

BUG=chromium-os:31099
TEST=Compiles and passes tests, no user visible changes (yet).

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=140027

[Mattias Nissler (ping if slow), 2012-05-23 18:57:39]

Here's a first stab at making a device setting for controlling the allowed
connection types for auto-updating. Let me know what you think!

[pastarmovj, 2012-05-23 19:20:40]

LGTM with two small nits/thoughts.

Btw I saw you don't decode the policies in device_settings_provider. I guess you
don't intend it will be used for anything critical in chrome itself.

http://codereview.chromium.org/10388254/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10388254/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:2586: 'name':
'DeviceUpdateScatterFactor',
While you are at it can you please check if all other new device policies have
entries here. Just dumping the idea before I forget it.

http://codereview.chromium.org/10388254/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:2591: 'example_value': 7200,
I know this is lame but we have started using milliseconds for all our time
related policies and I think it is good idea to keep this.

[garnold, 2012-05-23 22:41:43]

1 inline comment. General comments: (a) VPN is a virtual network interface,
which (iiuc) could be running on top of any of the physical interfaces, so I'm
not sure it needs to be included in the list of allowed interfaces;  (b) isn't
WiMAX a flavor of Wifi, either from the physical and/or the usability
standpoint? if so, it doesn't need a separate choice;  (c) this will only enable
to turn updates on/off on a particular medium, ignoring further parameters such
as allowed bandwidth usage, total bytes allowed, etc. I'm not sure how useful it
is for (say) cellular customers with limited data plans.

http://codereview.chromium.org/10388254/diff/8001/chrome/test/functional/poli...
File chrome/test/functional/policy_test_cases.py (right):

http://codereview.chromium.org/10388254/diff/8001/chrome/test/functional/poli...
chrome/test/functional/policy_test_cases.py:277:
'DeviceUpdateAllowedConnectionTypes': (None, [ 'wifi' ], [], ['chromeos']),
Why not [ 'ethernet', 'wifi' ] ?

[Mattias Nissler (ping if slow), 2012-05-24 08:12:31]

http://codereview.chromium.org/10388254/diff/8001/chrome/test/functional/poli...
File chrome/test/functional/policy_test_cases.py (right):

http://codereview.chromium.org/10388254/diff/8001/chrome/test/functional/poli...
chrome/test/functional/policy_test_cases.py:277:
'DeviceUpdateAllowedConnectionTypes': (None, [ 'wifi' ], [], ['chromeos']),
On 2012/05/23 22:41:43, garnold wrote:
> Why not [ 'ethernet', 'wifi' ] ?

This is a sample value to configure, so it doesn't matter what value we specify.
Note that this test only becomes relevant once we have chrome://settings UI, so
at this point, this line is just to satisfy the presubmit check.

[Mattias Nissler (ping if slow), 2012-05-24 08:12:39]

On 2012/05/23 22:41:43, garnold wrote:
> 1 inline comment. General comments: (a) VPN is a virtual network interface,
> which (iiuc) could be running on top of any of the physical interfaces, so I'm
> not sure it needs to be included in the list of allowed interfaces;  

That's a valid point, and I'm fine with just removing it from the list. Then
again, I can see admins saying they don't want to route bandwidth-heavy update
downloads through their VPNs. I modeled this after what level of granularity you
have in
http://code.google.com/searchframe#wZuuyuB8jKQ/src/platform/update_engine/fli...
(which probably chokes on Type=vpn right now).

> (b) isn't
> WiMAX a flavor of Wifi, either from the physical and/or the usability
> standpoint? if so, it doesn't need a separate choice;  

The code referenced above actually does treat WiMax differently from WiFi. Cost
parameters probably also vary usually between WiFi and WiMax, so think it makes
sense to keep them separate.

> (c) this will only enable
> to turn updates on/off on a particular medium, ignoring further parameters
such
> as allowed bandwidth usage, total bytes allowed, etc. I'm not sure how useful
it
> is for (say) cellular customers with limited data plans.

Good point. From the enterprise policy point of view, we can add whatever rules
you like to device settings. Then again, we shouldn't overengineer this IMHO. As
a cellular user without limited data, I would personally rather switch AU over
cellular off and only enable it temporarily if I really need to get that
critical update on the road. Glenn, Sumit, any product-side thoughts on this?

> 
>
http://codereview.chromium.org/10388254/diff/8001/chrome/test/functional/poli...
> File chrome/test/functional/policy_test_cases.py (right):
> 
>
http://codereview.chromium.org/10388254/diff/8001/chrome/test/functional/poli...
> chrome/test/functional/policy_test_cases.py:277:
> 'DeviceUpdateAllowedConnectionTypes': (None, [ 'wifi' ], [], ['chromeos']),
> Why not [ 'ethernet', 'wifi' ] ?

[Mattias Nissler (ping if slow), 2012-05-25 13:58:07]

http://codereview.chromium.org/10388254/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10388254/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:2586: 'name':
'DeviceUpdateScatterFactor',
On 2012/05/23 19:20:40, pastarmovj wrote:
> While you are at it can you please check if all other new device policies have
> entries here. Just dumping the idea before I forget it.

I checked, and all are fine (except for special cases such as CameraEnabled).

http://codereview.chromium.org/10388254/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:2591: 'example_value': 7200,
On 2012/05/23 19:20:40, pastarmovj wrote:
> I know this is lame but we have started using milliseconds for all our time
> related policies and I think it is good idea to keep this.

I actually didn't introduce the field, so you have to take this to jaysri@.

[Mattias Nissler (ping if slow), 2012-05-29 14:15:03]

Gilad, Don, are you OK to proceed with this version?

[garnold, 2012-05-29 19:55:22]

I'm okay with this. Looks like we've omitted the VPN checkbox entirely---sounds
fine, too, we can always add it later if deemed necessary. Minor gripes inline.
Thanks!

http://codereview.chromium.org/10388254/diff/16001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10388254/diff/16001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:2594: 'desc': '''Specifies the number of
seconds up to which a device may randomly delay its download of an update from
the time the update was first pushed out to the server. The device may wait a
portion of this time in terms of wall-clock-time and the remaining portion in
terms of the number of update checks. In any case, the scatter is upper bounded
to a constant amount of time so that a device does not ever get stuck waiting to
download an update forever.'''
(a) "and the remaining portion in terms of the number of update checks"---I'm
not sure what that means...?  (b) "upper bounded to a constant amount of
time"---s/to/by/ ?  what is this constant? Doesn't the integer value
corresponding to this property represent that upper bound? And why would a
device wait forever?

http://codereview.chromium.org/10388254/diff/16001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:2605: 'desc': ''' The types of
connections that are OK to run updates over. OS updates potentially put heavy
strain on the connection due to their size and may incur additional cost.
Therefore, they are by default not enabled for connection types that are
considered expensive, which include WiMax, Bluetooth and Cellular at the moment.
(a) "are OK to run updates over" --> "are OK to use for updating ChromeOS" ? 
(b) "may incur additional cost"---what cost?  Perhaps it's enough to say
"consume substantial bandwidth" or something along those lines?  (c) "considered
expensive" --> "have limited bandwidth or bounded capacity"

http://codereview.chromium.org/10388254/diff/16001/chrome/test/functional/pol...
File chrome/test/functional/policy_test_cases.py (right):

http://codereview.chromium.org/10388254/diff/16001/chrome/test/functional/pol...
chrome/test/functional/policy_test_cases.py:277:
'DeviceUpdateAllowedConnectionTypes': (None, [ 'wifi' ], [], ['chromeos']),
Why aren't we enable ethernet by default?

[Mattias Nissler (ping if slow), 2012-05-31 11:24:44]

+Jay for some of Gilad's comments.

New version is up. Good to go?

http://codereview.chromium.org/10388254/diff/16001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10388254/diff/16001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:2594: 'desc': '''Specifies the number of
seconds up to which a device may randomly delay its download of an update from
the time the update was first pushed out to the server. The device may wait a
portion of this time in terms of wall-clock-time and the remaining portion in
terms of the number of update checks. In any case, the scatter is upper bounded
to a constant amount of time so that a device does not ever get stuck waiting to
download an update forever.'''
On 2012/05/29 19:55:22, garnold wrote:
> (a) "and the remaining portion in terms of the number of update checks"---I'm
> not sure what that means...?  (b) "upper bounded to a constant amount of
> time"---s/to/by/ ?  what is this constant? Doesn't the integer value
> corresponding to this property represent that upper bound? And why would a
> device wait forever?

This is actually a copy of the comment in
http://git.chromium.org/gitweb/?p=chromium/src.git;a=blob;f=chrome/browser/po...
that I just added here in order to be able to have the policy show up in
about:policy. 

Regarding your questions:

(a) Update checks don't necessarily result in an immediate download, but might
re-check at a later point.
(b) It should be "by" and the constant is the value of this policy. The point
regarding waiting forever is about preventing that condition: If we were to
throw the dice at each update check, there wouldn't be a deterministic upper
bound (assuming we always roll the dice again before actually downloading).

Does this answer your questions? If you want changes to the text, let's discuss
with Jay and make the changes in a separate CL.

http://codereview.chromium.org/10388254/diff/16001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:2605: 'desc': ''' The types of
connections that are OK to run updates over. OS updates potentially put heavy
strain on the connection due to their size and may incur additional cost.
Therefore, they are by default not enabled for connection types that are
considered expensive, which include WiMax, Bluetooth and Cellular at the moment.
On 2012/05/29 19:55:22, garnold wrote:
> (a) "are OK to run updates over" --> "are OK to use for updating ChromeOS" ? 
> (b) "may incur additional cost"---what cost?  Perhaps it's enough to say
> "consume substantial bandwidth" or something along those lines?  (c)
"considered
> expensive" --> "have limited bandwidth or bounded capacity"

(a) done
(b) 3G data plans cost actual dollars. The same may or may not apply to WiMax.
(c) "considered expensive" is in reference to
http://git.chromium.org/gitweb/?p=chromiumos/platform/update_engine.git;a=blo...
which I'd like stay consistent with.

http://codereview.chromium.org/10388254/diff/16001/chrome/test/functional/pol...
File chrome/test/functional/policy_test_cases.py (right):

http://codereview.chromium.org/10388254/diff/16001/chrome/test/functional/pol...
chrome/test/functional/policy_test_cases.py:277:
'DeviceUpdateAllowedConnectionTypes': (None, [ 'wifi' ], [], ['chromeos']),
On 2012/05/29 19:55:22, garnold wrote:
> Why aren't we enable ethernet by default?

This is not a default, and this line is just here to make the presubmit script
happy. This test is meant to test chrome://settings UI, which is not applicable
to these two policies at the moment. I've no removed the 'wifi' now as well in
order to avoid confusion.

[garnold, 2012-06-01 00:28:14]

+1 (how to mark it on the CL page?)

http://codereview.chromium.org/10388254/diff/16001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10388254/diff/16001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:2594: 'desc': '''Specifies the number of
seconds up to which a device may randomly delay its download of an update from
the time the update was first pushed out to the server. The device may wait a
portion of this time in terms of wall-clock-time and the remaining portion in
terms of the number of update checks. In any case, the scatter is upper bounded
to a constant amount of time so that a device does not ever get stuck waiting to
download an update forever.'''
On 2012/05/31 11:24:45, Mattias Nissler wrote:
> (a) Update checks don't necessarily result in an immediate download, but might
> re-check at a later point.

I still don't understand what it means. It may be helpful to simplify this text.
Perhaps "in terms of" is what's confusing me---?

> (b) It should be "by" and the constant is the value of this policy. The point
> regarding waiting forever is about preventing that condition: If we were to
> throw the dice at each update check, there wouldn't be a deterministic upper
> bound (assuming we always roll the dice again before actually downloading).

If the upper bound == the number of seconds specified by
DeviceUpdateScatterFactor, then the meaning of the last sentence is the same as
that of the first sentence, only vaguer, in which case it should be removed. 

> If you want changes to the text, let's discuss
> with Jay and make the changes in a separate CL.

Cool.

[Mattias Nissler (ping if slow), 2012-06-01 13:58:38]

Julian, I fixed this. Still looks good?

[pastarmovj, 2012-06-01 14:04:58]

Still Good. LGTM.