fix: infer correct shard in statefulset setup (#17124, #17016)

[woehrl01]

Fixes #17124
Fixes #17016
Fixes #17183

Following fixes:

1. The env for the shard env does not exist by default. This causes that there will be only shard 0 used. In a multi shard setup all clusters assigned to other shards won't be updated.

   This PR fixes that by correctly inferring the shard in that scenario.

2. The shard number comparison is accidentally done on the pointer and not the value of the shard, this of course results in different values to be compared

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[ishitasequeira]

@woehrl01 Thanks for the PR!!

I remember we have been using defaultShardNumberValue as 0 for application controller deployed as a StatefulSet and -1 as the default value for the application controller deployed as a Deployment. It was intended for the default shard value if not specified via environment variable to always be 0.

I would test out your changes once to see if the change resolves the issue.

[woehrl01]

@ishitasequeira you're welcome.

Yes, that's why I set the value to 0 in case there is only one replica. If I see this right, this is the only case where it makes sense to hardcore it to that value, as any other scenario will make the sharding not work.

[ishitasequeira]

Thanks @woehrl01 , I tested the fix you have and was working on a fix on my end which led to the similar changes.

There are e2e test failures though. Can you take a look at the e2e tests once?

[woehrl01]

@ishitasequeira Thank you, I think those are failing randomly (they have in the past). I just triggered another run.

[leoluz]

Tks a lot for working on this bug @woehrl01!

Once this PR is merged someone should probably work on this: #17183
cc @ishitasequeira

[woehrl01]

@ishitasequeira

I probably need your help here, the e2e now just uncover some strange concurrency/deadlock issues, which I also see in our 2.10. Deployment.

without any change tests are failing differently:

Run 1:

Run 2

[jannfis]

application-controller crashes during tests:

16:47:01                controller | �[mINFO[0000] Updating cluster shards                      
16:47:01                controller | �[mDEBU[0000] Checking if cluster https://kubernetes.default.svc with clusterShard 0 should be processed by shard 0 
16:47:01                controller | �[mINFO[0000] Start watch clusters                         
16:47:01                controller | �[mE0212 16:47:01.545059   15505 runtime.go:79] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
16:47:01                controller | �[mgoroutine 173 [running]:
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/runtime.logPanic({0x365f340?, 0x68c0d60})
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:75 +0x85
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0xc00012f760?})
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:49 +0x6b
16:47:01                controller | �[mpanic({0x365f340?, 0x68c0d60?})
16:47:01                controller | �[m	/opt/hostedtoolcache/go/1.21.6/x64/src/runtime/panic.go:914 +0x21f
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller/sharding.hasShardingUpdates(...)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/sharding/cache.go:152
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller/sharding.(*ClusterSharding).Add(0xc00012f740, 0xc00067ef00)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/sharding/cache.go:82 +0x131
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller/cache.(*liveStateCache).handleAddEvent(0xc000f649a0, 0xc00067ef00)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/cache/cache.go:730 +0x3b
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/util/db.(*db).WatchClusters(0xc0010a3200, {0x47c0670, 0xc000ffff90}, 0xc000b13fd0, 0xc000b13fe0, 0xc000b13ff0)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/util/db/cluster.go:144 +0xc2
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller/cache.(*liveStateCache).Run.func1()
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/cache/cache.go:717 +0x107
16:47:01                controller | �[mgithub.com/argoproj/gitops-engine/pkg/utils/kube.RetryUntilSucceed.func1()
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/github.com/argoproj/gitops-engine/pkg/utils/kube/kube.go:411 +0xfb
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/wait.PollImmediateUntil.ConditionFunc.WithContext.func1({0x4105e5, 0xc000aa9140})
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:222 +0x13
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/wait.runConditionWithCrashProtectionWithContext({0x47c0670?, 0xc0010b12c0?}, 0xc000169550?)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:235 +0x4c
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/wait.poll({0x47c0670, 0xc0010b12c0}, 0x56?, 0x47c0558?, 0x6cc7880?)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:582 +0x32
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/wait.PollImmediateUntilWithContext({0x47c0670?, 0xc0010b12c0?}, 0x3a47d20?, 0xc000231638?)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:547 +0x53
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/wait.PollImmediateUntil(0x3a47d20?, 0xc000169670, 0xc000169660?)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:538 +0x6b
16:47:01                controller | �[mgithub.com/argoproj/gitops-engine/pkg/utils/kube.RetryUntilSucceed({0x47c0670?, 0xc000ffff90?}, 0x2540be400, {0x3d6a5c9, 0xe}, {{0x47ca588?, 0xc000444ac0?}, 0xc0001697b0?}, 0xc000169720)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/github.com/argoproj/gitops-engine/pkg/utils/kube/kube.go:409 +0x116
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller/cache.(*liveStateCache).Run(0xc000f649a0, {0x47c0670?, 0xc000ffff90})
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/cache/cache.go:716 +0x12a
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller.(*ApplicationController).Run.func1()
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/appcontroller.go:809 +0x2e
16:47:01                controller | �[mcreated by github.com/argoproj/argo-cd/v2/controller.(*ApplicationController).Run in goroutine 110
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/appcontroller.go:809 +0x6ae
16:47:01                controller | �[mpanic: runtime error: invalid memory address or nil pointer dereference [recovered]
16:47:01                controller | �[m	panic: runtime error: invalid memory address or nil pointer dereference
16:47:01                controller | �[m[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x2162311]
16:47:01                controller | �[mgoroutine 173 [running]:
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0xc00012f760?})
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:56 +0xcd
16:47:01                controller | �[mpanic({0x365f340?, 0x68c0d60?})
16:47:01                controller | �[m	/opt/hostedtoolcache/go/1.21.6/x64/src/runtime/panic.go:914 +0x21f
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller/sharding.hasShardingUpdates(...)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/sharding/cache.go:152
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller/sharding.(*ClusterSharding).Add(0xc00012f740, 0xc00067ef00)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/sharding/cache.go:82 +0x131
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller/cache.(*liveStateCache).handleAddEvent(0xc000f649a0, 0xc00067ef00)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/cache/cache.go:730 +0x3b
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/util/db.(*db).WatchClusters(0xc0010a3200, {0x47c0670, 0xc000ffff90}, 0xc000b13fd0, 0xc000b13fe0, 0xc000b13ff0)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/util/db/cluster.go:144 +0xc2
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller/cache.(*liveStateCache).Run.func1()
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/cache/cache.go:717 +0x107
16:47:01                controller | �[mgithub.com/argoproj/gitops-engine/pkg/utils/kube.RetryUntilSucceed.func1()
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/github.com/argoproj/gitops-engine/pkg/utils/kube/kube.go:411 +0xfb
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/wait.PollImmediateUntil.ConditionFunc.WithContext.func1({0x4105e5, 0xc000aa9140})
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:222 +0x13
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/wait.runConditionWithCrashProtectionWithContext({0x47c0670?, 0xc0010b12c0?}, 0xc000169550?)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:235 +0x4c
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/wait.poll({0x47c0670, 0xc0010b12c0}, 0x56?, 0x47c0558?, 0x6cc7880?)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:582 +0x32
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/wait.PollImmediateUntilWithContext({0x47c0670?, 0xc0010b12c0?}, 0x3a47d20?, 0xc000231638?)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:547 +0x53
16:47:01                controller | �[mk8s.io/apimachinery/pkg/util/wait.PollImmediateUntil(0x3a47d20?, 0xc000169670, 0xc000169660?)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:538 +0x6b
16:47:01                controller | �[mgithub.com/argoproj/gitops-engine/pkg/utils/kube.RetryUntilSucceed({0x47c0670?, 0xc000ffff90?}, 0x2540be400, {0x3d6a5c9, 0xe}, {{0x47ca588?, 0xc000444ac0?}, 0xc0001697b0?}, 0xc000169720)
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/vendor/github.com/argoproj/gitops-engine/pkg/utils/kube/kube.go:409 +0x116
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller/cache.(*liveStateCache).Run(0xc000f649a0, {0x47c0670?, 0xc000ffff90})
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/cache/cache.go:716 +0x12a
16:47:01                controller | �[mgithub.com/argoproj/argo-cd/v2/controller.(*ApplicationController).Run.func1()
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/appcontroller.go:809 +0x2e
16:47:01                controller | �[mcreated by github.com/argoproj/argo-cd/v2/controller.(*ApplicationController).Run in goroutine 110
16:47:01                controller | �[m	/home/runner/work/argo-cd/argo-cd/controller/appcontroller.go:809 +0x6ae
16:47:01                controller | �[mTerminating controller```

[ishitasequeira]

Thanks @jannfis for sharing the logs. The crash seems to be because of the change return int64(*old.Shard) != int64(*new.Shard) part of this PR.

[woehrl01]

@ishitasequeira oh that's awkward 😅🙈 thank you!

Should be nil == nil handled as the same? I'm not sure in which case this makes sense.

[leoluz]

Please check my comment

[leoluz]

Should be nil == nil handled as the same? I'm not sure in which case this makes sense.

@woehrl01 This should return false as per the code implemented at line 149

[woehrl01]

@leoluz if that's the case then I would say that my check is correct.

If both are nil, it returns false. If one of them is nil, the other can't, because of the previous check. so they are different and this should return true.

If your check should be used, I would modify the previous one to replace the && with || . Right?

[woehrl01]

@ishitasequeira @leoluz

Thank you for your feedback, I just added some tests. During writing of the tests I discovered some other bugs, and I changed the logic of hasShardingUpdates because I now see why it has been confusing before.

The overall problem addressed are:

• The Shard of the "Cluster"- resource should never be modified by the algorithm, because it will not represent the actual configuration, it should only be stored in the state of the cache. This fixes a bug where round-robin was not deterministic at all, if clusters are added or removed.
• The pointer of the cluster was accidentally the pointer of the loop variable, which in Golang has some strange side effects. I changed that, now the shards are mapped to the correct cluster value. In combination with the previous error this caused a really strange behaviour in the Init function.

I also added some more useful log outputs to see if clusters have changed their shard based on an update/delete event.

Please let me know what you think!

[woehrl01]

@leoluz While I was at it, I directly integrated #17183. This makes sense in my opinion because this part was one of the actual needed fixes.

[agaudreault]

LGTM

[agaudreault]

The previous PR seemed to update the cluster shard value in the cache and not only the shard number in the Shards object.

I think the goal was to persist the computed shard in the cache so we do not need to call sharding.getClusterShard(c) and recompute the shard number every time updateDistribution() is called. Basically, once the controller starts managing a cluster, it will manage it forever.

Now with the dynamic cluster sharding, I think you are right. We do have to recompute all the cluster shards whenever a cluster is added/modified/deleted.

[woehrl01]

@agaudreault Thanks for confirming, the problem with "controller starts managing a cluster, it will manage it forever." is that this does not work properly as every shard could have a different view on the cluster. If clusters are added/removed and stateful set controllers are restarting at different time intervals (e.g. spot interruptions)

[leoluz]

LGTM

[ishitasequeira]

LGTM!! Thanks @woehrl01 for working on the fix.

[leoluz]

/cherry-pick release-2.10

[gcp-cherry-pick-bot]

Cherry-pick failed with Merge error 5d6111b7459af8b1cf1c49559f1ec001410a6e9f into temp-cherry-pick-9bf215-release-2.10