On startup, the launcher waits up to 5 seconds until it has seen
its own registry entry because it uses the registry to decide if
other components are able to handle a request, and if not, fail
the request.
In the case of a ZK disconnection, we will lose all information
about registered components as well as the tree caches. Upon
reconnection, we will repopulate the tree caches and re-register
our component.
If the tree cache repopulation happens first, our component
registration may be in line behind several thousand ZK events. It
may take more than 5 seconds to repopulate and it would be better
for the launcher to wait until the component registry is up to date
before it resumes processing.
To fix this, instead of only waiting on the initial registration,
we check each time through the launcher's main loop that the registry
is up-to-date before we start processing. This should include
disconnections because we expect the main loop to abort with an
error and restart in those cases.
This operates only on local cached data, so it doesn't generate any
extra ZK traffic.
Change-Id: I1949ec56610fe810d9e088b00666053f2cc37a9a
This follows the previous change and is intended to have little
or no behavior changes (only a few unit tests are updated to use
different placeholder values). It updates all textual references
of build numbers to build ids to better reflect that they are
UUIDs instead of integers.
Change-Id: I04b5eec732918f5b9b712f8caab2ea4ec90e9a9f
When we export and import image data (for backup/restore purposes),
we need to reset the ZK sequence counter for image builds in order
to avoid collisions. The only way we can do that is to create and
then delete a large number of znodes. Some sites (including
OpenDev) have sequence numbers that are in the hundreds of thousands.
To avoid this time-consuming operation (which is only intended to
be run to restore from backup -- when operators are already under
additional stress!), this change switches the build IDs from integer
sequences to UUIDs. This avoids the problem with collisions after
import (at least, to the degree that UUIDs avoid collisions).
The actual change is fairly simple, but many unit tests need to be
updated.
Since the change is user-visible in the command output (image lists,
etc), a release note is added.
A related change which updates all of the textual references of
build "number" to build "id" follows this one for clarity and ease
of review.
Change-Id: Ie7c68b094bc9733914a808756eeee8b62f696713
If a TreeCache encounters a ZK disconnect, then any events in its
event queue are no longer useful and possibly counter-productive.
If we process events from before the disconnection after a
reconnection, the cache could get into an erroneous state.
The cache is designed so that on initial or re-connection, we first
establish the watch, and then walk the tree to look for previously
existing data. All events we process (whether generated by the
watch or by walking the tree) must be in order after the watch was
established.
To ensure this, we clear the event queues on reconnect and restart
the event queue threads.
Additionally, this change cleans up handling of the case where
the cache key is None (the parser says we're not interested in the
object) but the preCacheHook didn't tell us to exit early. In that
case, we would previously most likely try to pop the None object
from the cache, which would generally be harmless. But we shouldn't
be doing that anyway, so add a check for whether we have an object
key in the main cache method and ignore the return values from the
preCacheHook.
Change-Id: I4bbedb19364c894a2033ef8c1d2e439299971a83
A logic error in the TreeCache prevented us from correctly
re-establishing the state of node locks after a zk connection
loss. In such a case, we need to fetch data for every item in
the tree to determine whether it exists, but we were only fetching
data for cache objects, not ancillary znodes like lock nodes.
Change-Id: I4e9faa4981b90a6b1f46d2f0347866e2166d5b7f
The TreeCache can be a bit of a bottleneck since it is centralizing
in a single thread fetch operations which may have previously
happened in different threads. It also may in some cases fetch
more data than we did previously since it will update all objects
in memory as they are updated in ZK regardless of whether nodepool
is interested in them at the moment. Or at least, it may concentrate
those fetches in a smaller timespan.
To address the bottleneck, this change uses asynchronous fetches
from ZooKeeper. The background worker which previously updated the
cache in response to events from ZK is split into two threads. The
first will decide whether or not a fetch for a given event is
necessary, and then fire off the async fetch operation if so. It
then pushes the future for that operation, along with other event
information into a second queue. A second background thread
(called the "playback" worker) will pop events from that queue,
still in order, and await the future. This ensures all cache
operations are still ordered, while multiple fetch operations can
be outstanding in parallel.
This relies even more heavily on the path parsing implemented by the
three caches to be correct. In particular, we want to make sure that
we don't accidentally fetch a znode if we're just dealing with a
lock node event. The previous code sequenced the checks so that we
would handle the lock node event and short circuit before deciding
whether to fetch. The new code reverses that, which means that it
is paramount that we don't accidentally have the path parsing return
a match on a lock node. The cache would function correctly, but it
would silently perform far more operations than necessary, so such an
error would likely go unnoticed for some time. To prevent this,
explicit testing for path parsing is added.
Change-Id: I6ef10c724c3993ee565510ab917dce64d2e3d3f9
If something is wrong with the TreeCache or ZK, then we may end up
with many events queued for processing. In that case, log a warning
message (but only every 60 seconds to avoid runaway logging).
Change-Id: I939b06aef3c65168c6211e45f1ca38ab672ba1e4
There were two errors related to connection handling in the new
treecache implementation:
1) Upon disconnection, the cache receives a "NONE" event with
the client state ("CONNECTING") and no path. This is a signal to
the watcher that the connection state has changed. Since we process
connection changes via the session listener, we should ignore this
event. We were previously attempting to process it and assumed that
the path attribute would be present. This change ignores NONE events
with no path.
2) The treecache is responsible for re-establishing watches if the
connection is LOST or SUSPENDED. Previously, it would only
re-establish watches and refresh the cache if the connection was LOST.
This change updates it to re-establish/refresh in both cases. The
"_started" variable is no longer necessary to distinguish between the
two. However, it may be useful for future consumers to know whether
or not the cache is synced, so it is converted to "_ready" in case
we want to expose that in the future.
Change-Id: Id1f9c34628d1cc04881621a83d1d1f78e9e0f366
Change Ic52dc0202bf629462d774406b08b7dee01396dd8 added a thread lock
to the Node and NodeRequest objects to avoid multiple threads
overwriting the lock attribute and therefore causing us to forget
whether a node was locked. However, some lock paths employ blocking
zk locks which can lead to deadlocks between the two types of locks.
I.e., one thread holds the ZK lock and tries to acquire the thread
lock in order to release the ZK lock, while another thread holds
the thread lock while trying to acquire the ZK lock.
We could avoid all of this if we get rid of the two-phase commit
problem where we set the lock attribute of the Node after
acquiring or releasing the ZK lock and just rely on the ZK lock
itself. However, the underlying kazoo Lock recipe appears to
suffer from the same original issue: it appears possible for an
release to overwrite the internal locked flag while an acquire is
happening.
So instead, we shore up our locking strategy by holding the
thread lock over the entire time period that the ZK lock is held.
This means that locking order is always:
1) acquire thread
2) acquire zk
3) release zk
4) release thread
Except in the case of a non-ephemeral ZK lock, such as that used
by the metastatic driver. We release the thread lock after
acquiring the ZK lock in that case. It doesn't really matter
anyway, the lock is never used again, and the ZK unlock method is
performed without reference to the Node.lock attribute. But it
seems like the right behavior.
Change-Id: I01b14a5af83ab6067573ef1dee9803041c339fbe
This replaces the three treecache derived caches with a new
cache based on a persistent recursive watcher.
This should greatly reduce the number of watches on the zk server,
radically simplify the caching logic, and allows us to ignore
events for znodes we are uninterested in (for example, we can avoid
performing "get" calls on lock nodes) which should reduce zk
network traffic.
Change-Id: Ie8b578f3b8dd39fdf43025a31d55ce4e5801b563
For sites with large numbers of image uploads, performing a web API
request of /image-list can be time-consuming. To improve response
time, maintain a TreeCache of image uploads and use that when listing
images.
The "image-list" CLI is unable to use this cache, and is already as
fast as it can be given the number of ZK requests it needs to issue.
The same is true for "dib-image-list", and this change adds a cache
for that as well.
Finally, since we otherwise would have three or four nearly identical
cache implementations, the TreeCache system is refactored into a base
class that can be used for all the caches. It has some hook points
to deal with the very slight behavior differences.
Change-Id: Ibff0a9016936b461eccb1b48dcf42f5ad8d8434e
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
Image build requests can now be retrieved through the /dib-request-list
endpoint or via the dib-request-list sub-command. The list will show the
age of the request and if it is still pending or if there is already a
build in progress.
Change-Id: If73d6c9fcd5bd94318f389771248604a7f51c449
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 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
Previously launchers were identify as:
hostname-pid-provider-pool
The problem with this setup is that using containers with host
networking and namespaced pids you can end up with multiple launchers
using the same launcher id for the same provider pool. This problem also
arises if you are replacing launcher1.example.com with
launcher1.otherexample.com.
To fix this we update the launcher ids to be:
fqdn-provider-pool-randomuuid
These ids are already not expected to survive beyond the lifetime of any
single process as they use a pid value. This means we can replace this
pid value with a randomly generated uuid value instead. We also use the
host fqdn instead of hostname to be less ambiguous in the case where two
different hosts with different fqdns share a hostname.
Change-Id: I419718e63b31b12d8dfe971031cd8a81ad582480
As with the parent it seems that we can end up with empty image build
records as well as empty image upload records. Handle the build case as
well so that we can still list image builds properly when in this state.
This logs the problem as well so that you can debug it further.
Change-Id: Ic5521f5e2d2b65db94c2a5794f474913631a7a1b
Multiple builders running cleanup of uploads in parallel can lead to a
race condition which results in empty image upload nodes in Zookeeper.
The problem is the creation of the image upload number lock inside the
image upload. The lock creation can re-created an already deleted image
upload w/o any data.
Later iterations will fail to parse the empty znode data.
It's not 100% clear how we arrive at this race condition, but we
definitely see multiple builders in the 'guarded' section trying to
delete an upload.
The following exception shows that there is some kind of race:
2020-06-23 11:00:45,225 ERROR nodepool.builder.CleanupWorker.0:
Exception cleaning up build <ImageBuild {'state': 'ready',
'state_time': 1592718469.360345, 'builder':
'nodepool-builder-2', 'builder_id':
'8e2c599b-fbfd-4d68-b8df-2fd2efdaa705', 'formats': 'qcow2,raw',
'username': 'Administrator', 'python_path': 'auto', 'id':
'0000013010', 'stat': ZnodeStat(czxid=129130944630,
mzxid=129131204208, ctime=1592712393474, mtime=1592718469363,
version=1, cversion=1, aversion=0, ephemeralOwner=0,
dataLength=214, numChildren=1, pzxid=129131204230)}> of image
example-image in provider <Provider example-provider>:
Traceback (most recent call last):
File "/opt/nodepool/lib/python3.6/site-packages/nodepool/builder.py", line 499, in _cleanupImage
self._cleanupProvider(provider, image, build.id)
File "/opt/nodepool/lib/python3.6/site-packages/nodepool/builder.py", line 292, in _cleanupProvider
self._deleteUpload(upload)
File "/opt/nodepool/lib/python3.6/site-packages/nodepool/builder.py", line 345, in _deleteUpload
upload.provider_name, upload.id)
File "/opt/nodepool/lib/python3.6/site-packages/nodepool/zk.py", line 1572, in deleteUpload
self.client.delete(path, recursive=True)
File "/opt/nodepool/lib/python3.6/site-packages/kazoo/client.py", line 1341, in delete
return self._delete_recursive(path)
File "/opt/nodepool/lib/python3.6/site-packages/kazoo/client.py", line 1374, in _delete_recursive
self._delete_recursive(child_path)
File "/opt/nodepool/lib/python3.6/site-packages/kazoo/client.py", line 1376, in _delete_recursive
self.delete(path)
File "/opt/nodepool/lib/python3.6/site-packages/kazoo/client.py", line 1343, in delete
return self.delete_async(path, version).get()
File "/opt/nodepool/lib/python3.6/site-packages/kazoo/handlers/utils.py", line 75, in get
raise self._exception
kazoo.exceptions.NotEmptyError
Order of events that might have led to the above exception:
- A locks upload
- A deletes children of upload (including lock of A)
- B locks upload (possible, because lock was deleted by A)
- A tries do delete the parent node, which leads to the above exception
- ...
Related change: https://review.opendev.org/#/c/716566/
Change-Id: I5989c9f5f4c9f1615ae7372250e2e5fdad720256
After Ia94f20b15ab9cf2ae4969891eedccec8d5291d36 the hostname fields
are just used for informational purposes. Use the FQDN so something
like 'nb01.opendev.org' and 'nb01.openstack.org' are differentiated in
the output of something like 'nodepool dib-image-list'.
Change-Id: I1f061958ff271f707fddbe9ef74fd2e2a228e4ca
If you use a ipv6 literal as the host: parameter in zookeeper-services
section, it gets incorretly concatentated with the port when building
the host list. Detect ipv6 literals and quote them with rfc2732 []'s
when building the host string.
Change-Id: I49a749142e8031be37a08960bf645faca4e97149
Change the 'info' command output to include image upload data.
For each image, we'll now output each build and the uploads for the build.
Change-Id: Ib25ce30d30ed718b2b6083c2127fdb214c3691f4
Zuul is already creating node request containing the event id. To expand
the event id logging to nodepool we need to set and de-/serialize the
event_id field.
For node request that originate from within nodepool, we will generate a
random UUID as an event id.
Change-Id: I56b0bc18f3a55b8c3e3315a46b6276811e95a54f
This code is setting the ImageUpload state field to DELETING
and updating the data in ZooKeeper. However, it was using a new
ImageUpload object to do so, and we lose all other data, like the
external_id, external_name, etc. If the delete should fail in the
provider on the first attempt, the next attempt to delete will not
have these attributes, some of which (external_name) are needed to
actually delete the image. This means we leak the image in the
provider because the 2nd attempt will not try to delete in the
provider and will remove the ZooKeeper record.
During review of this fix, I realized that we were not locking
specific image uploads before modifying them or deleting them.
Although this likely would not cause any problems, it may have
caused the odd error in competing builder processes. This adds
a locking mechanism for that data, and tests to validate the lock.
Change-Id: I3fbf1c1a3ca10cc397facd4e939cbf4cbb2d8305
When we create a server using the openstack provider, in certain
conditions it is helpful to collect the host_id value. As an example,
Zuul will pass through this information into a job inventory file which
will allow an operator to better profile where jobs run with in a cloud.
This can be helpful trying to debug random jobs timing out within a
cloud.
Change-Id: If29397e67a470462561f24c746375b8291ac43ab
Signed-off-by: Paul Belanger <pabelanger@redhat.com>
This config option, available under each provider pool section, can
contain static key-value pairs that will be stored in ZooKeeper on
each Node znode. This will allow us to pass along abitrary data from
nodepool to any user of nodepool (specifically, zuul). Initially, this
will be used to pass along zone information to zuul executors.
Change-Id: I126d37a8c0a4f44dca59c11f76a583b9181ab653
This adds support to return cached data by the nodeIterator. This can
be done easily by utilizing the TreeCache recipe of kazoo.
Depends-On: https://review.openstack.org/616398
Change-Id: I23a992417d186b712864f2b00e79bc88bbfca967
Reorganizes the unit tests into a new subdirectory, and
adds a new functional test subdirectory (that will be further
divided into subdirs for drivers).
Change-Id: I027bba619deef9379d293fdd0457df6a1377aea8
James E. Blair