)]}'
{"doc/source/configuration.rst":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"db4b1c96127e2f40d765721ed8ad215367d80ace","unresolved":false,"context_lines":[{"line_number":509,"context_line":".. attr:: tenant-resource-limits"},{"line_number":510,"context_line":"   :type: list"},{"line_number":511,"context_line":""},{"line_number":512,"context_line":"   A list of global resource limits enforced per tenant (i.e. Zuul tenants)"},{"line_number":513,"context_line":""},{"line_number":514,"context_line":"   .. code-block:: yaml"},{"line_number":515,"context_line":""}],"source_content_type":"text/x-rst","patch_set":5,"id":"93dec979_d5f9a16c","line":512,"updated":"2021-08-11 17:35:13.000000000","message":"nit: Maybe e.g. instead of i.e. here since programs other than zuul could supply a tenant.  Also, feel free to end sentences with punctuation.  :)  (I think some lines in the previous section omitted it probably only because the author may have been concerned with the interaction with :ref: (but they needn\u0027t be)).\n\nI wouldn\u0027t -1 this, but since there are some more significant changes I\u0027m requesting below, it might be nice to go ahead and tidy this up.","commit_id":"2053d39324ff255cd1f7e4833ac19f74ffee88b6"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"cf1ac9270d12c46242abb8ad700479bb37eb712f","unresolved":false,"context_lines":[{"line_number":513,"context_line":""},{"line_number":514,"context_line":"   These limits are calculated on a best-effort basis using a launchers locally"},{"line_number":515,"context_line":"   cached view on existing nodes. Therefore, these limits are not guaranteed to"},{"line_number":516,"context_line":"   be exact in all cases."},{"line_number":517,"context_line":"   "},{"line_number":518,"context_line":"   .. code-block:: yaml"},{"line_number":519,"context_line":""}],"source_content_type":"text/x-rst","patch_set":11,"id":"46905ab9_8e071edf","line":516,"updated":"2021-08-18 13:56:58.000000000","message":"I think it\u0027s fine to keep this caveat in the docs, though I don\u0027t think it\u0027s fair to blame the cache.\n\nThis was true before too -- there is no global coordination, so even if we performed zk queries for everything in real time, multiple launchers would still race to fill the quota.  That race could occur over seconds or even minutes as they performed cloud operations.  Whereas the additional skew introduced by using a cached node list is on the order of milliseconds.\n\nIt would me more correct to say that because multiple launchers do not coordinate with each other the limits are not guaranteed to be exact.","commit_id":"2bf8a55edcf43a4d9108f068c5bda1655d40d0e9"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"ce607104792e56fe6b00cc54336c6fcc085fd286","unresolved":true,"context_lines":[{"line_number":514,"context_line":"   These limits are calculated on a best-effort basis. Because of parallelism"},{"line_number":515,"context_line":"   within launcher instances, and especially with multiple launcher instances,"},{"line_number":516,"context_line":"   the limits are not guaranteed to be exact."},{"line_number":517,"context_line":"   "},{"line_number":518,"context_line":"   .. code-block:: yaml"},{"line_number":519,"context_line":""},{"line_number":520,"context_line":"      tenant-resource-limits:"}],"source_content_type":"text/x-rst","patch_set":12,"id":"3c474fd9_adb20aab","line":517,"updated":"2021-08-30 06:48:50.000000000","message":"nit: whitespace","commit_id":"7275ecfe3c95d3375921fcf36d437b5598b48f64"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"ce607104792e56fe6b00cc54336c6fcc085fd286","unresolved":true,"context_lines":[{"line_number":530,"context_line":"      :example: example-tenant"},{"line_number":531,"context_line":"      :required:"},{"line_number":532,"context_line":""},{"line_number":533,"context_line":"      A tenant name correspodinding, e.g., to a Zuul tenant."},{"line_number":534,"context_line":""},{"line_number":535,"context_line":"   .. attr:: max-servers"},{"line_number":536,"context_line":"      :default: infinity"}],"source_content_type":"text/x-rst","patch_set":12,"id":"87bcb91f_5190449a","line":533,"updated":"2021-08-30 06:48:50.000000000","message":"nit: drop the e.g.,","commit_id":"7275ecfe3c95d3375921fcf36d437b5598b48f64"}],"nodepool/driver/__init__.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"a4f37f05efec14940e70606f63b9a4013d37fe8f","unresolved":false,"context_lines":[{"line_number":381,"context_line":"        self.manager \u003d self.pw.getProviderManager()"},{"line_number":382,"context_line":"        # this is set by a child class if it overrides this method"},{"line_number":383,"context_line":"        # usually set via `manager.estimatedNodepoolQuotaUsed`"},{"line_number":384,"context_line":"        # which requrires the manager to implement `QuotaSupport`"},{"line_number":385,"context_line":"        self._nodepool_quota_used \u003d None"},{"line_number":386,"context_line":""},{"line_number":387,"context_line":"    @property"}],"source_content_type":"text/x-python","patch_set":3,"id":"2a6dec76_81a4059a","line":384,"updated":"2021-07-15 22:19:18.000000000","message":"It would be helpful to describe what this is and what it\u0027s used for.  But I\u0027m not sure we\u0027ll be able to keep it as-is -- se the comment about removing the estimatedQuotaUsed calls.\n\nI think the simplest thing would be to leave the existing system alone, and then add a new launcher method that iterates over all the ZK nodes and returns a QuotaInformation object.  You can use the nodeIterator to make sure we\u0027re iterating over the TreeCache which means we don\u0027t need to talk to ZK.  It should be fairly efficient.","commit_id":"99c96a2022dbd83a9178a79cbe8a0add8ee23cb9"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"a4f37f05efec14940e70606f63b9a4013d37fe8f","unresolved":false,"context_lines":[{"line_number":582,"context_line":"            self.log.info("},{"line_number":583,"context_line":"                \"Deferring node request because it would exceed tenant quota\")"},{"line_number":584,"context_line":"            self._declinedHandlerCleanup(unpause\u003dFalse)"},{"line_number":585,"context_line":"            return"},{"line_number":586,"context_line":""},{"line_number":587,"context_line":"        if declined_reasons:"},{"line_number":588,"context_line":"            self.log.info(\"Declining node request because %s\","}],"source_content_type":"text/x-python","patch_set":3,"id":"836abd96_101bde20","line":585,"updated":"2021-07-15 22:19:18.000000000","message":"I think it would be better if we could avoid getting this far -- a fair bit of work has already been done needlessly to get to this point: we\u0027ve locked the node request, created a request handler, added it to the launcher\u0027s list of ongoing requests, and then we\u0027re going to immediately unlock the request and remove the request handler record because it\u0027s done.\n\nIf we could perform this check inside the _assignHandlers method in the launcher, we could avoid all of that (especially the zk writes).  That could get pretty important in a bad situation, where a tenant is fully at quota and still has lots of requests in the queue; we would need to lock and unlock every one of them just to get past them to any requests from other tenants that are behind them.\n\nNow, there\u0027s still a race condition, in that another launcher could fulfill a request which puts the tenant at quota between the check in _assignHandlers and here.  But there\u0027s already a race condition even with only this check -- two launchers could be executing this code at exactly the same time.  This is a situation that is unique to cluster-wide quota calculations -- we don\u0027t have it anywhere else in nodepool.\n\nFixing that race condition would be very costly indeed (we would either need to add some kind of tenant lock for all providers, or check the tenant quota yet again after fulfilling the request to see if we need to give it back).  But perhaps we\u0027re okay with the possibility a tenant might go over quota due to a race condition across providers.  If so, then we don\u0027t need to solve that problem.  And if we\u0027re okay with that, then we probably don\u0027t need a second check here and can just move this to _assignHandlers.","commit_id":"99c96a2022dbd83a9178a79cbe8a0add8ee23cb9"}],"nodepool/driver/openstack/handler.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"a4f37f05efec14940e70606f63b9a4013d37fe8f","unresolved":false,"context_lines":[{"line_number":315,"context_line":""},{"line_number":316,"context_line":"    def _setFromPoolWorker(self):"},{"line_number":317,"context_line":"        super()._setFromPoolWorker()"},{"line_number":318,"context_line":"        self._nodepool_quota_used \u003d self.manager.estimatedNodepoolQuotaUsed()"},{"line_number":319,"context_line":""},{"line_number":320,"context_line":"    @property"},{"line_number":321,"context_line":"    def alive_thread_count(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"f87c448b_5d31fcce","line":318,"updated":"2021-07-15 22:19:18.000000000","message":"I\u0027m a little concerned here.  Several calls to self.manager.estimatedNodepoolQuotaUsed have been removed from hasRemainingQuota in favor of using this value.  But this value is only set when a NodeRequestHandler is created.  They can live for quite a long time and exist in a sort of suspended state while they wait for quota to become available.  I\u0027m not convinced we can remove those calls.","commit_id":"99c96a2022dbd83a9178a79cbe8a0add8ee23cb9"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"a4f37f05efec14940e70606f63b9a4013d37fe8f","unresolved":false,"context_lines":[{"line_number":409,"context_line":"        tenant_quota \u003d QuotaInformation("},{"line_number":410,"context_line":"            default\u003dmath.inf,"},{"line_number":411,"context_line":"            **self.pw.nodepool.config.tenant_resource_limits[tenant])"},{"line_number":412,"context_line":"        self.log.info(str(tenant_quota))"},{"line_number":413,"context_line":""},{"line_number":414,"context_line":"        tenant_quota.subtract(used_quota)"},{"line_number":415,"context_line":"        tenant_quota.subtract(needed_quota)"}],"source_content_type":"text/x-python","patch_set":3,"id":"301a1bb2_b720db20","line":412,"updated":"2021-07-15 22:19:18.000000000","message":"Leftover from development?","commit_id":"99c96a2022dbd83a9178a79cbe8a0add8ee23cb9"}],"nodepool/launcher.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"db4b1c96127e2f40d765721ed8ad215367d80ace","unresolved":false,"context_lines":[{"line_number":173,"context_line":"                # do not decline request as it would pause the provider for"},{"line_number":174,"context_line":"                # requests of other tenants."},{"line_number":175,"context_line":"                # Instead defer it to be handled and fulfilled at a later run."},{"line_number":176,"context_line":"                log.info("},{"line_number":177,"context_line":"                    \"Deferring request because it would exceed tenant quota\")"},{"line_number":178,"context_line":"                continue"},{"line_number":179,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"7c85ce45_dc9337de","line":176,"updated":"2021-08-11 17:35:13.000000000","message":"We usually log things we don\u0027t do (especially things that we repeatedly don\u0027t do, like this) at debug level.  Otherwise a user running at info level would see this message over and over.  Note that this is similar to line 128 above or 182 below.","commit_id":"2053d39324ff255cd1f7e4833ac19f74ffee88b6"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"db4b1c96127e2f40d765721ed8ad215367d80ace","unresolved":false,"context_lines":[{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def _getUsedQuotaForTenant(self, tenant_name):"},{"line_number":266,"context_line":"        used_quota \u003d QuotaInformation()"},{"line_number":267,"context_line":"        for node in self.zk.nodeIterator():"},{"line_number":268,"context_line":"            if not node.resources:"},{"line_number":269,"context_line":"                continue"},{"line_number":270,"context_line":"            if node.tenant_name \u003d\u003d tenant_name:"}],"source_content_type":"text/x-python","patch_set":5,"id":"e4bd5ae4_4c4c8c82","line":267,"updated":"2021-08-11 17:35:13.000000000","message":"Imagine one large tenant with thousands of requests, and it\u0027s at tenant quota.  assignHandlers is going to call hasTenantQuota for every one of those requests, and each of those calls is going to iterate over all of the the nodes in ZK (and there should be a lot, because that big tenant is at quota).  That makes this polynomial time.\n\nThe easiest fix would probably be to do a short (30 seconds?) TTL cache on getUsedQutoaForTenant.  Then we\u0027re back to linear time for assignHandlers.","commit_id":"2053d39324ff255cd1f7e4833ac19f74ffee88b6"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"9b6846245bfbdf1b9c954b17bf92e905e8c50a7f","unresolved":false,"context_lines":[{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def _getUsedQuotaForTenant(self, tenant_name):"},{"line_number":266,"context_line":"        used_quota \u003d QuotaInformation()"},{"line_number":267,"context_line":"        for node in self.zk.nodeIterator():"},{"line_number":268,"context_line":"            if not node.resources:"},{"line_number":269,"context_line":"                continue"},{"line_number":270,"context_line":"            if node.tenant_name \u003d\u003d tenant_name:"}],"source_content_type":"text/x-python","patch_set":5,"id":"b7d6865a_b2f7897a","line":267,"updated":"2021-08-17 00:51:19.000000000","message":"Those are good points; I stand corrected on the nodeIterator cache :).  The nodeIterator uses cached data to provide the actual node info, but the list of nodes still comes from ZK.  So at least we won\u0027t be performing nodes*requests fetches of data, but we will perform a get_children for each request that we consider processing.  Maybe we can reduce that by altering nodeIterator to use the cache to provide the list of nodes for this?  It should be almost accurate (modulo the lag introduced by the ZK watch firing).\n\nThis might need to be an additional argument to nodeIterator if the existing usages of it are expecting an authoritative list of nodes.\n\nIf that\u0027s difficult, I agree it\u0027s not a showstopper, but I think it might be relatively simple and still sufficiently accurate for this purpose.\n\nIt\u0027s possible if the tests run very quickly we might see the race and so we might need to make sure all the nodes the test expects are in the tree cache before proceeding.","commit_id":"2053d39324ff255cd1f7e4833ac19f74ffee88b6"},{"author":{"_account_id":31083,"name":"Benjamin Schanzel","email":"benjamin.schanzel@bmw.de","username":"benjamin.schanzel"},"change_message_id":"3f81f11870af3aff79adddc2bc4a0a9fcca27687","unresolved":true,"context_lines":[{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def _getUsedQuotaForTenant(self, tenant_name):"},{"line_number":266,"context_line":"        used_quota \u003d QuotaInformation()"},{"line_number":267,"context_line":"        for node in self.zk.nodeIterator():"},{"line_number":268,"context_line":"            if not node.resources:"},{"line_number":269,"context_line":"                continue"},{"line_number":270,"context_line":"            if node.tenant_name \u003d\u003d tenant_name:"}],"source_content_type":"text/x-python","patch_set":5,"id":"a08aa96e_268b8b38","line":267,"in_reply_to":"e4bd5ae4_4c4c8c82","updated":"2021-08-12 11:06:46.000000000","message":"Referring to your comment on patch set 3 [1], the nodeIterator is already a cached view of the nodes in zk and should be quite efficient.\n\nIntroducing a ttl_cache here would be difficult at least at the corresponding unit tests because we\u0027d either need to make sure the cache is disabled for the tests (ttl\u003d0) or make the tests pause for at least ttl+1 at several points for them to not be flaky (this potentially introduces a considerable time penalty, depending on the chosen ttl value).\n\nAlso, I\u0027m a bit concerned that a ttl-cached view on the tenant quota would make the race condition you mentioned here [2] even worse.\n\nSo all in all I\u0027d rather cope with the additional iteration over the cached/local node list.\n\n[1] https://review.opendev.org/c/zuul/nodepool/+/800765/3/nodepool/driver/__init__.py#384\n[2] https://review.opendev.org/c/zuul/nodepool/+/800765/3/nodepool/driver/__init__.py#585","commit_id":"2053d39324ff255cd1f7e4833ac19f74ffee88b6"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"ce607104792e56fe6b00cc54336c6fcc085fd286","unresolved":true,"context_lines":[{"line_number":255,"context_line":"        for ntype in request.node_types:"},{"line_number":256,"context_line":"            # can not determine needed quota for ntype if label not in pool"},{"line_number":257,"context_line":"            # therefore just skip them here to avoid errors in"},{"line_number":258,"context_line":"            # \u0027quotaNeededByLabel\u0027"},{"line_number":259,"context_line":"            if ntype not in pool.labels:"},{"line_number":260,"context_line":"                continue"},{"line_number":261,"context_line":"            needed_quota.add(provider_manager.quotaNeededByLabel(ntype, pool))"}],"source_content_type":"text/x-python","patch_set":12,"id":"a750370b_05d66655","line":258,"updated":"2021-08-30 06:48:50.000000000","message":"Note that 785821 adds resources so I think you could account for them using this; see ~\n\nhttps://review.opendev.org/c/zuul/nodepool/+/785821/3/nodepool/driver/utils.py","commit_id":"7275ecfe3c95d3375921fcf36d437b5598b48f64"}],"releasenotes/notes/tenant-scoped-resource-limits-7d0dcc3d6e279334.yaml":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"a4f37f05efec14940e70606f63b9a4013d37fe8f","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Add Tenant-Scoped Resource Quota"},{"line_number":5,"context_line":""},{"line_number":6,"context_line":"    Adding the option to put quota on resources on a per-tenant basis (i.e."},{"line_number":7,"context_line":"    Zuul tenants)."}],"source_content_type":"text/x-yaml","patch_set":3,"id":"020184b2_075cfde6","line":4,"updated":"2021-07-15 22:19:18.000000000","message":"This is written a bit like a commit message instead of a release note.  I\u0027d drop the first sentance.","commit_id":"99c96a2022dbd83a9178a79cbe8a0add8ee23cb9"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"a4f37f05efec14940e70606f63b9a4013d37fe8f","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    Add Tenant-Scoped Resource Quota"},{"line_number":5,"context_line":""},{"line_number":6,"context_line":"    Adding the option to put quota on resources on a per-tenant basis (i.e."},{"line_number":7,"context_line":"    Zuul tenants)."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"    It adds a new top-level config structure ``tenant-resource-limits`` under"},{"line_number":10,"context_line":"    which one can specify a number of tenants, each with ``max-servers``,"}],"source_content_type":"text/x-yaml","patch_set":3,"id":"975158d2_c4c5f3d5","line":7,"updated":"2021-07-15 22:19:18.000000000","message":"s/Adding/Added/ and s/put/set/ and then this is a good introductory sentence.","commit_id":"99c96a2022dbd83a9178a79cbe8a0add8ee23cb9"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"a4f37f05efec14940e70606f63b9a4013d37fe8f","unresolved":false,"context_lines":[{"line_number":6,"context_line":"    Adding the option to put quota on resources on a per-tenant basis (i.e."},{"line_number":7,"context_line":"    Zuul tenants)."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"    It adds a new top-level config structure ``tenant-resource-limits`` under"},{"line_number":10,"context_line":"    which one can specify a number of tenants, each with ``max-servers``,"},{"line_number":11,"context_line":"    ``max-cores``, and ``max-ram`` limits.  These limits are valid globally,"},{"line_number":12,"context_line":"    i.e., for all providers. This is contrary to currently existing provider"}],"source_content_type":"text/x-yaml","patch_set":3,"id":"191c7e7c_cc6c5480","line":9,"updated":"2021-07-15 22:19:18.000000000","message":"Maybe say \"A new top-level config structure has been added:\" since the antecedent of \"it\" isn\u0027t clear.","commit_id":"99c96a2022dbd83a9178a79cbe8a0add8ee23cb9"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"a4f37f05efec14940e70606f63b9a4013d37fe8f","unresolved":false,"context_lines":[{"line_number":9,"context_line":"    It adds a new top-level config structure ``tenant-resource-limits`` under"},{"line_number":10,"context_line":"    which one can specify a number of tenants, each with ``max-servers``,"},{"line_number":11,"context_line":"    ``max-cores``, and ``max-ram`` limits.  These limits are valid globally,"},{"line_number":12,"context_line":"    i.e., for all providers. This is contrary to currently existing provider"},{"line_number":13,"context_line":"    and pool quotas, which only are consindered for nodes of the same provider."},{"line_number":14,"context_line":"    This feature is optional and tenant quotas are ignored for any NodeRequests"},{"line_number":15,"context_line":"    that do not deliver tenant information with them. Also no quota is"}],"source_content_type":"text/x-yaml","patch_set":3,"id":"5133234a_c6378c20","line":12,"updated":"2021-07-15 22:19:18.000000000","message":"Maybe say \"This differs from currently existing...\"; \"contrary\" sounds like they work against each other rather than working together.","commit_id":"99c96a2022dbd83a9178a79cbe8a0add8ee23cb9"}]}