refactor: replace unstructured informers with typed informers in work…#297
refactor: replace unstructured informers with typed informers in work…#297klprakhar wants to merge 4 commits intovolcano-sh:mainfrom
Conversation
|
Welcome @klprakhar! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
…load manager Signed-off-by: klprakhar <prakharkulshrestha939@gmail.com>
klprakhar
force-pushed
the
refactor/typed-informers-workloadmanager
branch
from
3908c0f to
dc600c6
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Workload Manager’s handling of AgentRuntime and CodeInterpreter resources to use typed client-go informers/listers instead of dynamic/unstructured informers, improving type safety and reducing manual conversions.
Changes:
- Switch
AgentRuntime/CodeInterpreterlookups inworkload_builder.goto use typed informer listers withapierrors.IsNotFoundhandling. - Add an AgentCube typed clientset + shared informer factory initialization in
k8s_client.go. - Update the
Informerswiring ininformers.goto use typed informer interfaces and updated cache sync/run logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/workloadmanager/workload_builder.go | Replaces unstructured cache access + conversions with typed lister Get() calls and NotFound mapping. |
| pkg/workloadmanager/k8s_client.go | Initializes the AgentCube typed clientset and informer factory for runtime CRDs. |
| pkg/workloadmanager/informers.go | Switches Informers fields to typed informers and updates run/sync to use .Informer() methods. |
| type Informers struct { | ||
| AgentRuntimeInformer cache.SharedIndexInformer | ||
| CodeInterpreterInformer cache.SharedIndexInformer | ||
| AgentRuntimeInformer agentruntimev1alpha1.AgentRuntimeInformer | ||
| CodeInterpreterInformer agentruntimev1alpha1.CodeInterpreterInformer | ||
| PodInformer cache.SharedIndexInformer |
There was a problem hiding this comment.
Changing Informers.AgentRuntimeInformer / CodeInterpreterInformer from cache.SharedIndexInformer to the typed informer interfaces breaks tests and any code that constructs Informers with fake SharedIndexInformers (e.g. pkg/workloadmanager/informers_test.go currently assigns cache.SharedIndexInformer and will no longer compile). Update tests to use a generated fake AgentCube clientset+SharedInformerFactory or provide small test fakes that implement the typed informer interfaces (exposing Informer() + Lister()).
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| "k8s.io/apimachinery/pkg/runtime" |
There was a problem hiding this comment.
k8s.io/apimachinery/pkg/runtime is no longer referenced in this file after switching to typed listers. This will cause an unused import compile error; please remove the import (or reintroduce a usage if still needed).
| "k8s.io/apimachinery/pkg/runtime" |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the workload manager to use typed informers and listers for AgentCube resources, eliminating manual type conversions from unstructured objects. The review feedback suggests improving lifecycle management by incorporating the AgentCube informer factory into the Informers struct and using its Start method to manage informers consistently.
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
| "k8s.io/client-go/informers" | ||
| "k8s.io/client-go/tools/cache" | ||
|
|
||
| agentruntimev1alpha1 "github.com/volcano-sh/agentcube/client-go/informers/externalversions/runtime/v1alpha1" | ||
| ) |
There was a problem hiding this comment.
To support including the SharedInformerFactory for AgentCube resources in the Informers struct, add the corresponding package to the imports.
| "k8s.io/apimachinery/pkg/runtime/schema" | |
| "k8s.io/client-go/informers" | |
| "k8s.io/client-go/tools/cache" | |
| agentruntimev1alpha1 "github.com/volcano-sh/agentcube/client-go/informers/externalversions/runtime/v1alpha1" | |
| ) | |
| "k8s.io/apimachinery/pkg/runtime/schema" | |
| "k8s.io/client-go/informers" | |
| "k8s.io/client-go/tools/cache" | |
| agentcubeinformers "github.com/volcano-sh/agentcube/client-go/informers/externalversions" | |
| agentruntimev1alpha1 "github.com/volcano-sh/agentcube/client-go/informers/externalversions/runtime/v1alpha1" | |
| ) |
| type Informers struct { | ||
| AgentRuntimeInformer cache.SharedIndexInformer | ||
| CodeInterpreterInformer cache.SharedIndexInformer | ||
| AgentRuntimeInformer agentruntimev1alpha1.AgentRuntimeInformer | ||
| CodeInterpreterInformer agentruntimev1alpha1.CodeInterpreterInformer | ||
| PodInformer cache.SharedIndexInformer | ||
| informerFactory informers.SharedInformerFactory | ||
| } | ||
|
|
||
| func NewInformers(k8sClient *K8sClient) *Informers { | ||
| return &Informers{ | ||
| AgentRuntimeInformer: k8sClient.dynamicInformer.ForResource(AgentRuntimeGVR).Informer(), | ||
| CodeInterpreterInformer: k8sClient.dynamicInformer.ForResource(CodeInterpreterGVR).Informer(), | ||
| AgentRuntimeInformer: k8sClient.agentcubeInformer.Runtime().V1alpha1().AgentRuntimes(), | ||
| CodeInterpreterInformer: k8sClient.agentcubeInformer.Runtime().V1alpha1().CodeInterpreters(), | ||
| PodInformer: k8sClient.podInformer, | ||
| informerFactory: k8sClient.informerFactory, | ||
| } |
There was a problem hiding this comment.
It is recommended to include the agentcubeInformer factory in the Informers struct. This allows for more consistent and robust lifecycle management of the informers (e.g., using the factory's Start method) rather than manually running individual informers in goroutines.
type Informers struct {
AgentRuntimeInformer agentruntimev1alpha1.AgentRuntimeInformer
CodeInterpreterInformer agentruntimev1alpha1.CodeInterpreterInformer
PodInformer cache.SharedIndexInformer
informerFactory informers.SharedInformerFactory
agentcubeInformer agentcubeinformers.SharedInformerFactory
}
func NewInformers(k8sClient *K8sClient) *Informers {
return &Informers{
AgentRuntimeInformer: k8sClient.agentcubeInformer.Runtime().V1alpha1().AgentRuntimes(),
CodeInterpreterInformer: k8sClient.agentcubeInformer.Runtime().V1alpha1().CodeInterpreters(),
PodInformer: k8sClient.podInformer,
informerFactory: k8sClient.informerFactory,
agentcubeInformer: k8sClient.agentcubeInformer,
}
}| func (ifm *Informers) run(stopCh <-chan struct{}) { | ||
| ifm.informerFactory.Start(stopCh) | ||
| go ifm.AgentRuntimeInformer.Run(stopCh) | ||
| go ifm.CodeInterpreterInformer.Run(stopCh) | ||
| go ifm.AgentRuntimeInformer.Informer().Run(stopCh) | ||
| go ifm.CodeInterpreterInformer.Informer().Run(stopCh) | ||
| } |
There was a problem hiding this comment.
Use the agentcubeInformer.Start method to start the informers managed by the AgentCube factory. This is more consistent with how the core informerFactory is handled and avoids manual goroutine management for individual informers.
func (ifm *Informers) run(stopCh <-chan struct{}) {
ifm.informerFactory.Start(stopCh)
ifm.agentcubeInformer.Start(stopCh)
}Signed-off-by: klprakhar <prakharkulshrestha939@gmail.com>
Signed-off-by: klprakhar <prakharkulshrestha939@gmail.com>
| @@ -77,18 +82,17 @@ func (ifm *Informers) RunAndWaitForCacheSync(ctx context.Context) error { | |||
|
|
|||
| func (ifm *Informers) run(stopCh <-chan struct{}) { | |||
| ifm.informerFactory.Start(stopCh) | |||
There was a problem hiding this comment.
agentcubeInformer.Start(stopCh) is called before any AgentCube typed informers are instantiated via .Informer(). In the generated informer code, AgentRuntimes()/CodeInterpreters() are lazy and only register the underlying SharedIndexInformer with the factory when .Informer() (or .Lister()) is invoked; Start() only starts informers already present in the factory map. As a result, the AgentRuntime/CodeInterpreter informers may never be started and waitForCacheSync can time out in production. Instantiate the informers (e.g., call ifm.AgentRuntimeInformer.Informer() and ifm.CodeInterpreterInformer.Informer()) before calling ifm.agentcubeInformer.Start(stopCh) (either in NewInformers or at the top of run).
| ifm.informerFactory.Start(stopCh) | |
| ifm.informerFactory.Start(stopCh) | |
| ifm.AgentRuntimeInformer.Informer() | |
| ifm.CodeInterpreterInformer.Informer() |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
+ Coverage 43.37% 47.72% +4.35%
==========================================
Files 30 30
Lines 2610 2812 +202
==========================================
+ Hits 1132 1342 +210
+ Misses 1355 1330 -25
- Partials 123 140 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // TODO(hzxuzhonghu): make use of typed informer, so we don't need to do type conversion below | ||
| runtimeObj, exists, err := informer.CodeInterpreterInformer.GetStore().GetByKey(codeInterpreterKey) | ||
| // Use typed informer lister to get CodeInterpreter | ||
| codeInterpreterObj, err := informer.CodeInterpreterInformer.Lister().CodeInterpreters(namespace).Get(codeInterpreterName) |
There was a problem hiding this comment.
Can we set the warm-pool OwnerReference APIVersion/Kind explicitly instead of reading them from codeInterpreterObj.TypeMeta? Typed informer objects can have empty TypeMeta, and the current E2E run rejects every warm-pool SandboxClaim with empty ownerReferences.
There was a problem hiding this comment.
Fixed it. You are right the objects retrieved from typed informers often have empty TypeMeta. I have updated the code to explicitly set the APIVersion and Kind in the OwnerReference using runtimev1alpha1.SchemeGroupVersion and runtimev1alpha1.CodeInterpreterKind constants, ensuring that SandboxClaims are correctly populated and accepted by the API.
Signed-off-by: klprakhar <prakharkulshrestha939@gmail.com>
klprakhar
force-pushed
the
refactor/typed-informers-workloadmanager
branch
from
e197c46 to
34c366d
Compare
|
Hi @acsoto, I have fixed the change you mentioned, please let me know if any more tweaks are required happy to resolve them! The CI is also green and the PR overview by copilot is also good. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: acsoto The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| ifm.AgentRuntimeInformer.Informer() | ||
| ifm.CodeInterpreterInformer.Informer() |
There was a problem hiding this comment.
You can change these two types to informer
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR refactors the
workloadmanagerto use typed informers and listers instead of generic unstructured dynamic informers forAgentRuntimeandCodeInterpreterresources.Key improvements:
TODO(hzxuzhonghu)in the codebase.unstructuredto typed object conversion.apierrors.IsNotFoundchecks for resource retrieval.Special notes for your reviewer:
The refactor covers the initialization of the AgentCube clientset in
k8s_client.go, the informer setup ininformers.go, and the logic updates inworkload_builder.go.