Skip to content

[slice] Create Slice for each PodSet in Workload. #520

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 1, 2025
Merged

[slice] Create Slice for each PodSet in Workload. #520

merged 2 commits into from
Aug 1, 2025

Conversation

mbobrovskyi
Copy link
Collaborator

Fixes / Features

  • Create Slice for each PodSet in Workload.

@mbobrovskyi mbobrovskyi requested a review from IrvingMg July 7, 2025 14:28
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch from 91c16b1 to c75be04 Compare July 7, 2025 14:52
@mbobrovskyi mbobrovskyi marked this pull request as ready for review July 7, 2025 14:54
@mbobrovskyi mbobrovskyi changed the title Create Slice for each PodSet in Workload. [slice] Create Slice for each PodSet in Workload. Jul 7, 2025
@mbobrovskyi mbobrovskyi mentioned this pull request Jul 7, 2025
Closed
IrvingMg
IrvingMg reviewed Jul 7, 2025
View reviewed changes
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch 2 times, most recently from 7fd40a4 to d4dad14 Compare July 8, 2025 07:19
IrvingMg
IrvingMg reviewed Jul 8, 2025
View reviewed changes
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch from d4dad14 to a8e0871 Compare July 8, 2025 07:42
@IrvingMg
Copy link
Collaborator

IrvingMg commented Jul 8, 2025

LGTM.

cc: @PBundyra @gabesaba

@mbobrovskyi mbobrovskyi mentioned this pull request Jul 15, 2025
Merged
@mbobrovskyi
Copy link
Collaborator Author

@gabesaba PTAL

@gabesaba
Copy link
Collaborator

@gabesaba PTAL

Let's put this PR on hold until the other two (#521, #517) merge. It will be more overhead given that they're also modifying workload controller, and the behavior of those two (especially #521) is more fundamental

pajakd
pajakd reviewed Jul 28, 2025
View reviewed changes
pajakd
pajakd reviewed Jul 28, 2025
View reviewed changes
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch from a8e0871 to faaacef Compare July 30, 2025 08:28
pajakd
pajakd reviewed Jul 30, 2025
View reviewed changes
pajakd
pajakd reviewed Jul 30, 2025
View reviewed changes
pajakd
pajakd reviewed Jul 30, 2025
View reviewed changes
mbobrovskyi
mbobrovskyi commented Jul 30, 2025
View reviewed changes
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch 3 times, most recently from 38c1d4f to 81637a6 Compare July 30, 2025 11:40
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch from fe3d460 to cd17118 Compare August 1, 2025 06:02
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch from cd17118 to bad2360 Compare August 1, 2025 06:20
pajakd
pajakd reviewed Aug 1, 2025
View reviewed changes
pajakd
pajakd reviewed Aug 1, 2025
View reviewed changes
pajakd
pajakd reviewed Aug 1, 2025
View reviewed changes
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch from 7e78b87 to dd1f55e Compare August 1, 2025 10:17
gabesaba
gabesaba reviewed Aug 1, 2025
View reviewed changes
gabesaba
gabesaba reviewed Aug 1, 2025
View reviewed changes
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch from dd1f55e to c2bc1ba Compare August 1, 2025 11:10
gabesaba
gabesaba reviewed Aug 1, 2025
View reviewed changes
pajakd
pajakd reviewed Aug 1, 2025
View reviewed changes
pajakd
pajakd reviewed Aug 1, 2025
View reviewed changes
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch from c2bc1ba to e4f97e2 Compare August 1, 2025 12:15
pajakd
pajakd reviewed Aug 1, 2025
View reviewed changes
@mbobrovskyi mbobrovskyi force-pushed the slice/multiple-slice-for-workload branch from e4f97e2 to 504a983 Compare August 1, 2025 12:51
@mbobrovskyi mbobrovskyi requested review from pajakd and gabesaba August 1, 2025 12:52
@pajakd
Copy link
Collaborator

pajakd commented Aug 1, 2025

/lgtm

gabesaba
gabesaba reviewed Aug 1, 2025
View reviewed changes
slice/internal/controller/workload_controller.go
Comment on lines +429 to +432
capacity := len(wl.Status.Admission.PodSetAssignments) - len(slicesByName)
if capacity < 0 {
capacity = 0
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this ever be negative? and this is just for sizing the slice? I think i'm in favor of deleting this logic, but we can do it in a follow-up PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be negative if the user manually creates extra Slices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#572

gabesaba
gabesaba reviewed Aug 1, 2025
View reviewed changes
slice/internal/controller/workload_controller.go
subblockLevelIndex := topology.SubblockLevelIndex(&psa)
for _, domain := range psa.TopologyAssignment.Domains {
nodeSelectors.Insert(domain.Values[subblockLevelIndex])
sliceName := core.SliceName(wl.Name, psa.Name)
Copy link
Collaborator

@gabesaba gabesaba Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In follow-up PR:

Should we use IsRelevantPodTemplateSpec in this loop? Otherwise, we may try to create a slice for an invalid podset. Can you add a test for this please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #573.

@gabesaba
Copy link
Collaborator

gabesaba commented Aug 1, 2025

lgtm, thank you! Let's address the last comments in a follow-up PR.

@gabesaba gabesaba merged commit 7f83d39 into slice-main Aug 1, 2025
18 of 22 checks passed
@gabesaba gabesaba deleted the slice/multiple-slice-for-workload branch August 1, 2025 14:26
This was referenced Aug 1, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabesaba gabesaba gabesaba left review comments

@mwysokin mwysokin Awaiting requested review from mwysokin

@mimowo mimowo Awaiting requested review from mimowo

@PBundyra PBundyra Awaiting requested review from PBundyra PBundyra is a code owner

@mwielgus mwielgus Awaiting requested review from mwielgus

@IrvingMg IrvingMg Awaiting requested review from IrvingMg

@pajakd pajakd Awaiting requested review from pajakd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mbobrovskyi @IrvingMg @gabesaba @pajakd