Skip to content

Temporarily disable trap in diff_model as logFileRotate or testLogFileRotate may return non-zero code. #2629

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 4 commits into from
Nov 22, 2021
Merged

Temporarily disable trap in diff_model as logFileRotate or testLogFileRotate may return non-zero code. #2629

merged 4 commits into from
Nov 22, 2021

Conversation

ankedia
Copy link
Member

@ankedia ankedia commented Nov 18, 2021

The wdt shell script or logFileRotate may return non-zero code and it will go to trap if the trap is on. This change temporarily disables the trap and then restores it in the end.

jshum2479
jshum2479 reviewed Nov 18, 2021
View reviewed changes
operator/src/main/resources/scripts/modelInImage.sh Outdated
@@ -1354,7 +1360,7 @@ function wdtRotateAndCopyLogFile() {
testLogFileRotate "${WDT_OUTPUT_DIR}/${logFileName}"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed since logFileRotate already called it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

@ankedia ankedia changed the title Temporarily disable trap in diff_model as it logFileRotate or testLogFileRotate may return non-zero code. Temporarily disable trap in diff_model as logFileRotate or testLogFileRotate may return non-zero code. Nov 18, 2021
@tbarnes-us
Copy link

tbarnes-us commented Nov 20, 2021 •
edited
Loading

I delegate to @jshum2479 - who approved yesterday but hasn't seen today's diff.

Minor comment: stop/start in the rotate function itself looks unhelpful since all of the callers will have already stopped, it implicitly starts a trap that's about to be started by all of the callers, and if a caller actually deliberately had no trap it'd confusingly start one...

@ankedia
Copy link
Member Author

ankedia commented Nov 20, 2021 •
edited
Loading

I delegate to @jshum2479 - who approved yesterday but hasn't seen today's diff.

Minor comment: stop/start in the rotate function itself looks unhelpful since all of the callers will have already stopped, it implicitly starts a trap that's about to be started by all of the callers, and if a caller actually deliberately had no trap it'd confusingly start one...

Currently the rotate function is only called in the modelInImage.sh script which has trap enabled. I have removed the stop/start in wdtRotateAndCopyLogFile function.

@jshum2479 jshum2479 self-requested a review November 20, 2021 13:31
tbarnes-us
tbarnes-us approved these changes Nov 20, 2021
View reviewed changes
Copy link

@tbarnes-us tbarnes-us left a comment

Choose a reason for hiding this comment

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

Welp since Johny is happy I'm happy :)

@rjeberhard rjeberhard merged commit 19c305f into main Nov 22, 2021
@ankedia ankedia deleted the disable_trap_for_wdt_logs branch November 23, 2021 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jshum2479 jshum2479 jshum2479 approved these changes

@tbarnes-us tbarnes-us tbarnes-us approved these changes

@rjeberhard rjeberhard Awaiting requested review from rjeberhard

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
@ankedia @tbarnes-us @jshum2479 @rjeberhard