goharbor / harbor

An open source trusted cloud native registry project that stores, signs, and scans content.
https://goharbor.io
Apache License 2.0
23.81k stars 4.74k forks source link

Support for CockroachDB (don't use pg_advisory_lock) #8649

Open embik opened 5 years ago

embik commented 5 years ago

Is your feature request related to a problem? Please describe. We would like to use CockroachDB for a replicated / clustered database backend, possibily even for a multi-region setup of Harbor. Since CockroachDB speaks a dialect of PostgreSQL, this should be possible with some modifications.

The main issue with CockroachDB seems to be the lack of support for pg_advisory_lock() - CockroachDB will probably not support this function in the near future.

Describe the solution you'd like Implement CockroachDB support, e.g. by solving #8511 and #6942 and drop pg_advisory_lock() calls (maybe via a flag / configuration option). I'm honestly not sure if that's possible, I just wanted to document the main issue with CockroachDB support that we encountered.

Describe the main design/architecture of your solution

---

Describe the development plan you've considered

---

Additional context

This is what Harbor Core logs when you configure CockroachDB as target:

2019-08-13T10:11:44Z [ERROR] [/common/dao/pgsql.go:112]: Failed to upgrade schema, error: "try lock failed in line 0: SELECT pg_advisory_lock($1) (details: pq: unknown function: pg_advisory_lock())" 
KarstenSiemer commented 5 years ago

I'd highly suggest to put more effort into this if this is a highly requested feature. Since Cockroach always uses SERIALIZABLE isolation instead of a locking kind mechanism the implementation will need some customizations to be of highest quality. For anyone interested these sources will give you a quick intro: Isolation in CockroachDB Client-side Transaction Retries Build a Java App with CockroachDB(JDBC or Hibernate)

steven-zou commented 5 years ago

@embik @KarstenSiemer

Thank you very much for raising the requirement to the Harbor community. We'll check and prioritize it based on the release plans and resources.

If you want to accelerate the progress, you guys can promote it in the community and get more supports from the community to improve the priority.

reasonerjt commented 5 years ago

@embik @KarstenSiemer Thanks a lot for raising this.

The error is thrown by the migrator, it seems it supports cockroach DB: https://github.com/golang-migrate/migrate/tree/master/database/cockroachdb

We'll do some investigation to see if we can support that with minimal change in configuration.

linux-wizard commented 4 years ago

Hi, we want also to use CockroachDB for Harbor as we have Harbor clusters in 5 DCs across the world, and CockroachDb DC will be a perfect solution to consolidate our data from all DCs in 1 table

bearrito commented 4 years ago

I am in roughly the same position as @linux-wizard. There are numerous tools that all seem to want to follow their own pattern of putting a non-HA DB on a PVC. It isn't sustainable for everyone to follow this pattern from an operational perspective.

Many folks are very eager to adopt this solution. Being able to configure an external DB of various flavors is essential for production use cases.

msiebuhr commented 4 years ago

From a quick read of https://github.com/goharbor/harbor/blob/master/src/common/dao/pgsql.go#L24, this could be replaced by the ditto cockroachdb migrator (+ some re-naming of postgres to cockroachdb).

But there is quite a bit of code in that module, and I haven't checked if any/how much is postgres/sqlite aware to begin with. (Though cockroachdb is a fairly close cousin of postgres, so most things should just work).

nyarly commented 4 years ago

I'd love to see this implemented - running in Harbor in a pure-Kubernetes mode would be excellent, and the PostgreSQL dependency makes that unnecessarily complicated.

MaddSauer commented 4 years ago

Same here, same arguments like @nyarly +1

MnrGreg commented 4 years ago

Agreed. A built-in HA kubernetes native option would be better than the external HA PostgreSQL. +1

nyarly commented 4 years ago

@reasonerjt Are you still working on this?

bitsf commented 4 years ago

I think it would be better if someone can contribute PR first and do some basic verification, that would be great help to push the progress

hcsaustrup commented 3 years ago

+1

Jordan886 commented 3 years ago

+1

tribock commented 2 years ago

+1

kajla commented 2 years ago

+1

nyarly commented 2 years ago

I've been working on this; I think the code changes could be minor, but there's coordination happening with MySQL work, and #16420 is a blocker for me.

github-actions[bot] commented 2 years ago

This issue is being marked stale due to a period of inactivity. If this issue is still relevant, please comment or remove the stale label. Otherwise, this issue will close in 30 days.

Jordan886 commented 2 years ago

I've been working on this; I think the code changes could be minor, but there's coordination happening with MySQL work, and #16420 is a blocker for me.

thanks, let's keep this open while #16420 get sorted out

nyarly commented 2 years ago

@Jordan886 #16420 went stale - I'm really not sure how to proceed, since I'm blocked by a code vendoring process

wwentland commented 1 year ago

It would be incredibly helpful to be able to utilise CockroachDB for Harbor as well.

What is the current status or plan? Were you able to unblock yourself, @nyarly in light of @Jordan886's comment?

nyarly commented 1 year ago

I'm utterly at a loss as to how to proceed, @wwentland:

The ORM Harbor uses supports Cockroach natively, but the way they manage dependencies is idiosyncratic: code files appear to be manually added to the vendor/ directory, and in this case Cockroach support wasn't included.

The project contribution guides indicate that all dependency updates have to be made by a core contributor. #16420 was the request to import the whole ORM, which was then labelled "help-wanted" and left to go stale.

I've tried to join the working group related to DB compatibility, but that was frustrating. Their focus was on MySQL, and building import tooling. Compounded with challenges with time zones and language difficulties, I gave up.

My takeaway from the experience, regarding development practices and project governance has been that I can't recommend Harbor in my organization or elsewhere. I wish I could say differently.

wwentland commented 1 year ago

Thank you for the additional information, @nyarly. Much appreciated, even if it isn't the news I had hoped for. I've just raised both issues on Slack and hopefully we can get some traction around this again.

Sounds as if the actual changes required to enable users to utilise CockroachDB are relatively straightforward.

My takeaway from the experience, regarding development practices and project governance has been that I can't recommend Harbor in my organization or elsewhere. I wish I could say differently.

Just out of curiosity: Which alternative did you settle on in lieu of Harbor?

nyarly commented 1 year ago

We've been relying on the image storage from our cloud provider.

filhodanuvem commented 10 months ago

Not sure if it's a matter to document this here, but I've switched the protocol on my connection string from postgresql://user:password@... to cockroachdb://user:password@... and the golang-migrate worked.

phin1x commented 10 months ago

@filhodanuvem which version of harbor are you using and where do you specify the connection string. i didn't find a setting to specify the database dsn.

filhodanuvem commented 10 months ago

Sorry @phin1x, I'm not using harbor, I just added a comment here trying to help since the problem is actually in the golang-migrate tool.

That said, it seems that harbor has a real limitation that doesn't allow you to specify the full database dsn, I suggest you to try one of the following approaches:

phin1x commented 10 months ago

Thank you for the clarification. i looked at the code and wondered where the dsn should take effect.

They removed the vendor folder in one of the latest commits. If we could pass the schema from the config to golang-migrate, we would have full cockroachdb support.

phin1x commented 9 months ago

i have built a custom version of harbor with support for cockroachdb. unfortunately, i realized that there is much more to do than enabling the cockroachdb driver in golang-migrate and changing the schema. harbor uses some postgres features that are currently not implemented by cockroach (e.g. triggers or stored procedures). all sql code needs to be checked for compatibility with cockroachdb and triggers need to be implemented client-side.