Revert "autoupdate.py: Allow max_updates through update URL"
This reverts commit aef2b297549b3b2de1c67de16a8433bb9419ac34.
Reason for revert: <We didn't need it. We could solve it other ways. Done in: https://chromium-review.googlesource.com/c/chromiumos/third_party/autotest/+/2057929>
Original change's description:
> autoupdate.py: Allow max_updates through update URL
>
> Currently in order to put a ceiling on the number of updates that can be
> performed in a devserver, we need to spawn a new devserver with flag
> --max_updates. The problem is now for each run of an autotest, they
> should spwan a new devserver alongside the lab devservers which is not
> ideal.
>
> We solve this problem by dynamically configuring the devserver
> using a unique identifier. This is done by calling into 'session_id' API
> of the devserver. Then clients can send their requests appending this
> session_id to be responded likewise.
>
> For maximum updates, we first configure the devserver to set a
> 'max_updates' data for a unique session ID. Then client can send
> requests using the session ID as a query string be capped on the number
> of updates they get.
>
> BUG=chromium:1004489
> TEST=autoupdate_unittest.py
> TEST=devserver_integration_test.py
>
> Change-Id: Ieef921b177ba0ec789d6471a34a4f8e44f5482af
> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/dev-util/+/1996148
> Tested-by: Amin Hassani <ahassani@chromium.org>
> Commit-Queue: Amin Hassani <ahassani@chromium.org>
> Reviewed-by: Allen Li <ayatane@chromium.org>
Bug: chromium:1004489
Change-Id: I686a4e18ed69b5516896791cf667029fd9b2e3cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/dev-util/+/2067609
Reviewed-by: Amin Hassani <ahassani@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
diff --git a/autoupdate.py b/autoupdate.py
index 6f9e83a..957bcc9 100644
--- a/autoupdate.py
+++ b/autoupdate.py
@@ -7,9 +7,6 @@
from __future__ import print_function
-import collections
-import contextlib
-import datetime
import json
import os
import threading
@@ -116,92 +113,6 @@
return self.table.get(host_id)
-class SessionTable(object):
- """A class to keep a map of session IDs and data.
-
- This can be used to set some configuration related to a session and
- retrieve/manipulate the configuration whenever needed. This is basically a map
- of string to a dict object.
- """
-
- SESSION_EXPIRATION_TIMEDIFF = datetime.timedelta(hours=1)
- OCCASIONAL_PURGE_TIMEDIFF = datetime.timedelta(hours=1)
-
- Session = collections.namedtuple('Session', ['timestamp', 'data'])
-
- def __init__(self):
- """Initializes the SessionTable class."""
- self._table = {}
- # Since multiple requests might come for this session table by multiple
- # threads, keep it under a lock.
- self._lock = threading.Lock()
- self._last_purge_time = datetime.datetime.now()
-
- def _ShouldPurge(self):
- """Returns whether its time to do an occasional purge."""
- return (datetime.datetime.now() - self._last_purge_time >
- self.OCCASIONAL_PURGE_TIMEDIFF)
-
- def _IsSessionExpired(self, session):
- """Returns whether a session needs to be purged.
-
- Args:
- session: A unique identifer string for a session.
- """
- return (datetime.datetime.now() - session.timestamp >
- self.SESSION_EXPIRATION_TIMEDIFF)
-
- def _Purge(self):
- """Cleans up entries that have been here long enough.
-
- This is so the memory usage of devserver doesn't get bloated.
- """
- # Try to purge once every hour or so.
- if not self._ShouldPurge():
- return
-
- # Purge the ones not in use.
- self._table = {k: v for k, v in self._table.items()
- if not self._IsSessionExpired(v)}
-
- def SetSessionData(self, session, data):
- """Sets data for the given a session ID.
-
- Args:
- session: A unique identifier string.
- data: A data to set for this session ID.
- """
- if not session or data is None:
- return
-
- with self._lock:
- self._Purge()
-
- if self._table.get(session) is not None:
- _Log('Replacing an existing session %s', session)
- self._table[session] = SessionTable.Session(datetime.datetime.now(), data)
-
- @contextlib.contextmanager
- def SessionData(self, session):
- """Returns the session data for manipulation.
-
- Args:
- session: A unique identifier string.
- """
- # Cherrypy has multiple threads and this data structure is global, so lock
- # it to restrict simultaneous access by multiple threads.
- with self._lock:
- session_value = self._table.get(session)
- # If not in the table, just assume it wasn't supposed to be.
- if session_value is None:
- yield {}
- else:
- # To update the timestamp.
- self._table[session] = SessionTable.Session(datetime.datetime.now(),
- session_value.data)
- yield session_value.data
-
-
class Autoupdate(build_util.BuildObject):
"""Class that contains functionality that handles Chrome OS update pings."""
@@ -235,8 +146,6 @@
# host, as well as a dictionary of current attributes derived from events.
self.host_infos = HostInfoTable()
- self._session_table = SessionTable()
-
self._update_count_lock = threading.Lock()
def GetUpdateForLabel(self, label):
@@ -410,32 +319,20 @@
request = nebraska.Request(data)
self._LogRequest(request)
- session = kwargs.get('session')
- _Log('Requested session is: %s', session)
-
if request.request_type == nebraska.Request.RequestType.EVENT:
if (request.app_requests[0].event_type ==
nebraska.Request.EVENT_TYPE_UPDATE_DOWNLOAD_STARTED and
request.app_requests[0].event_result ==
nebraska.Request.EVENT_RESULT_SUCCESS):
- err_msg = ('Received too many download_started notifications. This '
- 'probably means a bug in the test environment, such as too '
- 'many clients running concurrently. Alternatively, it could '
- 'be a bug in the update client.')
-
with self._update_count_lock:
if self.max_updates == 0:
- _Log(err_msg)
+ _Log('Received too many download_started notifications. This '
+ 'probably means a bug in the test environment, such as too '
+ 'many clients running concurrently. Alternatively, it could '
+ 'be a bug in the update client.')
elif self.max_updates > 0:
self.max_updates -= 1
- with self._session_table.SessionData(session) as session_data:
- value = session_data.get('max_updates')
- if value is not None:
- session_data['max_updates'] = max(value - 1, 0)
- if value == 0:
- _Log(err_msg)
-
_Log('A non-update event notification received. Returning an ack.')
return nebraska.Nebraska().GetResponseToRequest(
request, response_props=nebraska.ResponseProperties(**kwargs))
@@ -444,11 +341,7 @@
# responses. Note that the counter is only decremented when the client
# reports an actual download, to avoid race conditions between concurrent
# update requests from the same client due to a timeout.
- max_updates = None
- with self._session_table.SessionData(session) as session_data:
- max_updates = session_data.get('max_updates')
-
- if self.max_updates == 0 or max_updates == 0:
+ if self.max_updates == 0:
_Log('Request received but max number of updates already served.')
kwargs['no_update'] = True
# Override the noupdate to make sure the response is noupdate.
@@ -497,12 +390,3 @@
# If no events were logged for this IP, return an empty log.
return json.dumps([])
-
- def SetSessionData(self, session, data):
- """Sets the session ID for the current run.
-
- Args:
- session: A unique identifier string.
- data: A dictionary containing some data.
- """
- self._session_table.SetSessionData(session, data)
diff --git a/autoupdate_unittest.py b/autoupdate_unittest.py
index 94db2b4..648ef7c 100755
--- a/autoupdate_unittest.py
+++ b/autoupdate_unittest.py
@@ -111,69 +111,5 @@
self.assertIn('error-unknownApplication', au_mock.HandleUpdatePing(request))
-
-class MaxUpdatesTableTest(unittest.TestCase):
- """Tests MaxUpdatesTable"""
-
- def testSessionTable(self):
- """Tests that SessionData() method correctly returns requested data."""
- table = autoupdate.SessionTable()
- g_data = {'foo': 0}
-
- table.SetSessionData('id-1', g_data)
- with table.SessionData('id-1') as data:
- data.update({'foo': data.get('foo') + 1})
- # Value of the global data should be increased by now.
- self.assertTrue(g_data['foo'], 1)
-
- # Increase again.
- with table.SessionData('id-1') as data:
- data.update({'foo': data.get('foo') + 1})
- self.assertTrue(g_data['foo'], 2)
-
- # Make sure multiple sessions can be set and used.
- g_data2 = {'foo': 10}
- table.SetSessionData('id-2', g_data2)
- with table.SessionData('id-2') as data:
- data.update({'foo': data.get('foo') + 1})
- self.assertTrue(g_data2['foo'], 11)
-
- def testNoneSession(self):
- """Tests if a session is not set, it should be returned as None."""
- table = autoupdate.SessionTable()
- # A session ID that has never been set should not return anything.
- with table.SessionData('foo-id') as data:
- self.assertDictEqual(data, {})
-
- def testOverrideSession(self):
- """Tests that a session can be overriden.."""
- table = autoupdate.SessionTable()
-
- table.SetSessionData('id-1', {'foo': 0})
- table.SetSessionData('id-1', {'bar': 1})
- with table.SessionData('id-1') as data:
- self.assertEqual(data.get('bar'), 1)
-
- @mock.patch.object(autoupdate.SessionTable, '_IsSessionExpired',
- side_effect=lambda s: 'foo' in s.data)
- @mock.patch.object(autoupdate.SessionTable, '_ShouldPurge',
- return_value=True)
- # pylint: disable=unused-argument
- def testPurge(self, p, ps):
- """Tests that data is being purged correctly."""
- table = autoupdate.SessionTable()
-
- table.SetSessionData('id-1', {'foo': 1})
- table.SetSessionData('id-2', {'bar': 2})
-
- # Set a random session to make _Purge() be called.
- table.SetSessionData('blah', {'blah': 1})
- # Only id-1 should be purged by now.
- with table.SessionData('id-1') as data:
- self.assertDictEqual(data, {})
- with table.SessionData('id-2') as data:
- self.assertDictEqual(data, {'bar': 2})
-
-
if __name__ == '__main__':
unittest.main()
diff --git a/devserver.py b/devserver.py
index 0bdc787..72dc9e2 100755
--- a/devserver.py
+++ b/devserver.py
@@ -1562,29 +1562,9 @@
label = '/'.join(args)
body_length = int(cherrypy.request.headers.get('Content-Length', 0))
data = cherrypy.request.rfile.read(body_length)
+
return updater.HandleUpdatePing(data, label, **kwargs)
- @cherrypy.expose
- def session(self, *args):
- """Adds a new Session for a unique session ID with POST body as data.
-
- Calling this API establishes a configuration by keeping a piece of data (in
- JSON format) for this specific session ID. Users can later send requests to
- devserver identifying this session ID to have different responses. The
- session ID is a unique identifier string (can be in any format)
-
- To use, call this API like:
- curl -X POST -d '{"foo": "bar"}' \
- http://127.0.0.1:8080/session/some-random-string
-
- The users of this API will send the session ID as a query string with any
- other parameters they like:
-
- curl http://127.0.0.1:8080/some-api?session=some-random-string
- """
- content_length = int(cherrypy.request.headers.get('Content-Length', 0))
- content = cherrypy.request.rfile.read(content_length)
- return updater.SetSessionData(args[0], json.loads(content))
def _CleanCache(cache_dir, wipe):
"""Wipes any excess cached items in the cache_dir.
@@ -1620,7 +1600,6 @@
group.add_option('--host_log',
action='store_true', default=False,
help='record history of host update events (/api/hostlog)')
- # TODO(crbug/1004489): Deperecate max_updates.
group.add_option('--max_updates',
metavar='NUM', default=-1, type='int',
help='maximum number of update checks handled positively '
diff --git a/devserver_integration_test.py b/devserver_integration_test.py
index c1846e3..fbde817 100755
--- a/devserver_integration_test.py
+++ b/devserver_integration_test.py
@@ -56,14 +56,6 @@
</app>
</request>
""")
-DOWNLOAD_STARTED_REQUEST = Template("""<?xml version="1.0" encoding="UTF-8"?>
-<request protocol="3.0" updater="ChromeOSUpdateEngine" updaterversion="0.1.0.0" ismachine="1">
- <os version="Indy" platform="Chrome OS" sp="0.11.254.2011_03_09_1814_i686"></os>
- <app appid="$appid" version="11.254.2011_03_09_1814" lang="en-US" track="developer-build" board="x86-generic" hardware_class="BETA DVT" delta_okay="true">
- <event eventtype="13" eventresult="1"></event>
- </app>
-</request>
-""")
# RPC constants.
STAGE = 'stage'
@@ -410,38 +402,6 @@
connection.close()
self.assertEqual(update_data, contents)
- def testMaxUpdates(self):
- """Tests MaxUpdates functionality of autoupdate."""
- update_label = '/'.join([UPDATE, LABEL])
- update_request = UPDATE_REQUEST.substitute({'appid': '{DEV-BUILD}'})
- download_started_request = DOWNLOAD_STARTED_REQUEST.substitute(
- {'appid': '{DEV-BUILD}'})
-
- session = 'foo-id'
- post_data = json.dumps({'max_updates': 2})
- self._MakeRPC('session/%s' % session, data=post_data)
-
- # The first request should provide an actual update.
- response = self._MakeRPC(update_label, data=update_request)
- self.assertIn('updatecheck status="ok"', response)
- # Tell devserver we started to download.
- response = self._MakeRPC(update_label, data=download_started_request,
- session=session)
-
- # Another update request also should go fine.
- response = self._MakeRPC(update_label, data=update_request,
- session=session)
- self.assertIn('updatecheck status="ok"', response)
- # Tell devserver we started to download.
- response = self._MakeRPC(update_label, data=download_started_request,
- session=session)
-
- # But the third update request should not be valid anymore since we hit the
- # max updates ceiling.
- response = self._MakeRPC(update_label, data=update_request,
- session=session)
- self.assertIn('updatecheck status="noupdate"', response)
-
def testPidFile(self):
"""Test that using a pidfile works correctly."""
with open(self.pidfile, 'r') as f:
