Address Excessive API Server calls from CNI Pods #1419
Conversation
cmd/aws-k8s-agent/main.go
Outdated
| } | ||
|
|
||
| ipamContext, err := ipamd.New(kubeClient, eniConfigController) | ||
| ipamContext, err := ipamd.New(standaloneK8SClient, k8sClient) |
There was a problem hiding this comment.
Why does ipamd.New need both cached and uncached client? That seems weird, or a sign that we need cache invalidation somewhere.
cmd/aws-k8s-agent/main.go
Outdated
| if err != nil { | ||
| log.Errorf("Failed to create client: %v", err) | ||
| //Check API Server Connectivity | ||
| if k8sapi.CheckAPIServerConnectivity() != nil{ |
There was a problem hiding this comment.
... why do this, rather than just wait until the first actual apiserver call to effectively do it for us?
(In particular, just because the apiserver was available here doesn't mean it's always going to be available during every other part of execution - so we still have to handle intermittent connectivity issues elsewhere.)
There was a problem hiding this comment.
Sure, a success here doesn't guarantee the call will go through a while later. This was mainly done to be in parity with the current CNI codeflow, where we check API server connectivity and essentially crash if the initial API Server connectivity check fails. Sort of helps to catch or highlight basic connectivity issues to API server as part of CNI bootstrap..
https://github.com/aws/amazon-vpc-cni-k8s/blob/master/pkg/k8sapi/discovery.go#L87
|
|
||
| // ENIConfigStatus defines the observed state of ENIConfig | ||
| type ENIConfigStatus struct { | ||
| // Fill me |
There was a problem hiding this comment.
Nit: If ENIConfigStatus is not used, can we remove it?
There was a problem hiding this comment.
Yeah, just the standard Spec and Status objects for CRDs/K8S resources. Although, we're not using it right now for ENIConfig CRD, it is good to have them in place.
| Namespace: metav1.NamespaceSystem, | ||
| } | ||
|
|
||
| err := d.k8sClient.List(ctx, &podList, &listOptions) |
There was a problem hiding this comment.
Big 👍 to the new label selector approach. The apiserver thanks you.
Done. |
What type of PR is this?
Enhancement
Which issue does this PR fix:
CNI daemonset Pods currently run a controller to handle ENIConfig CRDs along with Node and Pod objects. CNI code base depends on a rather old version of operator-sdk (v0.0.7) and this combined with dynamic api group discovery is contributing to a rather high rate of API Server calls from each CNI pod (approx 8500 calls per hr from each node). As we scale the cluster, this becomes a clear problem and needs to be dealt with.
What does this PR do / Why do we need it:
PR attempts to address the above issue via a 2-way approach depending on the resource it is dealing with.
For ENIConfig and Node resources, we will now use a K8S client with a cache tied to it - following the list+watch approach for both these resources. Both ENIConfig and Node resources are accessed during the ENI creation flow when custom networking is enabled and we don't want to introduce an API server call during this workflow.
For Pods, we will make an API server call on need basis.
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
With the change, API Server call count reduced to about 30-35 per hr from each node from the current 8500 (approx) per hr from each node.
Testing done on this change:
Verified both regular and custom networking mode.
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No. Yes, upgrade scenario has been tested.
Does this change require updates to the CNI daemonset config files to work?:
No
Does this PR introduce any user-facing change?:
No
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.