Support task resource tracking in OpenSearch

[tushar-kharbanda72]

Description

• Added framework support for tracking resource utilisation by tasks.
• Currently, SearchTask and SearchShardTask supports resource tracking. Can be easily extended to any other task if required in future.
• Dynamic setting support is added to enable and disable the feature at runtime.
• List Tasks API now makes sure it refreshes the running tasks resource info before returning response.

Issues Resolved

#1179

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[opensearch-ci-bot]

❌   Gradle Check failure d023569a4cd8ec1f4e820d56a141fcfd7dc47489
Log 3842

Reports 3842

[opensearch-ci-bot]

❌   Gradle Check failure d4a085e20f3c5a9a9aa91ed64f4a39446d651bec
Log 3843

Reports 3843

[tushar-kharbanda72]

@dblock @andrross @Bukhtawar Could you please review this PR. Thanks

[opensearch-ci-bot]

❌   Gradle Check failure 8ac708a5d59beea160f13dd77ca26d0398b84341
Log 3851

Reports 3851

[Bukhtawar]

Thanks Tushar. The changes look good overall. I'll continue with the full review in sometime but below are some early comments I have.

[Bukhtawar]

We should add task in thread context centrally as a part of task creation, instead of doing it at multiple places after task creation, it could lead to maintenance overhead overtime

[tushar-kharbanda72]

This is done as per the ThreadContext's usage documentation which is something like :

1. Clone/Stash existing thread context
2. Add new fields to the new thread context.
3. Execute the operation (or take task to a different thread)
4. Restore the original thread context.

If we want to do it in register method then we would have to face challenges of making ThreadContext mutable which today throws exception and will also need to find a common place to remove the header from the thread context so that some other request doesn't use this just because we missed to remove it from thread context.

[Bukhtawar]

Lets just call out this works for tasks that forks off on a separate thread(non-network thread)

[reta]

@tushar-kharbanda72 I would like to back @Bukhtawar concerns, manipulating thread context in a such away is not desirable (cause essentially someone down the line could override it again). The suggestion I have in mind is to introduce the distinction between Runnable (no task context) and TaskRunnable extends Runnable (with Task ID as final property). In this case, the taskId could be extracted by thread pool and baked into the thread local context from the start (by checking if Runnable is instance of TaskRunnable). What do you think?

[tushar-kharbanda72]

@reta Agree with the problem you mentioned of Task Id being overridden. The solution for that I think could be to always preserve Task Id when a thread context stash is called and always make sure it makes it to the new context. Thoughts on this?

Yes having taskId available in TaskRunnable without ThreadContext is desirable but I don't seem to currently have a way to get that information for all use cases in OpenSearch without having changes at multiple places in codebase. I believe this essentially translates to having a task id available where ever we are submitting any runnable to an executor. Right?

[reta]

@tushar-kharbanda72 yes, it is possible to manage task id transfer using stash(), but fundamental problem here (in my opinion) - the thread context should not be manipulated through TaskManager from the outside. Yes, it will introduce larger scope of changes, the benefit - less dependency on thread context (black box), explicit link between task and its execution model, just my 5 cents, @Bukhtawar do you have an option?

[andrross]

fundamental problem here (in my opinion) - the thread context should not be manipulated through TaskManager from the outside

I agree. This is essentially using the thread context to stash a global variable and is likely to lead to pain in the long term.

[tushar-kharbanda72]

@reta @andrross Currently coupling the thread context update with when a task gets registered in resource tracking service as both together made more sense.

Having task id set as a member in the runnable is an ideal case scenario but comes with it's own sets of challenges:

• Submitting a runnable will not be very flexible. We would need Task Id available as a function param at every point throughout the code base wherever we need to submit a runnable.
• If a blocking sync child action gets executed on the same thread - this would essentially mean updating the runnable's task id in some way and reverting it back as soon as it finishes.
• For new features folks would need to handle the overlapping case of making task id available and I see this would increase the number of params they handle in the function.

I see the benefits of the suggested approach but it also comes with the above mentioned downsides and these could be even more when we deep dive further.

Also, similar approach is used for request tracing X-Opaque-Id in OpenSearch which I see a more critical feature for some users.

@Bukhtawar Would like to hear your thoughts on this as well.

[reta]

@tushar-kharbanda72 sorry for the delay, I certainly agree it is challenging to have robust and reliable context propagation support but this is also very important to avoid any surprises by opening the gate for (arbitrary) context manipulations. The approach could be generalized by eliminating the focus on task to abstract runnable with contextual setup: in this case the thread context could be established by runnable (fe, associated with task or anything else) before hand.

There are quite a few examples of contextual runnables like ContextPreservingAbstractRunnable and ContextPreservingRunnable.

X-Opaque-Id is also a good example of context propagation but it is passed through requestHeaders, may be we could extend ThreadContextStruct with non-headers key/value pairs, task id being one of them. I will try to prototype the possible implementation(s) shortly and share with you.

[tushar-kharbanda72]

@reta Created an issue to discuss further on this #2819 (comment). We can use this to close the final change required for making task id available to runnable.

I believe this can be added in a follow up PR as it's not changing the core logic and just the way runnable knows about the Task. Do let me know your thoughts.

[dblock]

@dblock @andrross @Bukhtawar Could you please review this PR. Thanks

Thanks for jumping in @Bukhtawar! Will leave this to you for now until you're happy with the PR and we can have more 👀 on it later.

[opensearch-ci-bot]

❌   Gradle Check failure ba1116110b2dad43043138c9f199dd2a17b1f693
Log 3972

Reports 3972

[opensearch-ci-bot]

❌   Gradle Check failure 61a6e76d079d0eed9e9ca669d4268a6427dd5805
Log 3973

Reports 3973

[opensearch-ci-bot]

❌   Gradle Check failure 8a187522ac9a20b0f90831048e095649fa5adda8
Log 3974

Reports 3974

[opensearch-ci-bot]

❌   Gradle Check failure a0e291e588a3e4c6a76bc1551f97924d1ee230e1
Log 3977

Reports 3977

[opensearch-ci-bot]

❌   Gradle Check failure 36d2a5238399791e3e1bae339cecaea400a3c20b
Log 4195

Reports 4195

[opensearch-ci-bot]

❌   Gradle Check failure b92f5e3aea8b947cc35049b386167244e4c5e246
Log 4196

Reports 4196

[opensearch-ci-bot]

❌   Gradle Check failure da2220dc2b957a1ec3420a400612adc108d476fe
Log 4197

Reports 4197

[opensearch-ci-bot]

❌   Gradle Check failure f6d43f905da4194e190fcbd35d6349adce96b9e4
Log 4198

Reports 4198

[opensearch-ci-bot]

❌   Gradle Check failure 4b71cbaef54f81c9bf9298d5ecb1525a552210b4
Log 4200

Reports 4200

[opensearch-ci-bot]

❌   Gradle Check failure 8c59e526942cfc413733d3b6c5a6db7a2927417c
Log 4271

Reports 4271

[opensearch-ci-bot]

❌   Gradle Check failure cbcf3c525bf516fb7164f0221491a7b25c1f96ec
Log 4275

Reports 4275

[opensearch-ci-bot]

✅   Gradle Check success 4eb49c56c1c155b30f374e05bbd24efea5645283
Log 4276

Reports 4276

[dblock]

This was reverted in #3046

[Bukhtawar]

@dblock can you point me to the build failure links?

[Bukhtawar]

Looks like this is the only assertion that is tripping

 java.lang.AssertionError: No thread should be marked active when task finishes

[dblock]

@Bukhtawar There are a few recent mentions of this in issues linked off #3046

[tushar-kharbanda72]

@dblock Will take a look at assertion failures and resolve the issue this week.