If we hit the internal timeout while launching or deleting a server,
we raise an exception and then log the traceback. This is a not-
unexpected occurance, and the traceback is not useful since it's
just one stack frame within the same class, so instead, let's log
these timeouts at warning level without the traceback.
Change-Id: Id4806d8ea2d0a232504e5a75d69cec239bcac670
We currently use a threadpool executor to scan up to 10 nodes at
a time for ssh keys. If they are slow to respond, that can create
a bottleneck. To alleviate this, use a state machine model for
managing the process, and drive each state machine from a single
thread.
We use select.epoll() to handle the potentially large number of
connections that could be happening simultaneously.
Note: the paramiko/ssh portion of this process spawns its own
thread in the background (and always has). Since we are now allowing
more keyscan processis in parallel, we could end up with an
unbounded set of paramiko threads in the background. If this is
a concern we may need to cap the number of requests handled
simultaneously. Even if we do that, this will still result in
far fewer threads than simply increasing the cap on the threadpool
executor.
Change-Id: I42b76f4c923fd9441fb705e7bffd6bc9ea7240b1
Adds a test to verify that the Adapter API to fetch the console logs
when a keyscan error occurs is called correctly.
Change-Id: I599b97f6829406ddfc7f1355b85c73fbe57533e7
To observe the performance of the ZK connection and the new tree
caches, add some statsd metrics for each of these. This will
let us monitor queue size over time.
Also, update the assertReportedStat method to output all received
stats if the expected stat was not found (like Zuul).
Change-Id: Ia7e1e0980fdc34007f80371ee0a77d4478948518
Depends-On: https://review.opendev.org/886552
The OpenStack SDK performs a lot of processing on the JSON data
returned by nova, and on large server lists, this can dwarf the
actual time needed to receive and parse the JSON.
Nodepool uses very little of this information, so let's use the
keystoneauth session to get a simple JSON list.
The Server object that SDK normally returns is a hybrid object
that provides both attributes and dictionary keys. One method
that we call has some lingering references to accessors, so we
create a UserDict subclass to handle those. Nodepool-internal
references are updated from attributes to dictionary keys.
Change-Id: Iecc5976858e8d2ee6894a521f6a30f10ae9c6177
This adds retry handling to the state machine driver and removes it
from the driver adapters.
This allows the adapters to be simpler as they don't need to implement
any internal retry handling. It also lets us choose to retry due
to events handled by the statemachine driver itself.
One of those events is a failed keyscan. In the old OpenStack driver,
a failed keyscan would result in a retry rather than a launch failure,
but the migration of the OpenStack driver to statemachine inadvertently
changed that behavior. This restores the old behavior, applies it
to all drivers, and codifies it with a unit test.
Several logging lines have been changed to use arguments instead of
format strings for consistency and safety.
Change-Id: I670ef8d762558cf8346f071be282af8dfc803747
The OpenStack driver reports some leaked metrics. Extend that in
a generic way to all statemachine drivers. Doing so also adds
some more metrics to the OpenStack driver.
Change-Id: I97c01b54b576f922b201b28b117d34b5ee1a597d
We consult ZooKeeper to determine the most recent image upload
when we decide whether we should accept or decline a request. If
we accept the request, we also consult it again for the same
information when we start building the node. In both cases, we
can use the cache to avoid what may potentially be (especially in
the case of a large number of images or uploads) quite a lot of
ZK requests. Our cache should be almost up to date (typically
milliseconds, or at the worst, seconds behind), and the worst
case is equivalent to what would happen if an image build took
just a few seconds longer. The tradeoff is worth it.
Similarly, when we create min-ready requests, we can also consult
the cache.
With those 3 changes, all references to getMostRecentImageUpload
in Nodepool use the cache.
The original un-cached method is kept as well, because there are
an enormous number of references to it in the unit tests and they
don't have caching enabled.
In order to reduce the chances of races in many tests, the startup
sequence is normalized to:
1) start the builder
2) wait for an image to be available
3) start the launcher
4) check that the image cache in the launcher matches what
is actually in ZK
This sequence (apart from #4) was already used by a minority of
tests (mostly newer tests). Older tests have been updated.
A helper method, startPool, implements #4 and additionally includes
the wait_for_config method which was used by a random assortment
of tests.
Change-Id: Iac1ff8adfbdb8eb9a286929a59cf07cd0b4ac7ad
This adds support for staying within OpenStack volume quota limits
on instances that utilize boot-from-volume.
Change-Id: I1b7bc177581d23cecd9443a392fb058176409c46
This updates the OpenStack driver to use the statemachine framework.
The goal is to revise all remaining drivers to use the statemachine
framework for two reasons:
1) We can dramatically reduce the number of threads in Nodepool which
is our biggest scaling bottleneck. The OpenStack driver already
includes some work in that direction, but in a way that is unique
to it and not easily shared by other drivers. The statemachine
framework is an extension of that idea implemented so that every driver
can use it. This change further reduces the number of threads needed
even for the openstack driver.
2) By unifying all the drivers with a simple interface, we can prepare
to move them into Zuul.
There are a few updates to the statemachine framework to accomodate some
features that only the OpenStack driver used to date.
A number of tests need slight alteration since the openstack driver is
the basis of the "fake" driver used for tests.
Change-Id: Ie59a4e9f09990622b192ad840d9c948db717cce2
The launcher implementation assumed that only one request handler will
be paused at any given point in time. However, this is not true when
e.g. the request handler accepts multiple requests that all run into a
quota limit during launch.
The consequence of this is that the pool is unpaused too early and we
might accept other node requests until the provider is paused again.
This could lead to a starvation of earlier paused handlers as they were
fulfilled in a LIFO fashion.
To fix this edge case we will store paused request handlers in a set and
only unpause the provider when there are no paused handlers anymore.
Paused handlers are now also run in priority order.
Change-Id: Ia34e2844533ce9942d489838c4ce14a605d79287
When a provider is at quota, we pause it, and paused means paused.
That means we don't do anything with any other requests.
Unfortunately, that includes requests that the given provider can't
even handle. So if a provider pauses because it is at quota while
other providers continue to operate, if a request for a node type
that no providers can handle arrives, then that request will remain
outstanding until this provider becomes unpaused and can decline it.
Requests shouldn't need to wait so long to be declined by providers
which can never under any circumstances handle them. To address this,
we will now run the assignHandlers method whether we are paused or
not. Within assignHandlers, we will process all requests regardless
of whether we are paused (but we won't necessarily accept them yet).
We will decide whether a request will be declined or not, and if it
will be declined, we will do so regardless of whether we are paused.
Finally, only if we are unpaused and do not expect to decline the
request will we accept it.
Change-Id: Ied9e4577670ea65b1d5ecfef95a7f837a7b6ac61
This allows users to create tags (or properties in the case of OpenStack)
on instances using string interpolation values. The use case is to be
able to add information about the tenant* which requested the instance
to cloud-provider tags.
* Note that ultimately Nodepool may not end up using a given node for
the request which originally prompted its creation, so care should be
taken when using information like this. The documentation notes that.
This feature uses a new configuration attribute on the provider-label
rather than the existing "tags" or "instance-properties" because existing
values may not be safe for use as Python format strings (e.g., an
existing value might be a JSON blob). This could be solved with YAML
tags (like !unsafe) but the most sensible default for that would be to
assume format strings and use a YAML tag to disable formatting, which
doesn't help with our backwards-compatibility problem. Additionally,
Nodepool configuration does not use YAML anchors (yet), so this would
be a significant change that might affect people's use of external tools
on the config file.
Testing this was beyond the ability of the AWS test framework as written,
so some redesign for how we handle patching boto-related methods is
included. The new approach is simpler, more readable, and flexible
in that it can better accomodate future changes.
Change-Id: I5f1befa6e2f2625431523d8d94685f79426b6ae5
For drivers that support tagging/metadata (openstack, aws, azure),
Add or enhance support for supplying tags for uploaded diskimages.
This allows users to set metadata on the global diskimage object
which will then be used as default values for metadata on the
provider diskimage values. The resulting merged dictionary forms
the basis of metadata to be associated with the uploaded image.
The changes needed to reconcile this for the three drivers mentioned
above are:
All: the diskimages[].meta key is added to supply the default values
for provider metadata.
OpenStack: provider diskimage metadata is already supported using
providers[].diskimages[].meta, so no further changes are needed.
AWS, Azure: provider diskimage tags are added using the key
providers[].diskimages[].tags since these providers already use
the "tags" nomenclature for instances.
This results in the somewhat incongruous situation where we have
diskimage "metadata" being combined with provider "tags", but it's
either that or have images with "metadata" while we have instances
with "tags", both of which are "tags" in EC2. The chosen approach
has consistency within the driver.
Change-Id: I30aadadf022af3aa97772011cda8dbae0113a3d8
This updates the ZooKeeper class to inherit from ZooKeeperBase
and utilize its connection methods.
It also moves the connection loss detection used by the builder
to be more localized and removes unused methods.
Change-Id: I6c9dbe17976560bc024f74cd31bdb6305d51168d
Now that the component we registered is a "pool" change the call
sites to use "launcher_pools" instead of "launchers". This may
reduce some ambiguity.
(s/launcher/pool/ might still be ambiguous since it may not be clear
whethere we're talking about our own pools or other pools; thus the
choice of "launcher_pool" for the variable name.)
Also, remove a redundant test assertion.
Change-Id: I865883cdb115bf72a3bd034d9290f60666d64b66
This lets users configure providers which should fulfill requests
before other providers. This facilitates using a less expensive
cloud before using a more expensive one.
The default priority is 100, to facilitate either raising above
or lowering below the default (while using only positive integers
in order to avoid confusion).
Change-Id: I969ea821e10a7773a0a8d135a4f13407319362ee
This uses a cache and lets us update metadata about components
and act on changes quickly (as compared to the current launcher
registry which doesn't have provision for live updates).
This removes the launcher registry, so operators should take care
to update all launchers within a short period of time since the
functionality to yield to a specific provider depends on it.
Change-Id: I6409db0edf022d711f4e825e2b3eb487e7a79922
We have made many improvements to connection handling in Zuul.
Bring those back to Nodepool by copying over the zuul/zk directory
which has our base ZK connection classes.
This will enable us to bring other Zuul classes over, such as the
component registry.
The existing connection-related code is removed and the remaining
model-style code is moved to nodepool.zk.zookeeper. Almost every
file imported the model as nodepool.zk, so import adjustments are
made to compensate while keeping the code more or less as-is.
Change-Id: I9f793d7bbad573cb881dfcfdf11e3013e0f8e4a3
The quota cache may not be a valid dictionary when
invalidateQuotaCache() is called (e.g. when 'ignore-provider-quota' is
used in OpenStack). In that case, don't attempt to treat the None as a
dictionary as this raises a TypeError exception.
This bug was preventing Quota errors from OpenStack from causing
nodepool to retry the node request when ignore-provider-quota is True,
because the OpenStack handler calles invalidateQuotaCache() before
raising the QuotaException. Since invalidateQuotaCache() was raising
TypeError, it prevented the QuotaException from being raised and the
node allocation was outright failed.
A test has been added to verify that nodepool and OpenStack will now
retry node allocations as intended.
This fixes that bug, but does change the behavior of OpenStack when
ignore-provider-quota is True and it returns a Quota error.
Change-Id: I1916c56c4f07c6a5d53ce82f4c1bb32bddbd7d63
Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
The optimization introduced in Id04f57ff1e4c28357370729b6383f5119cd616dc
can lead to a starvation of certain requests under the following
conditions:
* node request with a requested provider that doesn't support the
required node types
* another provider that could technically serve the request yields to
the requested provider
* requested provider is at quota or high inflow of node request
* requested provider postpones rejecting the yielded request as it
doesn't support the required labels
To avoid starvation of those request, we only yield to the requested
provider if it is capable of serving the request. This is done by
checking the supported labels of the requested provider.
Change-Id: I0ded785a13d1f955a71d519dc40e5e5c0ec35043
Propagate the NodeRequest.requestor to a new property Node.requestor
which usually holds the zuul_system_id, or "NodePool:min_ready".
This is set initially when first creating a node, and is updated when a
ready node is assigned to a request. This is to always know for which
requestor is a node got allocated.
Change-Id: Ifd54a94bae39f31a70bdedce384a64c2a04495c1
We noticed that a provider sometimes takes very long to serve a request
because it is busy declining requests with a higher priority. This is
made worse by the fact that we stop processing requests to remove
completed handlers every 15s. Afterwards we will start over and also
include new requests.
This means, that as long as there are requests with a higher priority we
will process them first even if they need to be declined because the
pool doesn't provide the requested label(s).
To improve this we will first sort the open node requests based on
whether they can be fulfilled by the current provider and only then by
priority. This has the added benefit that a provider might not even need
to decline a request if it could be fulfilled by another provider in the
meantime.
Change-Id: Id04f57ff1e4c28357370729b6383f5119cd616dc
If a provider (or its configuration) is sufficiently broken that
the provider manager is unable to start, then the launcher will
go into a loop where it attempts to restart all providers in the
system until it succeeds. During this time, no pool managers are
running which mean all requests are ignored by this launcher.
Nodepool continuously reloads its configuration file, and in case
of an error, the expected behavior is to continue running and allow
the user to correct the configuration and retry after a short delay.
We also expect providers on a launcher to be independent of each
other so that if ones fails, the others continue working.
However since we neither exit, nor process node requests if a
provider manager fails to start, an error with one provider can
cause all providers to stop handling requests with very little
feedback to the operator.
To address this, if a provider manager fails to start, the launcher
will now behave as if the provider were absent from the config file.
It will still emit the error to the log, and it will continuously
attempt to start the provider so that if the error condition abates,
the provider will start.
If there are no providers on-line for a label, then as long as any
provider in the system is running, node requests will be handled
and declined and possibly failed while the broken provider is offilne.
If the system contains only a single provider and it is broken, then
no requests will be handled (failed), which is the current behavior,
and still likely to be the most desirable in that case.
Change-Id: If652e8911993946cee67c4dba5e6f88e55ac7099
This change adds the option to put quota on resources on a per-tenant
basis (i.e. Zuul tenants).
It adds a new top-level config structure ``tenant-resource-limits``
under which one can specify a number of tenants, each with
``max-servers``, ``max-cores``, and ``max-ram`` limits. These limits
are valid globally, i.e., for all providers. This is contrary to
currently existing provider and pool quotas, which only are consindered
for nodes of the same provider.
Change-Id: I0c0154db7d5edaa91a9fe21ebf6936e14cef4db7
The launcher implements deletes using threads, and unlike with
launches, does not give drivers an opportunity to override that
and handle them without threads (as we want to do in the state
machine driver).
To correct this, we move the NodeDeleter class from the launcher
to driver utils, and add a new driver Provider method that returns
the NodeDeleter thread. This is added in the base Provider class
so all drivers get this behavior by default.
In the state machine driver, we override the method so that instead
of returning a thread, we start a state machine and add it to a list
of state machines that our internal state machine runner thread
should drive.
Change-Id: Iddb7ed23c741824b5727fe2d89c9ddbfc01cd7d7
Ansible needs to know which shell type the node uses to operate
correctly, especially for ssh connections for windows nodes because
otherwise ansible defaults to trying bash.
Change-Id: I71abfefa57aaafd88f199be19ee7caa64efda538
We need to set pool info on leaked instances so that they are properly
accounted for against quota. When a znode has provider details but not
pool details we can't count it against quota used but also don't count
it as unmanaged quota so end up in limbo.
To fix this we set pool info metadata so that when an instance leaks we
can recover the pool info and set it on the phony instance znode records
used to delete those instances.
Change-Id: Iba51655f7bf86987f9f88bb45059464f9f211ee9
As a follow-on to I81b57d6f6142e64dd0ebf31531ca6489d6c46583, bring
consistency to the resource leakage cleanup statistics provided by
nodepool.
New stats for cleanup of leaked instances and floating ips are added
and documented. For consistency, the downPorts stat is renamed to
leaked.ports.
The documenation is re-organised slightly to group common stats
together. The nodepool.task.<provider>.<task> stat is removed because
it is covered by the section on API stats below.
Change-Id: I9773181a81db245c5d1819fc7621b5182fbe5f59
As a follow-on to I615e530decbee6a46167a40748342d2193851c02, switch
one of the usernames in testing away from the default to test setting
the username from config.
Change-Id: Id05561b942ed96bc3cc011df6906d706f12d80bf
The "python-path" configuration option makes its way through to Zuul
where it sets the "ansible_interpreter_path" in the inventory.
Currently this defaults to "/usr/bin/python2" which is wrong for
Python 3-only distributions.
Ansible >=2.8 provides for automated discovery of the interpreter to
avoid runtime errors choosing an invalid interpreter [1]. Using this
should mean that "python-path" doesn't need to be explicitly for any
common case. As more distributions become Python 3 only, this should
"do the right thing" without further configuration.
This switches the default python-path to "auto". The dependent change
updates Zuul to accept this and use it when running with Ansible
>=2.8, or default back to "/usr/bin/python2" for earlier Ansible
versions.
Testing and documentation is updated, and a release note added.
[1] https://docs.ansible.com/ansible/2.8/reference_appendices/interpreter_discovery.html
Depends-On: https://review.opendev.org/682275
Change-Id: I02a1a618c8806b150049e91b644ec3c0cb826ba4
The nova api can return instance records for an instance that has been
deleted. When it does this the status should be "DELETED". This means we
should check for the instance to have no more record or if the record is
present check that the status if DELETED.
Change-Id: I7ad753a3c73f3d2cd78f4a380f78279af9206ada
There are some edge cases where the port cleanup logic is too
aggressive. This change attempts to address both of them in one commit:
* Some providers might spawn instances very slowly. In the past this was
handled by hardcoding the timeout to 10 minutes. This allows a user to
tweak the timeout in config.
* In the esoteric combination of using Ironic without the Ironic Neutron
agent, it's normal for ports to remain DOWN indefinitely. Setting the
timeout to 0, will work around that edge case.
Change-Id: I120d79c4b5f209bb1bd9907db172f94f29b9cb5d
We were depending only on the ephemeralness of launcher registration
nodes to indicate when a provider goes away. That only works when
nodepool-launcher is restarted since the ZK connection is shared
among all provider threads. This adds an API method to manually
deregister and modifies a test to ensure it works.
Change-Id: I363fc4a67a83b49b515b963f5d5accdcf1cb758c
Only a single test actually depends on having more than a single
upload thread active, so this is just wasteful. Reduce the default
to 1 and add an option to useBuilder() that tests may use to alter
the value.
Change-Id: I07ec96000a81153b51b79bfb0daee1586491bcc5
When creating instances with boot from volume we don't get quota
related information in the exception raised by wait_for_server. Also
in the server munch that is returned the fault information is
missing. This causes node failures when we run into the volume
quota. This can be fixed by explicitly fetching the server if we got
one and inspecting the fault information which contains more
information about the fault reason [1].
[1] Example fault reason:
Build of instance 4628f079-26a9-4a1d-aaa0-881ba4c7b9cb aborted:
VolumeSizeExceedsAvailableQuota: Requested volume or snapshot exceeds
allowed gigabytes quota. Requested 500G, quota is 10240G and 10050G
has been consumed.
Change-Id: I6d832d4dbe348646cd4fb49ee7cb5f6a6ad343cf
This adds the ability for a pool.label to override the
pool.host-key-checking value, while having a label exist in the same
pool. This is helpful because it is possible for 1 pool to mix network
configuration, and some nodes maybe missing a default gateway (making
them unroutable by default).
Change-Id: I934d42b8e48aedb0ebc03137b7305eb2af390fc7
Signed-off-by: Paul Belanger <pabelanger@redhat.com>
This change adds a new python_path Node attribute so that zuul executor
can remove the default hard-coded ansible_python_interpreter.
Change-Id: Iddf2cc6b2df579636ec39b091edcfe85a4a4ed10
We have a use case where we have a single pool, due to quota reasons,
but need the ability to selectively choose which network a label will
use. Now a nodepool operator will be able to define which networks are
attached to labels (in our case network appliances).
Change-Id: I3bfa32473c76b9fd59deee7d05b492e7cf67f69d
Signed-off-by: Paul Belanger <pabelanger@redhat.com>
James E. Blair