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

Fix removing tasks when a jobs service is removed #3112

Merged
merged 1 commit into from Feb 6, 2023

Conversation

dperny
Copy link
Collaborator

@dperny dperny commented Jan 25, 2023

Previously, the jobs orchestrators were missing the appropriate code to handle the deletion of a service. As a result, when a service was deleted, the tasks for that service would hang around indefinitely. It's likely that on a leadership change or restart, the tasks would have been deleted, but that's generally not an acceptable process as leadership changes or restarts aren't guaranteed.

Fixes this issue by adding event handling for services, similar to global or replicated services, that moves all tasks for a deleted service into Remove state, flagging them for deletion.

Previously, the jobs orchestrators were missing the appropriate code to
handle the deletion of a service. As a result, when a service was
deleted, the tasks for that service would hang around indefinitely. It's
likely that on a leadership change or restart, the tasks would have been
deleted, but that's generally not an acceptable process as leadership
changes or restarts aren't guaranteed.

Fixes this issue by adding event handling for services, similar to
global or replicated services, that moves all tasks for a deleted
service into Remove state, flagging them for deletion.

Signed-off-by: Drew Erny <derny@mirantis.com>
@dperny
Copy link
Collaborator Author

dperny commented Jan 25, 2023

Fixes moby/moby#44312

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

SGTM, but perhaps someone else wants to give it some eyes as well

@@ -201,6 +201,11 @@ func (o *Orchestrator) handleEvent(ctx context.Context, event events.Event) {
service = ev.Service
case api.EventUpdateService:
service = ev.Service
case api.EventDeleteService:
if orchestrator.IsReplicatedJob(ev.Service) || orchestrator.IsGlobalJob(ev.Service) {
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, but wondering if we should have IsJob() and/or IsService() utility for these

@thaJeztah
Copy link
Member

Not sure what's wrong with CircleCI, but as we're in the process of removing it altogether (things are running in GitHub actions now), I'll probably should just ignore it;

Using SSH Config Dir '/home/circleci/.ssh'
git version 2.38.0
Cloning git repository
Cloning into '.'...
remote: Enumerating objects: 45657, done.
remote: Counting objects: 100% (8854/8854), done.
remote: Compressing objects: 100% (4595/4595), done.
remote: Total 45657 (delta 3960), reused 8218 (delta 3699), pack-reused 36803
Receiving objects: 100% (45657/45657), 39.04 MiB | 36.74 MiB/s, done.
Resolving deltas: 100% (28065/28065), done.
Checking out branch
fatal: reference is not a tree: 75e563b4b3c63a740ac0aa413ad33c039d891f63

exit status 128
CircleCI received exit code 128

@thaJeztah thaJeztah merged commit 2805c9a into moby:master Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants