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

Evergreen: Update to use stop-orchestration.sh #1621

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Conversation

rozza
Copy link
Member

@rozza rozza commented Feb 4, 2025

JAVA-5778
@rozza rozza requested review from a team and katcharov and removed request for a team February 4, 2025 11:21
@rozza
Copy link
Member Author

rozza commented Feb 4, 2025

Example in the logs:

elif [ -f venv/Scripts/activate ]; then
. venv/Scripts/activate
fi
mongo-orchestration stop || true
Copy link
Collaborator

Choose a reason for hiding this comment

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

bash: line 20: mongo-orchestration: command not found

From the linked log, it looks like this did not cause a system failure, presumably because of the || true. Would there be a problem if we removed this from this command?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah that stops the system failure error that would happen without 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.

To clarify - we could remove the || true part but if the command fails then there is no more clean up. Which I think is the reason it was added in the first place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, iirc it's because in some variants we don't start mongo orchestration but we always attempt to stop it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason we always stop it is because if we put it in task which starts mongo-orchestration, it might be skipped if the testing failed; to avoid that ugliness, we put it into the clearup post step to ensure it is run always, even for static-analysis task.

Copy link
Contributor

Choose a reason for hiding this comment

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

In mongo-hibernate project, a separate post step is created (rather than as a step in clearup), so no need to use the || true hack.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rozza I see. If this is corresponds only to "bootstrap mongo-orchestration", then perhaps we could set an env var in that func, and then check it here? I think we can ensure that we rm -rf (which is the last part of cleanup) with the following, though it's a bit long:


if [ -n "$MONGO_ORCHESTRATION_STARTED" ]; then
  MONGO_ORCHESTRATION_OUTPUT=$(mongo-orchestration stop)
  MONGO_ORCHESTRATION_EXIT_CODE=$?
  echo "$MONGO_ORCHESTRATION_OUTPUT"
fi

cd -
rm -rf "$DRIVERS_TOOLS"

if [ $MONGO_ORCHESTRATION_EXIT_CODE -ne 0 ]; then
  exit $MONGO_ORCHESTRATION_EXIT_CODE
fi

(Don't know if I have the cd - in the right place.)

Not sure if this is worth it - up to you. LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

there must be some elegant way for sure; but conservativeness usually reigns in devops process.

I created my PR for mongo-hibernate at mongodb/mongo-hibernate#36. I assigned @rozza as one reviewer. Thanks for your reminder.

@jyemin jyemin self-requested a review February 4, 2025 15:22
Copy link
Collaborator

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

LGTM pending resolution of Max's comment

@rozza rozza requested a review from katcharov February 4, 2025 15:38
@rozza rozza closed this Feb 5, 2025
@rozza rozza reopened this Feb 5, 2025
@rozza rozza merged commit 80fbcb1 into mongodb:main Feb 5, 2025
55 of 60 checks passed
@rozza rozza deleted the JAVA-5778 branch February 5, 2025 10:44
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

4 participants