Flyteadmin digest comparison should rely on database semantics

[popojk]

Tracking issue

Closes #4780

Why are the changes needed?

In current TaskManager CreateTask code, FlyteAdmin checks if a task with the same ID already exists in the database. If it does, FlyteAdmin verifies whether the registered task has a different digest compared to the existing task. If no task with the same ID is found in the database, FlyteAdmin proceeds to create the task in the database.

However, the current approach may lead to a race condition that prevents the digest comparison for two identical tasks from occurring. For example, consider two identical tasks (tasks with the same ID and digest), A and B, being registered to FlyteAdmin simultaneously. It is likely that the digest check will be skipped because the existing task is not yet present in the database. Consequently, one task will be created in the database, and the other will fail due to a primary key conflict. (Refer to the diagram below for a better understanding.)

What changes were proposed in this pull request?

1.Do digest check in a transactional way:

The procedure of creating task should be 1. create task -> 2. if task id exists already(pramary key conflict) -> 3. do digest check. The pseudocode could look like

in the transaction:
try:
  create a task with given primary key
except:
  primary key already exists
  get existing entry with identical primary key
    if digest of existing == new entry's digest -> return `NewTaskExistsIdenticalStructureError`
    else -> return `NewTaskExistsDifferentStructureError`

In this way we can make sure that task digest will be checked even though 2 identical task registered at the same time frame. Refer to the diagram below for a better understanding.

2.Write Task to DB before write Description in TaskRepo Create method:

In current TaskRepo Create method, task description is created before task. However, if TaskManger catches primary key conflict error from task description creation and try to get existing task in DB for digest check, a task not found error could possibly occurred as task is not yet created in DB, which does not make sense for user. In this PR it is proposed to write Task to DB before write Description in TaskRepo Create method.

How was this patch tested?

Set up a simple workflow with 2 tasks

Write a shell script to request task registration 10 times at the same time to simulate hi concurrency situation. It is expected that each task will be registered successfully once only, otherwise the response message should shown AlreadyExists.

The result show each task only registered once as we expected

Then, we make a shell script to register 2 groups of tasks with same ID but different digest at the same time. It is expected that TaskExistsDifferentStructureError will shown in the response

The error shown as expected

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

[codecov]

Codecov Report

Attention: Patch coverage is 63.63636% with 8 lines in your changes missing coverage. Please review.

Project coverage is 37.10%. Comparing base (0585fba) to head (e717145).

Files with missing lines	Patch %	Lines
flyteadmin/pkg/manager/impl/task_manager.go	60.00%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6058   +/-   ##
=======================================
  Coverage   37.10%   37.10%           
=======================================
  Files        1318     1318           
  Lines      132326   132332    +6     
=======================================
+ Hits        49099    49102    +3     
- Misses      78955    78957    +2     
- Partials     4272     4273    +1     

Flag	Coverage Δ
unittests-datacatalog	51.58% <ø> (ø)
unittests-flyteadmin	54.09% <63.63%> (-0.01%)	⬇️
unittests-flytecopilot	30.99% <ø> (ø)
unittests-flytectl	62.29% <ø> (ø)
unittests-flyteidl	7.23% <ø> (ø)
unittests-flyteplugins	53.82% <ø> (ø)
unittests-flytepropeller	42.63% <ø> (ø)
unittests-flytestdlib	57.59% <ø> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.