-
Notifications
You must be signed in to change notification settings - Fork 57
refactor: replace unstructured informers with typed informers in work… #297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
dc600c6
925c535
121e3fb
34c366d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,6 +24,9 @@ import ( | |||||||||
| "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" | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| var ( | ||||||||||
|
|
@@ -50,18 +53,20 @@ var ( | |||||||||
| ) | ||||||||||
|
|
||||||||||
| type Informers struct { | ||||||||||
| AgentRuntimeInformer cache.SharedIndexInformer | ||||||||||
| CodeInterpreterInformer cache.SharedIndexInformer | ||||||||||
| AgentRuntimeInformer agentruntimev1alpha1.AgentRuntimeInformer | ||||||||||
| CodeInterpreterInformer agentruntimev1alpha1.CodeInterpreterInformer | ||||||||||
| PodInformer cache.SharedIndexInformer | ||||||||||
|
Comment on lines
55
to
58
|
||||||||||
| informerFactory informers.SharedInformerFactory | ||||||||||
| agentcubeInformer agentcubeinformers.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, | ||||||||||
| agentcubeInformer: k8sClient.agentcubeInformer, | ||||||||||
| } | ||||||||||
|
Comment on lines
55
to
70
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is recommended to include the 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,
}
} |
||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -77,18 +82,20 @@ func (ifm *Informers) RunAndWaitForCacheSync(ctx context.Context) error { | |||||||||
|
|
||||||||||
| func (ifm *Informers) run(stopCh <-chan struct{}) { | ||||||||||
| ifm.informerFactory.Start(stopCh) | ||||||||||
|
||||||||||
| ifm.informerFactory.Start(stopCh) | |
| ifm.informerFactory.Start(stopCh) | |
| ifm.AgentRuntimeInformer.Informer() | |
| ifm.CodeInterpreterInformer.Informer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can change these two types to informer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
}| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,9 +29,8 @@ import ( | |
| runtimev1alpha1 "github.com/volcano-sh/agentcube/pkg/apis/runtime/v1alpha1" | ||
| "github.com/volcano-sh/agentcube/pkg/common/types" | ||
| corev1 "k8s.io/api/core/v1" | ||
| 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" | ||
| "k8s.io/client-go/kubernetes" | ||
| "k8s.io/klog/v2" | ||
| "k8s.io/utils/ptr" | ||
|
|
@@ -242,22 +241,13 @@ func buildSandboxClaimObject(params *buildSandboxClaimParams) *extensionsv1alpha | |
| } | ||
|
|
||
| func buildSandboxByAgentRuntime(namespace string, name string, ifm *Informers) (*sandboxv1alpha1.Sandbox, *sandboxEntry, error) { | ||
| agentRuntimeKey := namespace + "/" + name | ||
| // TODO(hzxuzhonghu): make use of typed informer, so we don't need to do type conversion below | ||
| runtimeObj, exists, _ := ifm.AgentRuntimeInformer.GetStore().GetByKey(agentRuntimeKey) | ||
| if !exists { | ||
| return nil, nil, api.ErrAgentRuntimeNotFound | ||
| } | ||
|
|
||
| unstructuredObj, ok := runtimeObj.(*unstructured.Unstructured) | ||
| if !ok { | ||
| klog.Errorf("agent runtime %s type asserting unstructured.Unstructured failed", agentRuntimeKey) | ||
| return nil, nil, fmt.Errorf("agent runtime type asserting failed") | ||
| } | ||
|
|
||
| var agentRuntimeObj runtimev1alpha1.AgentRuntime | ||
| if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredObj.Object, &agentRuntimeObj); err != nil { | ||
| return nil, nil, fmt.Errorf("failed to convert unstructured to AgentRuntime: %w", err) | ||
| // Use typed informer lister to get AgentRuntime | ||
| agentRuntimeObj, err := ifm.AgentRuntimeInformer.Lister().AgentRuntimes(namespace).Get(name) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return nil, nil, api.ErrAgentRuntimeNotFound | ||
| } | ||
| return nil, nil, fmt.Errorf("failed to get agent runtime %s/%s from informer cache: %w", namespace, name, err) | ||
| } | ||
|
|
||
| sessionID := uuid.New().String() | ||
|
|
@@ -316,24 +306,13 @@ func buildCodeInterpreterEnvVars(templateEnv []corev1.EnvVar, authMode runtimev1 | |
| } | ||
|
|
||
| func buildSandboxByCodeInterpreter(namespace string, codeInterpreterName string, informer *Informers) (*sandboxv1alpha1.Sandbox, *extensionsv1alpha1.SandboxClaim, *sandboxEntry, error) { | ||
| codeInterpreterKey := namespace + "/" + codeInterpreterName | ||
| // 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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we set the warm-pool
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed it. You are right the objects retrieved from typed informers often have empty TypeMeta. I have updated the code to explicitly set the |
||
| if err != nil { | ||
| return nil, nil, nil, fmt.Errorf("failed to get code interpreter %s from informer cache: %w", codeInterpreterKey, err) | ||
| } | ||
| if !exists { | ||
| return nil, nil, nil, api.ErrCodeInterpreterNotFound | ||
| } | ||
| unstructuredObj, ok := runtimeObj.(*unstructured.Unstructured) | ||
| if !ok { | ||
| klog.Errorf("code interpreter %s type asserting unstructured.Unstructured failed", codeInterpreterKey) | ||
| return nil, nil, nil, fmt.Errorf("code interpreter type asserting failed") | ||
| } | ||
|
|
||
| var codeInterpreterObj runtimev1alpha1.CodeInterpreter | ||
| if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredObj.Object, &codeInterpreterObj); err != nil { | ||
| return nil, nil, nil, fmt.Errorf("failed to convert unstructured to CodeInterpreter: %w", err) | ||
| if apierrors.IsNotFound(err) { | ||
| return nil, nil, nil, api.ErrCodeInterpreterNotFound | ||
| } | ||
| return nil, nil, nil, fmt.Errorf("failed to get code interpreter %s/%s from informer cache: %w", namespace, codeInterpreterName, err) | ||
| } | ||
|
|
||
| // Check public key available if authMode is picod | ||
|
|
@@ -375,8 +354,8 @@ func buildSandboxByCodeInterpreter(namespace string, codeInterpreterName string, | |
| sessionID: sessionID, | ||
| idleTimeout: idleTimeout, | ||
| ownerReference: &metav1.OwnerReference{ | ||
| APIVersion: codeInterpreterObj.APIVersion, | ||
| Kind: codeInterpreterObj.Kind, | ||
| APIVersion: runtimev1alpha1.SchemeGroupVersion.String(), | ||
| Kind: runtimev1alpha1.CodeInterpreterKind, | ||
| Name: codeInterpreterObj.Name, | ||
| UID: codeInterpreterObj.UID, | ||
| }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To support including the
SharedInformerFactoryfor AgentCube resources in theInformersstruct, add the corresponding package to the imports.