Remove module dependency cycle between Thanos and Cortex

[abursavich]

Following up here from a Slack conversation with @bwplotka (cc: @pracucci)...

Summary of the issue

An old version of Prometheus pulled in k8s.io/client-go@v12.0.0+incompatible.
An old version of Alertmanager pulled in that version of Prometheus.
An old version of Cortex pulled in that version of Alertmanager.
An old version of Thanos pulled in that version of Cortex
A newer version of Cortex pulled in that version of Thanos
A newer version of Thanos pulled in that version of Cortex
...
A newer version of Cortex pulled in that version of Thanos
A newer version of Thanos pulled in that version of Cortex
A recent version of Cortex pulled in that version of Thanos
A recent version of Thanos pulled in that version of Cortex

Newer versions of Prometheus have moved to k8s.io/client-go@v0.20.2.
Newer versions of Alertmanager have removed the dependency on Prometheus.
Newer versions of Cortex have moved to newer versions of Alertmanager.
Newer versions of Thanos have moved to newer versions of Cortex.
Newer versions of Cortex have moved to newer versions of Thanos.

Kubernetes releases predate go modules and historical tags do not conform to go module standards.
Go treats k8s.io/client-go@v12.0.0+incompatible as newer than k8s.io/client-go@v0.20.2.
The cycle between Thanos and Cortex keeps their oldest history together alive in the dependency graph.

If you depend on Thanos or Cortex, then you get stuck with k8s.io/client-go@v12.0.0+incompatible and must use replace directives if you need to use a newer version such as k8s.io/client-go@v0.20.2.

Solution A

Go doesn't allow dependency cycles in packages, so there must not be any. Thanos and Cortex could work together to break the module cycle my extracting shared packages into separate modules.

This is theoretically the better option, but I don't know how much work would be involved and it may add a significant maintenance burden.

Solution B

Thanos and Cortex could work together to reset the cycle in their dependency graph. The cycle would continue to exist, but the historical dependencies would be dropped. This is probably the quicker fix and the other solution may still be undertaken in the future if desired.

Here's how this could work, which I've tested with dummy modules:

Step 1

• Thanos adds commit X that removes Cortex from its go.mod file. At this commit, Thanos won't build properly.
• Cortex adds commit Y that removes Thanos from its go.mod file. At this commit, Cortex won't build properly.

Step 2

• Thanos adds its dependency to Cortex back at github.com/cortexproject/cortex@Y. Thanos will now build properly.
• Cortex adds its dependency to Thanos back at github.com/thanos-io/thanos@X. Cortex will now build properly.

Step 3

• Thanos continues to update it dependency on Cortex as usual but never downgrades it to before commit Y.
• Cortex continues to update it dependency on Thanos as usual but never downgrades it to before commit X.

[roidelapluie]

Another idea is to add to go.mod of cortex and thanos:

exclude (
        // Exclude pre-go-mod kubernetes tags, as they are older
        // than v0.x releases but are picked when we update the dependencies.
        k8s.io/client-go v1.4.0
        k8s.io/client-go v1.4.0+incompatible
        k8s.io/client-go v1.5.0
        k8s.io/client-go v1.5.0+incompatible
        k8s.io/client-go v1.5.1
        k8s.io/client-go v1.5.1+incompatible
        k8s.io/client-go v10.0.0+incompatible
        k8s.io/client-go v11.0.0+incompatible
        k8s.io/client-go v2.0.0+incompatible
        k8s.io/client-go v2.0.0-alpha.1+incompatible
        k8s.io/client-go v3.0.0+incompatible
        k8s.io/client-go v3.0.0-beta.0+incompatible
        k8s.io/client-go v4.0.0+incompatible
        k8s.io/client-go v4.0.0-beta.0+incompatible
        k8s.io/client-go v5.0.0+incompatible
        k8s.io/client-go v5.0.1+incompatible
        k8s.io/client-go v6.0.0+incompatible
        k8s.io/client-go v7.0.0+incompatible
        k8s.io/client-go v8.0.0+incompatible
        k8s.io/client-go v9.0.0+incompatible
        k8s.io/client-go v9.0.0-invalid+incompatible
)

[roidelapluie]

This what we have done in Prometheus. I am wondering if all of this is yet another fallout of Prometheus removing the vendor/ directory.

[abursavich]

An exclude block like that is good for when you want to do go get -u ./... to update your dependencies and stick to k8s.io/client-go@v0.x.

Once you pick up a transitive dependency on k8s.io/client-go@v12.0.0+incompatible, then it's virtually impossible to exclude your way out. If you try to exclude it, the go tool will just pick the next commit after k8s.io/client-go@v12.0.0+incompatible. If you try to exclude that, the go tool will pick the next commit after it... and so on.

[abursavich]

Actually, it looks like there were changes made to the exclude directives in go 1.16: https://groups.google.com/g/golang-nuts/c/FAO6x5AhfPg/m/-Z5baPnLBQAJ

That might be enough for now... I'll do some tests.

[bwplotka]

Discussion during contributor hours:

• Stripping down cyclic modules to separate repo is the goal!

• Thanos imports Query Frontend and e2e part.

  • e2e should be easy. Re: Ported Cortex e2e modules, so we can reuse it smaller projects too. efficientgo/tools#5 - @pracucci mentions it might be ok as long as there is control over it, question is: Where would be the good repo place? Shared API/lib?
  • Experimentation phase - we don't want to slow down.
  • Query frontend we can take a look: We could trim down packages.

• We can do solution B, but we are not sure how that helps with the replace issue, maybe we don't know Go Module internals that much 🤗

[roidelapluie]

who owns github.com/corthanos ? ;)

[bwplotka]

👋🏽 @vatin could you check if the method above helps?

@abursavich are you unblocked?

Would be nice to fix it anyway (: There is a new way btw in Go to retract certain versions: https://golang.org/ref/mod#go-mod-file-retract but I think it does not help much here

I would not try to reset deps before Go1.17. Reasons: In that version all non-go modules Go packages will be blocked, so there will be lot's of fixing anyway. (:

[vatine]

@bwplotka For my specific use-case, I would prefer having "something that works directly from upstream" (this all basically ends up feeding into a Go build farm that extracts statistics from packages, see https://github.com/vatine/gochecker/ for details, and in the reports/ subdir for painfully many details). From my POV, it'd be easier to throw away the stats I have collected so far and simply not feed in Thanos as a "seed package") than trying to retrofit a "rewrite the go.mod for specific module-versions" framework into the existing builder.

[abursavich]

@abursavich are you unblocked?

Yeah, with go 1.16 I can just use excludes.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Closing for now as promised, let us know if you need this to be reopened! 🤗

[bwplotka]

This is about reseting the cycle, but ideally we can track removing cycle at all here 🤗

As discussed here:

#3832 (comment)

[bwplotka]

AC:

• Thanos is no longer dependant on Cortex. 🤔

Motivation:

• Continous pain of upgrading interfaces with breaking changes between project

[bwplotka]

Go 1.17 might help, so worth to check the upgrade process with this Go version

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[matej-g]

I believe this is still unresolved

[kakkoyun]

After recent developments concerning Cortex licensing, I think this has now more importance on the long run.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.