Skip to content

Conversation

@bupd
Copy link
Contributor

@bupd bupd commented Jan 6, 2025

Proposal for Single Active Replication feature.

Discussion & PR: goharbor/harbor#21347

@reasonerjt
Copy link
Contributor

@bupd Thanks for the proposal, I think this is a valid scenario and the design in UX makes sense to me.

However, in this design, it lacks the details in terms of how to implement it in the backend.

I think more details are needed like:

  • Will we need a global lock for harbor-core, or jobservice?
  • How to ensure the lock is functional when there are multiple instance of harbor-core?
  • What is the data structure of this lock?
  • The code in which file or package needs to be updated to trigger the "lock" "unlock" action.
  • How does it fail-over, for example if a lock is added and failed to unlock, what's the impact and how to mitigate it.

Copy link
Contributor

@reasonerjt reasonerjt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valid feature enhancement but this design needs more details.

@bupd
Copy link
Contributor Author

bupd commented Jun 11, 2025

I think more details are needed like:
Will we need a global lock for harbor-core, or jobservice?

No, we are checking the currently running replications from DB and that does not require locks.

How to ensure the lock is functional when there are multiple instance of harbor-core?

Invalid since there are no locks present.

What is the data structure of this lock?
The code in which file or package needs to be updated to trigger the "lock" "unlock" action.
How does it fail-over, for example if a lock is added and failed to unlock, what's the impact and how to mitigate it.

The above are Invalid, Since there is no lock happening on DB, core or jobservice. we are just checking the current replication executions and check if the previous replication executions are completed before starting a new one.

Thanks @reasonerjt I will update proposal. clearly stating that no locks/unlocks are involved.

@reasonerjt
Copy link
Contributor

reasonerjt commented Jun 11, 2025

No, we are checking the currently running replications from DB and that does not require locks.

I don't quite understand how you can identify the ongoing replicated artifacts, by simply checking the DB. Could you add more details?

I'll also double check the code to refresh my memory about details replication

@bupd
Copy link
Contributor Author

bupd commented Jun 11, 2025

No, we are checking the currently running replications from DB and that does not require locks.

I don't quite understand how you can identify the ongoing replicated artifacts, by simply checking the DB. Could you add more details?

I'll also double check the code to refresh my memory about details replication

There was a mistake in wording its policy and not artifact. This feature is more into not running the replications on the same policy when a replication execution is already running.

@reasonerjt
Copy link
Contributor

reasonerjt commented Jun 25, 2025

Overall, I think the proposal needs to be updated to reflect the actual problem and specific scenario it solves.

Additionally, @bupd IMO the Abstract, Background, Motivation, Use cases, Benefits may be combined into 2 sections. As they are all trying to explain what problem you wanna solve.

Copy link
Contributor Author

@bupd bupd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe all concerns have been addressed, and the PR proposal is now ready for merge.
Since we're nearing the 2.14 release, I suggest we avoid further nitpicks unless they're critical.

I would love to see this feature make it into 2.14.

Thank you all for your time and support!

@reasonerjt
Copy link
Contributor

reasonerjt commented Jul 22, 2025

Thank you @bupd

- If a execution with running status is found, a new replication job is created but **marked as "skipped"** instead of replication being executed normally.

This is not a nitpick, this is not OK when you say "a new replication job is created", but you mean "a record will be inserted to execution table". They are different objects per my understanding. It can confuse the reader and make him think a job will be created in jobservice, but in your case this is not what you are gonna do.

Signed-off-by: bupd <[email protected]>

Update wording

Signed-off-by: Prasanth Baskar <[email protected]>

update & add more technical details

Signed-off-by: Prasanth Baskar <[email protected]>

update wording

Signed-off-by: Prasanth Baskar <[email protected]>

add out of scope

Signed-off-by: Prasanth Baskar <[email protected]>

improve wording in proposal

Signed-off-by: Prasanth Baskar <[email protected]>

fix: wording

Signed-off-by: Prasanth Baskar <[email protected]>

Update single-active-replication.md

Signed-off-by: Prasanth Baskar <[email protected]>
@reasonerjt reasonerjt merged commit 697cdd4 into goharbor:main Jul 22, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants