WIP: migrate estimator protobuf to use standard protoc-gen-go#7298
WIP: migrate estimator protobuf to use standard protoc-gen-go#7298zhzhuang-zju wants to merge 1 commit intokarmada-io:masterfrom
Conversation
karmada-bot
added
kind/api-change
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
karmada-bot
added
the
size/XXL
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the scheduler-estimator's protobuf implementation to ensure its long-term compatibility with evolving Kubernetes APIs, specifically addressing changes in Kubernetes 1.35 and beyond. The core change involves transitioning to standard protobuf generation tools and introducing a dual-field strategy for Kubernetes object serialization. This approach allows for direct binary handling of Kubernetes types while preserving backward compatibility for existing components, facilitating a smooth upgrade path. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 successfully migrates the scheduler-estimator protobuf implementation from the legacy go-to-protobuf generator to the standard protoc-gen-go toolchain, addressing compatibility concerns with Kubernetes 1.35+. The adoption of a "Peer Fields" strategy, introducing new bytes fields alongside deprecated strongly-typed fields, is a robust approach to ensure graceful migration and wire compatibility. The addition of helper methods (Set..., Unmarshal..., MustSet...) simplifies the conversion between Kubernetes Go types and the new protobuf structure. The changes are comprehensive, affecting protobuf definitions, generation scripts, and Go code across various components to align with the new protobuf standard. However, several instances of ignored errors from Set and Unmarshal helper methods have been identified, which could lead to silent failures or incorrect data processing. These should be addressed to ensure the reliability of the new protobuf serialization/deserialization logic.
| Tolerations: replicaRequirements.NodeClaim.Tolerations, | ||
| } | ||
| _ = req.ReplicaRequirements.NodeClaim.SetNodeAffinity(replicaRequirements.NodeClaim.HardNodeAffinity) | ||
| _ = req.ReplicaRequirements.NodeClaim.SetTolerations(replicaRequirements.NodeClaim.Tolerations) |
There was a problem hiding this comment.
The SetTolerations method returns an error, but it is currently ignored. If marshaling fails, the TolerationsBytes field will not be populated, potentially leading to incorrect toleration matching or reliance on deprecated fields. It's important to handle this error to ensure the new 'Peer Fields' strategy works as intended. Consider propagating the error or logging it.
|
|
||
| for _, component := range components { | ||
| if component.ReplicaRequirements.ResourceRequest == nil { | ||
| requirements, _ := component.ReplicaRequirements.UnmarshalResourceRequest() |
There was a problem hiding this comment.
The UnmarshalResourceRequest method returns an error, but it is currently ignored. If unmarshaling fails, requirements might be an empty ResourceList, leading to incorrect resource aggregation. It's important to handle this error to ensure the new 'Peer Fields' strategy works as intended. Consider propagating the error or logging it.
| NodeSelector: cr.NodeClaim.NodeSelector, | ||
| Tolerations: cr.NodeClaim.Tolerations, | ||
| } | ||
| _ = out.NodeClaim.SetNodeAffinity(cr.NodeClaim.HardNodeAffinity) |
There was a problem hiding this comment.
The SetNodeAffinity method returns an error, but it is currently ignored. If marshaling fails, the NodeAffinityBytes field will not be populated, potentially leading to incorrect node affinity matching or reliance on deprecated fields. It's important to handle this error to ensure the new 'Peer Fields' strategy works as intended. Consider propagating the error or logging it.
| Tolerations: cr.NodeClaim.Tolerations, | ||
| } | ||
| _ = out.NodeClaim.SetNodeAffinity(cr.NodeClaim.HardNodeAffinity) | ||
| _ = out.NodeClaim.SetTolerations(cr.NodeClaim.Tolerations) |
There was a problem hiding this comment.
The SetTolerations method returns an error, but it is currently ignored. If marshaling fails, the TolerationsBytes field will not be populated, potentially leading to incorrect toleration matching or reliance on deprecated fields. It's important to handle this error to ensure the new 'Peer Fields' strategy works as intended. Consider propagating the error or logging it.
| Namespace: replicaRequirements.Namespace, | ||
| PriorityClassName: replicaRequirements.PriorityClassName, | ||
| } | ||
| _ = req.ReplicaRequirements.SetResourceRequest(replicaRequirements.ResourceRequest) |
There was a problem hiding this comment.
The SetResourceRequest method returns an error, but it is currently ignored. If marshaling fails, the ResourceRequestBytes field will not be populated, which could lead to incorrect behavior or reliance on deprecated fields. It's important to handle this error to ensure the new 'Peer Fields' strategy works as intended. Consider propagating the error or logging it if it's truly non-critical.
|
|
||
| if requirements.NodeClaim != nil { | ||
| tolerations = requirements.NodeClaim.Tolerations | ||
| tolerations, _ = requirements.NodeClaim.UnmarshalTolerations() |
There was a problem hiding this comment.
The UnmarshalTolerations method returns an error, but it is currently ignored. If unmarshaling fails, tolerations might be an empty slice, leading to incorrect node matching. It's important to handle this error to ensure the new 'Peer Fields' strategy works as intended. Consider propagating the error or logging it.
| tolerations, _ = requirements.NodeClaim.UnmarshalTolerations() | ||
| } | ||
|
|
||
| requirementsRL, _ := requirements.UnmarshalResourceRequest() |
There was a problem hiding this comment.
The UnmarshalResourceRequest method returns an error, but it is currently ignored. If unmarshaling fails, requirementsRL might be an empty ResourceList, leading to incorrect replica estimation. It's important to handle this error to ensure the new 'Peer Fields' strategy works as intended. Consider propagating the error or logging it.
| return 0, framework.AsResult(err) | ||
| } | ||
|
|
||
| requirements, _ := replicaRequirements.UnmarshalResourceRequest() |
There was a problem hiding this comment.
The UnmarshalResourceRequest method returns an error, but it is currently ignored. If unmarshaling fails, requirements might be an empty ResourceList, leading to incorrect quota evaluation. It's important to handle this error to ensure the new 'Peer Fields' strategy works as intended. Consider propagating the error or logging it.
| func (s *SchedulingSimulator) scheduleComponent(component pb.Component) bool { | ||
| requiredPerReplica := util.NewResource(component.ReplicaRequirements.ResourceRequest) | ||
| func (s *SchedulingSimulator) scheduleComponent(component *pb.Component) bool { | ||
| res, _ := component.ReplicaRequirements.UnmarshalResourceRequest() |
There was a problem hiding this comment.
The UnmarshalResourceRequest method returns an error, but it is currently ignored. If unmarshaling fails, res might be an empty ResourceList, leading to incorrect simulation results. It's important to handle this error to ensure the new 'Peer Fields' strategy works as intended. Consider propagating the error or logging it.
| // Set old field | ||
| m.ResourceRequest = make(map[string]*resource.Quantity) | ||
| for k, v := range res { | ||
| q := v |
There was a problem hiding this comment.
For consistency and clarity, consider adding a // copy comment here, similar to line 130 in the ReplicaRequirements.SetResourceRequest method. This helps to explicitly state the intention of copying the loop variable v.
zhzhuang-zju
force-pushed
the
protoc
branch
3 times, most recently
from
cbdbb02 to
d7bbc4f
Compare
Signed-off-by: zhzhuang-zju <m17799853869@163.com>
What type of PR is this?
/kind api-change
What this PR does / why we need it:
This PR migrates the scheduler-estimator protobuf implementation to use the standard
protoc-gen-gotoolchain, replacing the legacygo-to-protobufgenerator. This change is a proactive adaptation for Kubernetes 1.35+ dependencies.Background:
Kubernetes v1.35 has removed the
ProtoMessage()marker method from core REST API types (e.g., Pod, Node). To provide a buffer for downstream projects like Karmada, upstream Kubernetes introduced a temporary build tag,kubernetes_protomessage_one_more_release, which forces these types to implement an emptyProtoMessage()method. However, this compatibility layer is slated for complete removal in Kubernetes v1.36. This PR ensures Karmada is fully decoupled from this dependency ahead of time.Solution:
Adopted a "Peer Fields" strategy:
bytesfields (e.g.,NodeAffinityBytes) alongside existing fields to support direct binary transmission of K8s objects.protoc-gen-goandprotoc-gen-go-grpc.MustSet...,Unmarshal...) to handle the conversion between K8s Go types and the new protobuf structure seamlessly.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: