openstack/swift - swift - OpenDev: Free Software Needs Free Tools

The 'states' argument of get_shard_ranges() should be a list of ints,
but previously just a single int was tolerated. This was unnecessary
and led to inconsistent usage across call sites.
We'd like similar ContainerBroker methods, such as the anticipated
get_namespaces() [1], to have an interface consistent with
get_shard_ranges(), but not continue the unnecessary pattern of
supporting both a list and a single int argument for 'states'. This
patch therefore normalises all call sites to pass a list and
deprecates support for just a single int.
[1] Related-Change: If152942c168d127de13e11e8da00a5760de5ae0d
Change-Id: I056cefbf0894dbc68b9a6eb3d76ec4dc0a72de0d

The setUp method creates a timestamp iterator, so let's use it
consistently in all the tests.
Change-Id: Ibd06b243c6db93380b99227ac79157269a64b28a

As per the current release tested runtime, we test
python version from 3.8 to 3.11 so updating the
same in python classifier in setup.cfg
Change-Id: If270236c2b23f432e1a3e1508101a7cc86bbd73b

The allowed_digests option were added to the formpost middleware in
addition to the tempurl middleware[1], but the option was not added to
the formpost section in the example proxy config file.
[1] 2d063cd61f
Change-Id: Ic885e8bde7c1bbb3d93d032080b591db1de80970

If a method is not allowed, real swift proxy server app will return an
HTTPMethodNotAllowed response, whereas FakeSwift would previously
*raise* HTTPNotImplemented. S3Api deliberately sends requests with
method 'TEST' which is not allowed/implemented. To workaround the
difference in real and fake swift behaviour, FakeSwift was configured
to allow the 'TEST' method, and then in some tests an
HTTPMethodNotAllowed response was registered for 'TEST' requests!
This patch modifies FakeSwift to return an HTTPMethodNotAllowed
response to the incoming request when the request method is not
allowed.
It is no longer necessary for FakeSwift to support extending the
default list of allowed methods.
Change-Id: I550d0174e14a5d5a05d26e5cbe9d3353f5da4e8a

We remove s3api.FakeSwift and replace it with the "normal" FakeSwift.
Additionally the @s3acl decorator is removed and replaced with an
inheritance based pattern. This simplifies maintenance using more
familiar patterns and improves debugging.
Co-Authored-By: Clay Gerrard <clay.gerrard@gmail.com>
Change-Id: I55b596a42af01870b49fda22800f7a1293163eb8

The ?format=raw TestCase has it's own manifest setup and doesn't do any
segment validation. It's manifests are not suitable for use in other
TestCases.
Change-Id: Idf4b72bb59b8bf7232236ca544a3317b6e2e08fd

The Namespace class grew account/container properties to make them
easier to use in the proxy and subjected to similar consistency
requirements as the ShardRange's properties in the related change.
There are no new assertions added in this change, it merely
consolidates the py2/py3 validating helper which was duplicated
between the Namespace and ShardRange TestCases.
Related-Change-Id: Iebb09d6eff2165c25f80abca360210242cf3e6b7
Change-Id: Ide7f1dd3d9c664fb57c47dcd50edb44ae90ff5f9

When constructing an object listing from container shards, the proxy
would previously return the X-Backend-Record-Type header with the
value 'shard' that is returned with the initial GET response from the
root container. It didn't break anything but was plainly wrong.
This patch removes the header from object listing responses to request
that did not have the header. The header value is not set to 'object'
because in a request that value specifically means 'do not recurse
into shards'.
Change-Id: I94c68e5d5625bc8b3d9cd9baa17a33bb35a7f82f

ShardRange.name is required to have the form <account/container>. We'd
like to be able to replace ShardRange instances with the Namespace
superclass but still have the convenience of the account and container
accessors.
The name is stored as a single attribute and split when accessing via
the account and container getters, rather than splitting into two
attributes in the name setter, to minimise the overhead of
constructing Namespace instances. Where performance can be critical
(e.g. fetching the entire set of namespaces from a container server)
the number of Namespace instances constructed can be much greater than
the number whose account and container properties are used. The author
found that splitting in the account and container getters became more
efficient than splitting in the name setter when the rate of
constructing instances was ~2x greater than the rate of calling the
account and container getters.
The account and container property setters are removed from the
ShardRange class. The name setter is removed from the Namespace class.
These setter were never used.
Change-Id: Iebb09d6eff2165c25f80abca360210242cf3e6b7

Modern tests mostly consistently use a debug_logger when available and I
find the assertions a little more intuative to read than Mock's
assert_calls helpers.
Related-Change-Id: I13a24c4a0cd4045ab2fffb18067674b3e3bf742b
Change-Id: Ia9eeae6ed231a9be504967208efa3a0124f8cf09

Add a TestCase for the GetOrHeadHandler class and relocate existing
unit tests for that class from TestFuncs.
No changes other than lines moving.
Change-Id: I13a24c4a0cd4045ab2fffb18067674b3e3bf742b

During an MPU complete, the s3api first PUTs an SLO manifest and then
DELETEs the upload marker in <bucket>+segments. If the proxy's clock
is slow relative to the proxy that created the upload marker, the DELETE
will fail with a 409.
Previously the 409 would be returned to the client, which for some
clients (e.g. aws cli) does not trigger a retry. Both the manifest and
upload marker would remain, which would cause the upload to continue
to be listed as if still "in progress". Worse, since the complete
failed, the client might delete the segments, leaving the manifest in
place but with no segments.
This patch adds a "look-before-you-leap" pre-check on the upload
marker timestamp. If the marker is found to be in the future then
neither the manifest PUT nor the marker DELETE are attempted, and the
client receives a 503 response. If the pre-check passes but somehow
the marker DELETE still fails with a 409, the client will now receive
a 503 that will hopefully trigger a retry.
Closes-Bug: #2045046
Change-Id: Ie2e0cb75425e00cff533014af6b6fafad89bff94

Change-Id: I303a1ff97325654478c2a47af056deda37696b7b

Add sub-type timing metrics for container PUT/GET, includes below new metrics:
Container PUT: PUT_object, PUT_shard, PUT_container
Container GET: GET_object, GET_shard
Change-Id: Ifa2341b11b8244cad29fca574a13cf31134a4a0d

...and rename to _get_updating_shard_ranges.
The method is only used by the ObjectController, and it is only used
to fetch shard ranges in the 'updating' states.
Also relocate the associated unit tests.
Change-Id: I083e0c6898bf93d8a0dc593acd9723827e55508e

The per-service option was deprecated almost 4 years ago[1].
[1] 4601548dab
Change-Id: I45f7678c9932afa038438ee841d1b262d53c9da8

Create new helper functions to set and get namespaces in cache. Use
these in both the object and container controllers when caching
namespaces for updating and listing state shard ranges respectively.
Add unit tests for the new helper functions.
No intentional behavioural changes.
Change-Id: I6833ec64540fa19f658f0ee78952ecb43b49f169

The proxy_logging middleware needs an X-Backend-Storage-Policy-Index
header to populate the storage policy field in logs, and will look in
both request and response headers to find it.
Previously, the s3api middleware would indiscriminately copy the
X-Backend-Storage-Policy-Index from swift backend requests into the
S3Request headers [1]. This works for logging but causes the header
to leak between backend requests [2] and break mixed policy
multipart uploads. This patch sets the X-Backend-Storage-Policy-Index
header on s3api responses rather than requests.
Additionally, the middleware now looks for the
X-Backend-Storage-Policy-Index header in the swift backend request
*and* response headers, in the same way that proxy_logging would
(preferring a response header over a request header). This means that
a policy index is now logged for bucket requests, which only have
X-Backend-Storage-Policy-Index header in their response headers.
The s3api adds the value from the *final* backend request/response
pair to its response headers. Returning the policy index from the
final backend request/response is consistent with swift.backend_path
being set to that backend request's path i.e. proxy_logging will log
the correct policy index for the logged path.
The FakeSwift helper no longer looks in registered object responses
for an X-Backend-Storage-Policy-Index header to update an object
request. Real Swift object responses do not have an
X-Backend-Storage-Policy-Index header. By default, FakeSwift will now
update *all* object requests with an X-Backend-Storage-Policy-Index as
follows:
 - If a matching container HEAD response has been registered then
 any X-Backend-Storage-Policy-Index found with that is used.
 - Otherwise the default policy index is used.
Furthermore, FakeSwift now adds the X-Backend-Storage-Policy-Index
header to the request *after* the request has been captured. Tests
using FakeSwift.calls_wth_headers() to make assertions about captured
headers no longer need to make allowance for the header that FakeSwift
added.
Co-Authored-By: Clay Gerrard <clay.gerrard@gmail.com>
Closes-Bug: #2038459
[1] Related-Change: I5fe5ab31d6b2d9f7b6ecb3bfa246433a78e54808
[2] Related-Change: I40b252446b3a1294a5ca8b531f224ce9c16f9aba
Change-Id: I2793e335a08ad373c49cbbe6759d4e97cc420867

Change-Id: I8dac31e302031d4318890bb87bf5a26889371766

This patch reorganizes the SLO read response handling. The main goal
was to push the response header replacement for both GET/HEAD SLO and
multipart-manifest=get paths all into a common return path. A new
RespAttrs primitive is used to carry around some metadata details from
requests made in SLO. The authors hope these changes make the code more
easily readable and easier to modify.
Drive-By: add new "friendly_close" function in common.utils so we can
drain empty/error responses more confidently (and use it in swob and
request_helpers).
Drive-By: the tests added in the Related-Change discovered a 500 on
If-[Un]Modified-Since conditional GET requests - it probably wasn't
important, but this refactor fixed it on accident as a side effect.
Closes-Bug: #2040178
Co-Authored-By: Alistair Coles <alistairncoles@gmail.com>
Co-Authored-By: Ashwin Nair <nairashwin952013@gmail.com>
Related-Change-Id: I54094f3d2098f56b755ec19cc9315d06a6ca8b15
Change-Id: Idc84e70539fc7480b6ecb86e2f0da904baf2c727

Talk about graceful shutdowns, seamless reloads, and the swift-reload helper.
Change-Id: I9c86d1ce9853840d73e2107a6d79b2454c21438a

Change-Id: If06d4987adda873aad3bc6cf534db09d69c7683e

Ensure name/account/container are always consistent and always encode
utf8 in py2.
Co-Authored-By: Alistair Coles <alistairncoles@gmail.com>
Co-Authored-By: Matthew Oliver <matt@oliver.net.au>
Change-Id: Ia5374f55adf80fef92a92d916b3f89297463c673

We've been writing SLO manifests with size/etag sysmeta for more than 5
years, but we want our tests and code to continue to support the legacy
format forever. This test infra refactor will make that easier for test
authors to opt-in testing of legacy manifests by reusing a common
pattern for manifest setup across tests.
This consolidation also cleans up some duplication where two TestCases
had identical manifest setup and paves the way to more tidying of
similar (but slightly different) manifest setup across TestCases and
sharing of setup across future TestCases.
This manifest setup standardization also adopts a consistent naming
scheme for manifest sysmeta values so test assertions are easier to read
as correct at a glance (e.g. slo_etag vs json_md5)
Additionally leak tracking is added to the common base; SLO was already
really good about *closing* requests, but in many cases seems to not
bother reading/draining them (even when they might be empty/small).
As part of the leak tracking investigation a couple new tests were added
to explore the behavior of SLO's SegmentedIterable in the
request_helpers module.
Drive-By: Fix SegmentedIterable docstring: the constructor has
expected an iterable yielding dicts, not tuples, since the
Related-Change [2].
Drive-By: remove FakeSwift's now unused "register_responses" interface
and provide "register_next_response" as a replacment. This allows test
authors to extend the registered response for a given request key from a
common test setup into a "series of registered responses" by expressing
just the new/next response rather than forcing them to duplicate the
initial response in the explicit list passed to "register_responses".
Related-Bug: #2040178
Co-Authored-By: Alistair Coles <alistairncoles@gmail.com>
[1] Related-Change: Ia6ad32354105515560b005cea750aa64a88c96f9
[2] Related-Change-Id: Ib8dc216a84d370e6da7d6b819af79582b671d699
Change-Id: I54094f3d2098f56b755ec19cc9315d06a6ca8b15

Previously, if a memcache set command had an unexpected response then
an error was logged but no exception raised from MemcacheRing.set.
With this patch, when MemcacheRing.set fails in this way, a
MemcacheConnectionError will be raised if 'raise_on_error' is
True. This exception is now handled when the proxy tries to set
updating namespaces in memcache, and a counter metric emitted with
name 'object.shard_updating.cache.set_error'. If the set is successful
an 'object.shard_updating.cache.set' counter metric is emitted.
The logging of MemcacheRing errors is improved to include the memcache
key being set, in line with exception logging.
Change-Id: Icfc97751c80f4bb4a3373796c01bff6ed5843937

Include more useful information (e.g. shard name and local db file) in
the warning logs when a cleaved shard DB fails to replicate to its
primary nodes.
Change-Id: I830f67a611c08328d0c4ec71eba0c52cf9b4e1e5

If it is passed a 'seconds' arg, the eventlet Timeout *constructor*
schedules the timeout to be raised after that many seconds, even
though the Timeout context manager has not need been entered.
A unit test (test.unit.proxy.controllers.test_obj.TestECFragGetter.
test_iter_bytes_from_response_part_read_timeout) was constructng a
ChunkReadTimeout and passing seconds=9. The test happens to be one of
the last to run, so the timeout fires during the pytest runner exit
rather than another test:
Error in atexit._run_exitfuncs: Traceback (most recent call last):
File
"/home/vagrant/.tox-swift/py38/lib/python3.8/site-packages/eventlet/greenthread.py",
line 36, in sleep hub.switch() File
"/home/vagrant/.tox-swift/py38/lib/python3.8/site-packages/eventlet/hubs/hub.py",
line 313, in switch return self.greenlet.switch()
swift.common.exceptions.ChunkReadTimeout: 9 seconds
The seconds value was irrelevant to the test other than to provide a
non-empty string representation of the Timeout.
Related-Change: I76ea042ef4b3f5cc1caa4774e35d6191f4ca548e
Change-Id: I5164a7e1d01a0f54983967f0db872c05928f904d

Sharded DBs that have been completely emptied of objects will not be
removed if auto-sharding is disabled. The shards remain in place and
the sharder will repeatedly warn of a 'Reclaimable db stuck waiting
for shrinking'. This can occur often if expiring objects containers
become sharded, since these containers will always, by design, be
emptied. Each time this happens, another warning log starts to be
repeatedly emitted.
This patch reduces the rate of this specific warning to once per 24
hours.
Change-Id: I493cd661fe98817ca5fd2daf2a681fdf69e28923

See https://www.freedesktop.org/software/systemd/man/sd_notify.html#Description
for more information.
Note that this requires that we keep the NOTIFY_SOCKET env var
around for more than just the first READY message, so we want to be
careful about when we're sending the default "READY=1".
UpgradeImpact
=============
Since prior versions of Swift would unset the NOTIFY_SOCKET env var,
services must be fully restarted (rather than seamlessly reloaded) to
emit the new messages.
Related-Change: Ice224fc2a6ba0150be180955037c13fc90365479
Change-Id: I201734ae0d6232ecb1923e67864dd928f90b6586

Previously, WSGI server systemd unit files might have used something like
 ExecReload=kill -USR1 $MAINPID
This was risky; in the related change, reloads were made safer, but
required more than one ExecReload line. Meanwhile, systemd docs
(https://www.freedesktop.org/software/systemd/man/systemd.service.html#ExecReload=)
say
> It is strongly recommended to set ExecReload= to a command that
> not only triggers a configuration reload of the daemon, but also
> synchronously waits for it to complete.
which *neither* set of ExecReloads would do.
Now, add a new swift-reload command which, given a pid,
 * validates that the PID seems to belong to a Swift WSGI server
 manager process,
 * checks that the config used by that PID is still valid,
 * signals the PID to perform a seamless reload, and
 * waits for the reload to complete by monitoring the PID's children.
As a result, WSGI server systemd unit files can now use something like
 ExecReload=swift-reload $MAINPID
to follow systemd recommendations.
Change-Id: Ifcadd2f8427f107aae1921cdd311f7973b0312e1
Related-Change: I9e5e158ce8be92535430b9cabf040063f5188bf4

Related-Change: Icff8cf57474dfad975a4f45bf2d500c2682c1129
Change-Id: Ic2283fab0d18ea03c6beb353c6b934344606c15e