daemon: move circular initialization of policy.Repository to hive #24073
Conversation
lmb
added
release-note/misc
lmb
force-pushed
the
egw-mod-dependencies
branch
from
7bc0634
to
f5bba96
Compare
lmb
force-pushed
the
egw-mod-dependencies
branch
5 times, most recently
from
902e3e9
to
021babd
Compare
|
/test |
lmb
force-pushed
the
egw-mod-dependencies
branch
2 times, most recently
from
cd160bb
to
f8f1b83
Compare
|
@jibi given egress GW would be great if you could take a look |
|
/test Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed: Click to show.Test NameFailure OutputIf it is a flake and a GitHub issue doesn't already exist to track it, comment Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed: Click to show.Test NameFailure OutputIf it is a flake and a GitHub issue doesn't already exist to track it, comment |
| // | ||
| // Qeues must be allocated via [Repository.Start]. The function serves to | ||
| // satisfy hive invariants. | ||
| func NewStoppedPolicyRepository( |
There was a problem hiding this comment.
What does "Stopped" mean? I found out what it means by reading further along in the code, but it would be good to document it in this function's godoc.
There was a problem hiding this comment.
$ git grep NewPolicyReposit | grep -v test.go
daemon/cmd/daemon.go: // Must be done before calling policy.NewPolicyRepository() below.
pkg/policy/repository.go:// NewPolicyRepository creates a new policy repository.
pkg/policy/repository.go:func NewPolicyRepository(
$ git grep NewPolicyReposit | grep test.go | wc -l
68
It looks like the main NewPolicyRepository() is now just a helper for test code. It's a bit of a code smell to me to call this NewStoppedPolicyRepository() rather than just having a NewPolicyRepository() + Start(), but I appreciate that all those other test code references are a bit of a pain to fix up if they're all relying on this code as-is to initialize+start the policy repo. I'm OK to defer this for now.
There was a problem hiding this comment.
I've updated the function doc. I think Jussi's preference was to pass a hive.Lifecycle into the constructor, but that causes a big ripple effect, NewPolicyRepository is used a lot. So I did this botch to keep the changes small(er).
christarazi
added
area/daemon
Introduce a small wrapper around chan struct{} to be able to
provide it via hive to egressgateway and ipcache.
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
Add a constructor for Repository that doesn't allocate any queues. This is necessary to satisfy hive invariants. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
lmb
force-pushed
the
egw-mod-dependencies
branch
from
f8f1b83
to
5f216ff
Compare
IPCache, CachingIdentityAllocator and policy.Repository (really SelectorCache) have a gnarly circular dependency during initialization. Do the simplest possible thing and lift them into hive by providing a constructor that encapsulates the necessary logic. This way we can add dependencies on these without having to modularize them first. During shutdown of the IPCache we first cancel the context and then call IPCache.Shutdown, see cilium#21676. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
lmb
force-pushed
the
egw-mod-dependencies
branch
from
5f216ff
to
1ef13b4
Compare
|
/test |
lmb
added
the
ready-to-merge
|
Required reviews are in, CI is green, merging |
IPCache, CachingIdentityAllocator and policy.Repository (really SelectorCache) have a gnarly circular dependency during intialization. Do the simplest possible thing and lift them into hive by providing a constructor that encapsulates the neccessary logic.
This way we can add dependencies on these without having to modularize them first.
Pre-requisite for #23046.