HDDS-7463. SCM Pipeline scrubber never able to cleanup allocated pipeline.

[ashishkumar50]

What changes were proposed in this pull request?

While creating pipeline we are using time from java.util time package. During pipeline scrub we should use same java.util time package for correct comparison. So that pipeline which is in allocated state for long time can be removed.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7463

Please replace this section with the link to the Apache JIRA)

How was this patch tested?

Code has been built locally and manually tested.

[adoroszlai]

Thanks @sumitagrawl for reporting the problem, @ashishkumar50 for the patch.

1. The same instance of MonotonicClock is used in several places, so the same problem may affect other services. We should fix all of them.
2. Clock was introduced here to improve the tests (quicker and more deterministic than sleep or waitFor). Reverting to Instant.now() eliminates those advantages. It also breaks TestPipelineStateManagerImpl, which relies on TestClock.

I think we should try to fix MonotonicClock, or replace it with SystemClock for production use.

[sodonnel]

I don't recall why I didn't use SystemClock, and instead created MonotonicClock. Perhaps I was trying to replace some calls to Time.monoticNow() that were originally there. Looks that way in the original PR:

#2425

Strange it has taken so long for this to be noticed, as we have been using it for a long time.

I wonder can we just replace all occurrences of MontonicClock with Java.time.SystemClock to fix this?

[sumitagrawl]

I don't recall why I didn't use SystemClock, and instead created MonotonicClock. Perhaps I was trying to replace some calls to Time.monoticNow() that were originally there. Looks that way in the original PR:

#2425

Strange it has taken so long for this to be noticed, as we have been using it for a long time.

I wonder can we just replace all occurrences of MontonicClock with Java.time.SystemClock to fix this?

As I have checked, SystemClock will give system time wrt January 1, 1970 UTC. The use will fix this problem.

And MontonicClock will give time epoc wrt system start -- This will be useful to measure performance / time taken for a method

@sodonnel @adoroszlai Do we need change the clock instance for this to SystemClock ? or replace MonotonicClock all places.

[adoroszlai]

I don't recall why I didn't use SystemClock, and instead created MonotonicClock. Perhaps I was trying to replace some calls to Time.monoticNow() that were originally there

I guess MonotonicClock is better for comparing instants within the same process. Any service that gets timestamps from other process via messages, or from its own serialized state, needs to avoid it.

[kerneltime]

We should not use Instant.now() but revert to using only monotonic clocks in all places.

[sumitagrawl]

We should not use Instant.now() but revert to using only monotonic clocks in all places.

We need use SystemClock() as object inside method, because pipeline Time is serialized to DB also. But clock instance is also used for checking time taken, but way of usecase.

In Recon, its using Instance.now().

So,

1. create a new clock as system clock and with existing Monotonic clock, use systemClock for this usages
2. Same change for Recon also to remove Instance.now()

OR current changes in PR is ok as similar to Recon.

Please suggest the changes ....
@kerneltime @adoroszlai @sodonnel

[sodonnel]

I think we should replace MonotonicClock with SystemClock anywhere it is created and passed into objects to be used. It will achieve the intended goal and be more safe to use.

[sodonnel]

We should not use Instant.now() but revert to using only monotonic clocks in all places.

We need use SystemClock() as object inside method, because pipeline Time is serialized to DB also. But clock instance is also used for checking time taken, but way of usecase.

In Recon, its using Instance.now().

This is because we didn't change all usage of time to the new model. We have been fixing things on a case by case basis, and trying to ensure new code follows the new pattern.

1. create a new clock as system clock and with existing Monotonic clock, use systemClock for this usages

2. Same change for Recon also to remove Instance.now()

OR current changes in PR is ok as similar to Recon.

Existing change is not OK. It also breaks the tests. Lets just try to replace all occurrences on MonotonicClock in the project with java.util.SystemClock instead. It should be a drop in replacement.

Intellij reckons there are 24 occurrences of it through the code base, which isn't too many to fix.

[kerneltime]

@sodonnel is java.util.SystemClock guaranteed to be monotonic; I do not quite understand the need to not use the monotonic clock. I would assume, we want to replace all instances with a clock that does not time travel.

[sodonnel]

@kerneltime The monotonic clock as it stands cant be used to compare times across JVM restarts. Its starting point is some meaningless instance that can be used to compare duration within the same JVM instance. I feel this could result in more unexpected bugs like this one.

If not SystemClock, then what do you suggest?

[ashishkumar50]

Hi @kerneltime @sodonnel please suggest the change to be done for this issue.

[sodonnel]

For PipelineManager we need to switch from MonotonicClock to an instance of java.util.SystemClock instead.

Feels like going forward with Monotonic clock in other places is a risk too, but for now we can fix the immediate problem as above.

[sodonnel]

LGTM - we can commit after a green CI run.