-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Persisted shared ports during inplace update #9736
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
scheduler/util.go
Outdated
DiskMB: int64(update.TaskGroup.EphemeralDisk.SizeMB), | ||
DiskMB: int64(update.TaskGroup.EphemeralDisk.SizeMB), | ||
Ports: update.Alloc.AllocatedResources.Shared.Ports, | ||
Networks: update.Alloc.AllocatedResources.Shared.Networks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickethier pining if you happen to know, but noticed networks was dropped too, I'm guessing we want to carry over all fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, but I think we'd want a copy of it. I wonder if we should just set Shared
on line 730 to update.Alloc.AllocatedResources.Shared.Copy()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, its not clear to me why DiskMB
is update.TaskGroup.EphemeralDisk.SizeMB
instead of update.Alloc.AllocatedResources.Shared.DiskMB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, above Ports and Networks are set from the existing alloc because those cannot be updated inplace and is guarded by tasksUpdated
.
The same is happening when setting the DiskMB
here. So I think we can leave as is but would still add a .Copy()
to the Networks
.
AllocatedSharedResources were not being copied over to the new allocation struct the scheduler makes during inplace updates. This caused downstream issues after the plan was applied, namely the shared ports were dropped causing issues with service registration/deregistration. test that shared ports are preserved change log, also carry over shared network copy networks
a481ae0
to
1cc88d7
Compare
AllocatedSharedResources were not being copied over to the new allocation struct the scheduler makes during inplace updates. This caused downstream issues after the plan was applied, namely the shared ports were dropped causing issues with service registration/deregistration. test that shared ports are preserved change log, also carry over shared network copy networks
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Ensure that allocation AllocatedResources Shared Ports are not dropped during inplace updates.
When performing in place updates, the scheduler creates a shallow copy of the allocation and updates a few fields. It's not immediately clear to me why we do a shallow copy instead of something like
allocation.Copy()
. The AllocatedSharedResources Ports were not copied over causing issues after the plan was applied.As a side effect, removing or updating a task group service stanza, resulting in an inplace update would error and prevent a service from being registered or deregistered during an inplace update. This issue was reported in #9360
This error was displayed in the logs like so:
fixes #9360
fixes #9735