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.

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

Fix action executor to prevent blocking Taskomatic for actions that are already finished #7416

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Fix action executor to prevent blocking Taskomatic for actions that are already finished #7416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion java/code/src/com/redhat/rhn/taskomatic/task/MinionActionExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ public void execute(JobExecutionContext context) {
// HACK: it is possible that this Taskomatic task triggered before the corresponding Action was really
// COMMITted in the database. Wait for some minutes checking if it appears
int waitedTime = 0;
while (countQueuedServerActions(action) == 0 && waitedTime < ACTION_DATABASE_GRACE_TIME) {
while (countQueuedServerActions(action) == 0 && waitedTime < ACTION_DATABASE_GRACE_TIME &&
!allServerActionsFinished(action)) {
action = ActionFactory.lookupById(actionId);
try {
Thread.sleep(ACTION_DATABASE_POLL_TIME);
Expand All @@ -128,6 +129,14 @@ public void execute(JobExecutionContext context) {
return;
}

// Instead of putting the thread to sleep in the loop above, checking if all server actions have already
// finished (they might have been manually canceled, for example) will prevent blocking the Taskomatic for
// actions that will never appear in the database.
if (allServerActionsFinished(action)) {
log.warn("All server actions for action {} are finished. Skipping it.", actionId);
return;
}

if (countQueuedServerActions(action) == 0) {
log.error("Action with id={} has no server with status QUEUED", actionId);
return;
Expand Down Expand Up @@ -192,4 +201,13 @@ private long countQueuedServerActions(Action action) {
.filter(serverAction -> ActionFactory.STATUS_QUEUED.equals(serverAction.getStatus()))
.count();
}

private boolean allServerActionsFinished(Action action) {
return action != null &&
!CollectionUtils.isEmpty(action.getServerActions()) &&
Copy link
Member

Choose a reason for hiding this comment

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

does this mean an action with no server actions does never count as having all server actions finished?

Copy link
Member

Choose a reason for hiding this comment

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

i guess it makes sense given the countQueuedServerActions(action) == 0 check already. I'm not exactly sure in what scenario an action would be saved and scheduled without server actions and then in a separate transaction server actions get added. It sounds like all kinds of problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really know if it is possible to have an action here that has no server actions reaching this point of the code. I agree that it would be very problematic.

I just know that it is not what I'm searching for, maybe it just never happens. My point is that if the action has server actions and they are all finished I should not block the Taskomatic thread waiting for actions to appear as QUEUED in the database.

If this hypothetical case (having action without server actions) eventually happens I just want to keep the behavior exactly like it is today, and that is the reason to check if the list is not empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you OK with that? I can remove this check or even rename the method if you think it is necessary.

Copy link
Member

Choose a reason for hiding this comment

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

i think its fine for now. The code suggests that there are cases where the action has no server actions initially. The whole action hand off between tomcat and taskomatic seems just very fragile so i think this needs some proper way in the future.

action.getServerActions().stream().allMatch(serverAction ->
ActionFactory.STATUS_FAILED.equals(serverAction.getStatus()) ||
ActionFactory.STATUS_COMPLETED.equals(serverAction.getStatus())
);
}
}
1 change: 1 addition & 0 deletions java/spacewalk-java.changes.welder.fix-taskomatic-blocking
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Fix action executor to prevent blocking Taskomatic for actions that are already finished (bsc#1214121)
Loading