[
https://issues.apache.org/jira/browse/HDDS-17404?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17404:
------------------------------
Attachment: TestBugMC2RenewLostToExpiry.java
> LeaseManager monitor can expire a lease that was re-acquired or renewed after
> it found the old deadline passed
> --------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17404
> URL: https://issues.apache.org/jira/browse/HDDS-17404
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 0.2.1, 0.3.0, 0.4.0, 0.4.1, 0.5.0, 1.0.0, 1.1.0, 1.2.0,
> 1.3.0, 1.2.1, 1.4.0, 2.0.0, 1.4.1, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1
> Reporter: Siyao Meng
> Priority: Minor
> Attachments:
> MC-1-expire-only-the-timed-out-lease-atomically-with-renew.patch,
> TestBugMC1ExpiryRemovesReacquiredLease.java, TestBugMC2RenewLostToExpiry.java
>
>
> h3. Mechanism
> {{LeaseManager.LeaseMonitor.run}} decides expiry without the manager lock: it
> calls {{get(resource)}}, {{Lease.getRemainingTime}} and
> {{Lease.getCallback}}, and only then takes the lock in {{release(resource)}}.
> {{release}} removes whatever lease is stored under that resource at that
> moment and invalidates it. It never checks that this is the lease the monitor
> judged, or that its deadline is still in the past.
> {{Lease.renew}} takes no lock at all. It checks the non volatile {{expired}}
> flag and adds to {{leaseTimeout}}. The monitor does not read the timeout
> again before {{release}}.
> So two things can change between the monitor's decision and its commit:
> * the lease is released and the same resource is acquired again, so
> {{release}} removes the new lease;
> * the lease is renewed, so {{release}} removes a lease whose new deadline is
> in the future.
> h3. Trigger
> Any caller of {{LeaseManager}} that either releases and re-acquires the same
> resource, or renews a lease, close to the moment the monitor finds the old
> deadline passed. The window runs from the monitor's unlocked read until it
> gets the manager lock, so other callers doing {{acquire}} or {{release}} on
> the same manager make it wider.
> In tree, no production code does either at this commit.
> {{CloseContainerEventHandler}} only acquires; its leases are released only by
> the monitor and by {{shutdown}}. {{EventWatcher}} releases and re-acquires by
> event ID, but its only main subclass, {{RetriableDatanodeEventWatcher}}, is
> not instantiated in production code at this commit. Nothing outside tests
> calls {{Lease.renew}}. The defect is in the public API of
> {{hdds-server-framework}} and would affect any future caller of this kind.
> h3. Impact
> Reproduced on unmodified source through the public API:
> * *Re-acquired lease removed early.* A lease that is released and acquired
> again while the monitor is expiring the old one is removed and invalidated a
> few milliseconds after it was acquired, instead of at its 1000 ms deadline.
> {{get}} throws {{LeaseNotFoundException}}, {{renew}} throws
> {{LeaseExpiredException}}, the callback of the lease that was explicitly
> released runs once, and the new lease's own callback never runs.
> [^TestBugMC1ExpiryRemovesReacquiredLease.java]
> * *Successful renew dropped.* {{renew(60000)}} returns normally, the lifetime
> reads 60100 ms, and the monitor then invalidates the lease and runs its
> expiry callback; a second {{acquire}} of the same resource succeeds.
> [^TestBugMC2RenewLostToExpiry.java]
> A lost renew always lands after the monitor found the lease timed out. It
> returns normally, although the lease is then expired anyway.
> h3. Reproduction
> The two attached classes are self contained JUnit tests for
> {{hadoop-hdds/framework}} (target path and Maven command are in each class
> comment). They pass on unmodified source when the bug is present. Each holds
> the manager's own monitor, the lock that {{acquire}} and {{release}} already
> take, while the monitor thread is blocked on it to release the timed out
> lease, and does the release and re-acquire, or the renew, meanwhile. This
> only fixes an order that can also happen without help, so the tests show that
> the window exists, not how often it is hit. No state is injected and no code
> under test is changed.
> h3. Patch
> [^MC-1-expire-only-the-timed-out-lease-atomically-with-renew.patch], against
> f804bf26867246067017fdd9e5319c2cbdbd5db8. It also applies to master at
> a7da4f70195.
> The monitor now commits through a new private {{LeaseManager.expire(resource,
> lease)}}, which holds the manager lock and the lease's lock. It releases the
> lease only if it is still the active lease for the resource, compared by
> identity since {{Lease.equals}} compares only the resource, and its remaining
> time is still not positive. Otherwise the monitor runs no callback and scans
> again at once, so a renewed lease still expires at its new deadline.
> {{Lease.renew}} is {{synchronized}}, so it either completes before the
> monitor's check or fails after the invalidation. A renew that runs after the
> deadline but before the monitor removes the lease still extends it, as it
> already does today when it runs before the monitor's check; making it throw
> would break renews that work today. {{Lease.expired}} is {{volatile}}, so
> {{hasExpired}} and {{renew}} see an invalidation done by another thread.
> {{acquire}}, {{release}} and {{shutdown}} are unchanged, and callbacks still
> run on the executor, outside any lock.
> Covered by two new cases in {{TestLeaseManager}}:
> {{testExpiryKeepsLeaseReacquiredMeanwhile}} and
> {{testRenewDuringExpiryKeepsLease}}. The first also checks that the released
> lease's callback has not run once a later lease on the same resource has
> expired; the second checks that the renewed lease still expires at its new
> deadline with exactly one callback. Without the change both fail ("lease
> acquired after the release" and "lease renewed before it was expired":
> expected false but was true). With it {{TestLeaseManager}},
> {{TestEventWatcher}}, {{TestCloseContainerEventHandler}} and
> {{TestBlockManager}} pass (32 tests), checkstyle is clean, and both attached
> reproductions no longer show the bug.
> Found by TLA+ model checking and code review of the HDDS LeaseManager expiry
> and shutdown paths under HDDS-15926, on commit
> f804bf26867246067017fdd9e5319c2cbdbd5db8. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-8418 (removed an acquire variant that attached the callback
> after acquire, a different race) and HDDS-10899 (callback refactoring). The
> attached patch is a proposal for review. Generated with Specula (Claude Opus
> 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
