affinity: add ids filtering and skip-exist create options #10157
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughAdds optional "skip existing" behavior to affinity-group creation and an IDs filter for retrieval across client HTTP, server handlers, scheduling/affinity core, txn logic, and tests; introduces client options, group ID parsing/collection helpers, and a CreateAffinityGroupsIfNotExists codepath. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIHandler as "API Handler"
participant Manager as "Affinity Manager"
participant Storage as "Group State Storage"
Client->>APIHandler: POST /affinity-groups?skip_exist_check=true (groups)
APIHandler->>Manager: CreateAffinityGroupsIfNotExists(changes)
Manager->>Storage: GetAllAffinityGroupStates()
Storage-->>Manager: existingStates
Manager->>Manager: Filter out existing groups
Manager->>Manager: Validate & build label rules
Manager->>Storage: Save new groups
Storage-->>Manager: persist result
Manager-->>APIHandler: created + existing states
APIHandler-->>Client: 200 {all requested groups}
sequenceDiagram
participant Client
participant APIHandler as "API Handler"
participant Parser as "Parser (parse.go)"
participant Provider as "Group State Provider"
Client->>APIHandler: GET /affinity-groups?ids=id1,id2
APIHandler->>Parser: ParseGroupIDs([id1,id2])
Parser-->>APIHandler: validated IDs
APIHandler->>Provider: CollectGroupStates(provider, ids)
Provider->>Provider: GetAffinityGroupState(id) [per id]
Provider-->>APIHandler: map[id]->GroupState
APIHandler-->>Client: 200 {map of requested group states}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
bot
added
the
size/XXL
lhy1024
force-pushed
the
affinity-pr14
branch
from
cf9b759 to
4106b39
Compare
ti-chi-bot
bot
added
size/XL
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10157 +/- ##
==========================================
- Coverage 78.73% 78.70% -0.04%
==========================================
Files 522 523 +1
Lines 70270 70391 +121
==========================================
+ Hits 55330 55400 +70
- Misses 10938 10984 +46
- Partials 4002 4007 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/http/interface.go`:
- Around line 1273-1296: The Client interface is missing declarations for
methods implemented on *client: add method signatures for
CreateAffinityGroupsWithSkipExistCheck(ctx context.Context, affinityGroups
map[string][]AffinityGroupKeyRange) (map[string]*AffinityGroupState, error) and
GetAffinityGroups(ctx context.Context) (map[string]*AffinityGroupState, error)
to the Client interface so callers using the interface can access these
implementations; update the interface block that declares other affinity group
methods to include these two signatures matching the concrete implementations in
CreateAffinityGroupsWithSkipExistCheck and GetAffinityGroups.
🧹 Nitpick comments (2)
client/http/interface.go (1)
1273-1296: Consider extracting shared logic withCreateAffinityGroups.
CreateAffinityGroupsWithSkipExistCheckduplicates all ofCreateAffinityGroups(lines 1247–1271) except for the URI suffix and request name. A small helper or an optional parameter could eliminate the duplication.♻️ Sketch
-func (c *client) CreateAffinityGroups(ctx context.Context, affinityGroups map[string][]AffinityGroupKeyRange) (map[string]*AffinityGroupState, error) { +func (c *client) createAffinityGroupsInternal(ctx context.Context, affinityGroups map[string][]AffinityGroupKeyRange, skipExistCheck bool) (map[string]*AffinityGroupState, error) { reqGroups := make(map[string]CreateAffinityGroupInput, len(affinityGroups)) for groupID, ranges := range affinityGroups { reqGroups[groupID] = CreateAffinityGroupInput{Ranges: ranges} } req := CreateAffinityGroupsRequest{AffinityGroups: reqGroups} reqJSON, err := json.Marshal(req) if err != nil { return nil, errors.Trace(err) } + uri := AffinityGroups + name := "CreateAffinityGroups" + if skipExistCheck { + uri += "?skip_exist_check=true" + name = "CreateAffinityGroupsWithSkipExistCheck" + } var resp AffinityGroupsResponse err = c.request(ctx, newRequestInfo(). - WithName("CreateAffinityGroups"). - WithURI(AffinityGroups). + WithName(name). + WithURI(uri). WithMethod(http.MethodPost). WithBody(reqJSON). WithResp(&resp)) if err != nil { return nil, err } return resp.AffinityGroups, nil } + +func (c *client) CreateAffinityGroups(ctx context.Context, affinityGroups map[string][]AffinityGroupKeyRange) (map[string]*AffinityGroupState, error) { + return c.createAffinityGroupsInternal(ctx, affinityGroups, false) +} + +func (c *client) CreateAffinityGroupsWithSkipExistCheck(ctx context.Context, affinityGroups map[string][]AffinityGroupKeyRange) (map[string]*AffinityGroupState, error) { + return c.createAffinityGroupsInternal(ctx, affinityGroups, true) +}server/apiv2/handlers/affinity.go (1)
143-151: Minor:ErrBindJSONis a misleading wrapper for a query-parameter parsing error.
strconv.ParseBoolfailure isn't a JSON binding error. Consider using a more appropriate error, e.g.,ErrAffinityGroupContentor a direct bad-request message, to avoid confusing operators reading logs.Suggestion
if rawValue, ok := c.GetQuery("skip_exist_check"); ok { parsed, err := strconv.ParseBool(rawValue) if err != nil { - c.AbortWithStatusJSON(http.StatusBadRequest, errs.ErrBindJSON.Wrap(err).GenWithStackByCause().Error()) + c.AbortWithStatusJSON(http.StatusBadRequest, "invalid skip_exist_check value: "+rawValue) return } skipExistCheck = parsed }
Signed-off-by: lhy1024 <admin@liudos.us>
lhy1024
force-pushed
the
affinity-pr14
branch
from
d729a72 to
f917775
Compare
lhy1024
changed the title
|
@lhy1024: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
ti-chi-bot
bot
added
needs-cherry-pick-release-8.5
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: okJiang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
What problem does this PR solve?
Issue Number: Ref #9764
What is changed and how does it work?
Check List
Tests
Release note
Summary by CodeRabbit
New Features
Tests