Skip to content
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

Is there any plan to support sidecarContainers #5745

Open
Monokaix opened this issue Oct 26, 2024 · 8 comments
Open

Is there any plan to support sidecarContainers #5745

Monokaix opened this issue Oct 26, 2024 · 8 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@Monokaix
Copy link

What would you like to be added:
Add sidecarContainers support when compute resource requirements in resourceInterpreter.
Why is this needed:
sidecarContainer has been a beta feature and enabled by default in k8s v1.29, we should also support it to get a more accurate resources requirement. xrefs: https://kubernetes.io/docs/concepts/workloads/pods/sidecar-containers/

@Monokaix Monokaix added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 26, 2024
@Monokaix
Copy link
Author

Glad to contribute if it's acceptable.

@Monokaix
Copy link
Author

/assign

@RainbowMango
Copy link
Member

sidecarContainer has been a beta feature and enabled by default in k8s v1.29, we should also support it to get a more accurate resources requirement.

Can you give an example to explain the gaps we currently have? As far as I know, resource interpreters already consider sidecar containers when determining resource requirements.

// GenerateReplicaRequirements generates replica requirements for node and resources.
func GenerateReplicaRequirements(podTemplate *corev1.PodTemplateSpec) *workv1alpha2.ReplicaRequirements {
nodeClaim := GenerateNodeClaimByPodSpec(&podTemplate.Spec)
resourceRequest := util.EmptyResource().AddPodTemplateRequest(&podTemplate.Spec).ResourceList()
if nodeClaim != nil || resourceRequest != nil {
replicaRequirements := &workv1alpha2.ReplicaRequirements{
NodeClaim: nodeClaim,
ResourceRequest: resourceRequest,
}
if features.FeatureGate.Enabled(features.ResourceQuotaEstimate) {
replicaRequirements.Namespace = podTemplate.Namespace
// PriorityClassName is set from podTemplate
// If it is not set from podTemplate, it is default to an empty string
replicaRequirements.PriorityClassName = podTemplate.Spec.PriorityClassName
}
return replicaRequirements
}
return nil
}

@Monokaix
Copy link
Author

sidecarContainer has been a beta feature and enabled by default in k8s v1.29, we should also support it to get a more accurate resources requirement.

Can you give an example to explain the gaps we currently have? As far as I know, resource interpreters already consider sidecar containers when determining resource requirements.

// GenerateReplicaRequirements generates replica requirements for node and resources.
func GenerateReplicaRequirements(podTemplate *corev1.PodTemplateSpec) *workv1alpha2.ReplicaRequirements {
nodeClaim := GenerateNodeClaimByPodSpec(&podTemplate.Spec)
resourceRequest := util.EmptyResource().AddPodTemplateRequest(&podTemplate.Spec).ResourceList()
if nodeClaim != nil || resourceRequest != nil {
replicaRequirements := &workv1alpha2.ReplicaRequirements{
NodeClaim: nodeClaim,
ResourceRequest: resourceRequest,
}
if features.FeatureGate.Enabled(features.ResourceQuotaEstimate) {
replicaRequirements.Namespace = podTemplate.Namespace
// PriorityClassName is set from podTemplate
// If it is not set from podTemplate, it is default to an empty string
replicaRequirements.PriorityClassName = podTemplate.Spec.PriorityClassName
}
return replicaRequirements
}
return nil
}

Seems the func AddPodTemplateRequest is just copyed from old k8s version, and didn't consider the sidecarcontainers resources, sidecar container is a special init container with restartPolicy = Always, here is the logic of k8s when compute pod resource requests https://github.com/kubernetes/kubernetes/blob/b3cf9c6e5c7f7851529e17a4ebf51c2e98392cb4/staging/src/k8s.io/component-helpers/resource/helpers.go#L104

@RainbowMango
Copy link
Member

Oh, I see. Thanks for the reminder, that makes sense to me.

PS:
Link some materials about the SideCarContainer feature:

@Monokaix
Copy link
Author

A little question need to be cleared, what if the featuregate of sidecareContaiener is enabled in karmada control plan but is disabled in member cluster or vice versa, to put it simply, the replicas calculation is not consistent between control plane and members. How does karmdada solve the problem of inconsistent featuregate switches?

@RainbowMango
Copy link
Member

What would happen if we deploy a Deployment with sidecar container to a cluster where the SidecarContainers feature is disabled?

@Monokaix
Copy link
Author

Sidecarcontainer is a special init container but will run last forever with main container, the pod request computation result is different with Sidecarcontainer disabled and enabled, it will affect scheduling result, if the SidecarContainers is enaled in control plane, the pod request resources computation result in control plane will be larger than in member clusters with SidecarContainers disabled, and the first level of scheduling may think the pod is not schedulable but actually it can be scheduled in member cluster. And if Sidecarcontainer is disabled in control plane while enabled in member cluster, the pod maybe schedulable but actually it cann't be schedulable in member cluster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
Status: No status
Development

No branches or pull requests

2 participants