Fix the list calls to use informer #358
Conversation
kkmsft
changed the title
There was a problem hiding this comment.
Need to add watch verb in clusterrole in here (https://github.com/Azure/aad-pod-identity/tree/master/deploy) as well
pkg/crd/crd.go
Outdated
| @@ -23,13 +23,11 @@ import ( | |||
|
|
|||
| // Client represents all the watchers and informers | |||
There was a problem hiding this comment.
Can we change this comment also to say Client represents all informers
pkg/crd/crd.go
Outdated
| func newAssignedIDInformer(lw *cache.ListWatch) (cache.SharedInformer, error) { | ||
| azAssignedIDInformer := cache.NewSharedInformer(lw, &aadpodid.AzureAssignedIdentity{}, time.Minute*10) | ||
| if azAssignedIDInformer == nil { | ||
| return nil, fmt.Errorf("could not create %s nformer", aadpodid.AzureAssignedIDResource) |
pkg/crd/crd.go
Outdated
| func newPodIdentityExceptionInformer(lw *cache.ListWatch) (cache.SharedInformer, error) { | ||
| azPodIDExceptionInformer := cache.NewSharedInformer(lw, &aadpodid.AzurePodIdentityException{}, time.Minute*10) | ||
| if azPodIDExceptionInformer == nil { | ||
| return nil, fmt.Errorf("could not create %s nformer", aadpodid.AzureIdentityExceptionResource) |
pkg/crd/crd.go
Outdated
| // Note: List items returned from cache have empty Kind and API version.. | ||
| // Work around this issue since we need that for event recording to work. | ||
| if o.Kind == "" { | ||
| o.Kind = reflect.TypeOf(*o).String() |
There was a problem hiding this comment.
Since we are setting the kind based on object type we can generalize this into a function - pass the obj, crd group and crdversion so we can use it later if we have another version too.
pkg/crd/crd.go
Outdated
| } | ||
|
|
||
| resList = append(resList, *o) | ||
| glog.V(6).Infof("Appending binding: %s/%s to list.", o.Name, o.Namespace) |
There was a problem hiding this comment.
should be namespace/name
| glog.V(6).Infof("Appending binding: %s/%s to list.", o.Name, o.Namespace) | |
| glog.V(6).Infof("Appending binding: %s/%s to list.", o.Namespace, o.Name) |
pkg/crd/crd.go
Outdated
| o.APIVersion = aadpodid.CRDGroup + "/" + aadpodid.CRDVersion | ||
| } | ||
| resList = append(resList, *o) | ||
| glog.V(6).Infof("Appending Assigned ID: %s/%s to list.", o.Name, o.Namespace) |
There was a problem hiding this comment.
should be namespace/name
| glog.V(6).Infof("Appending Assigned ID: %s/%s to list.", o.Name, o.Namespace) | |
| glog.V(6).Infof("Appending Assigned ID: %s/%s to list.", o.Namespace, o.Name) |
pkg/crd/crd.go
Outdated
| o.APIVersion = aadpodid.CRDGroup + "/" + aadpodid.CRDVersion | ||
| } | ||
| resList = append(resList, *o) | ||
| glog.V(6).Infof("Appending Identity: %s/%s to list.", o.Name, o.Namespace) |
There was a problem hiding this comment.
should be namespace/name
| glog.V(6).Infof("Appending Identity: %s/%s to list.", o.Name, o.Namespace) | |
| glog.V(6).Infof("Appending Identity: %s/%s to list.", o.Namespace, o.Name) |
pkg/crd/crd.go
Outdated
| o.APIVersion = aadpodid.CRDGroup + "/" + aadpodid.CRDVersion | ||
| } | ||
| resList = append(resList, *o) | ||
| glog.V(6).Infof("Appending exception: %s/%s to list.", o.Name, o.Namespace) |
There was a problem hiding this comment.
should be namespace/name
| glog.V(6).Infof("Appending exception: %s/%s to list.", o.Name, o.Namespace) | |
| glog.V(6).Infof("Appending exception: %s/%s to list.", o.Namespace, o.Name) |
kkmsft
changed the title
| listObject, err := c.PodListWatch.List(metav1.ListOptions{ | ||
| FieldSelector: "status.podIP==" + podip + phaseStatusFilter, | ||
| }) | ||
| func isPhaseValid(p v1.PodPhase) bool { |
There was a problem hiding this comment.
We should add a comment here saying we also allowing Pending state to account for init containers. We can do it in a separate PR.
There was a problem hiding this comment.
Sure, will add in future PR.
|
|
||
| func (c *KubeClient) getPodList(podip string) ([]*v1.Pod, error) { | ||
| list, err := c.PodInformer.Lister().List(labels.Everything()) |
There was a problem hiding this comment.
Why do we need to explicitly pass labels.Everything() only for this informer list call?
There was a problem hiding this comment.
podInformer is from SharedInformerFactory. This has specific listers instead of generic listers like the SharedInformers (which is used for custom types like CRDs). Hence the listing uses labels which is specific to the pod lister.
… review comments)
Reason for Change:
Fix the list calls to use informer. It was found during scale tests that API Server was getting a large number of LIST calls for various CRDS. Investigations revealed a bug - currently we use list watch directly, instead of using informer for LIST calls for CRDs and pods(in some code paths).
Issue Fixed:
Notes for Reviewers: