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

setup: Don’t fail cleanup on empty remote engine environment #492

Merged
merged 1 commit into from
Jul 18, 2022

Conversation

mz-pdm
Copy link
Member

@mz-pdm mz-pdm commented Jun 25, 2022

When engine-setup fails on dnf repositories download, the followup
cleanup fails with

[ ERROR ] Failed to execute stage 'Clean up': 'NoneType' object has no attribute 'cleanup'

This patch fixes it and allows the cleanup finish normally.

Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

+1

@didib
Copy link
Member

didib commented Jun 28, 2022

Not sure I fully understand the flow:

When engine-setup fails on dnf repositories download, the followup cleanup fails with

[ ERROR ] Failed to execute stage 'Clean up': 'NoneType' object has no attribute 'cleanup'

Are you sure it's related to engine-setup failing or not? IIUC, the failure is simply that _setup (of this module) didn't yet run when the tool you run (can be setup, cleanup, whatever) already failed, for whatever reason - dnf repos, or just pressing Ctrl-C at the right moment.

This patch fixes it and allows the cleanup finish normally.

Code looks ok, but might have been cleaner (pun not intended) to move this function instead to STAGE_CLOSEUP. The difference between them is that STAGE_CLEANUP always runs (e.g. to tell the user where the log file is, etc.) and STAGE_CLOSEUP only runs if we didn't fail (and runs non-critical stuff that we can't, or don't want to, rollback - e.g. if a service fails to start, user can just start it manually or reboot the machine).

Not sure why I put it in STAGE_CLEANUP originally. I don't believe there was a specific need.

@mz-pdm mz-pdm force-pushed the setup-nonetype# branch 2 times, most recently from 084d0ef to 1b94f27 Compare June 29, 2022 17:31
@mz-pdm
Copy link
Member Author

mz-pdm commented Jun 29, 2022

IIUC, the failure is simply that _setup (of this module) didn't yet run when the tool you run (can be setup, cleanup, whatever) already failed, for whatever reason - dnf repos, or just pressing Ctrl-C at the right moment.

Yes, this is the probable cause.

Code looks ok, but might have been cleaner (pun not intended) to move this function instead to STAGE_CLOSEUP.
...
Not sure why I put it in STAGE_CLEANUP originally. I don't believe there was a specific need.

OK, let's do it this way.

When engine-setup fails on dnf repositories download, the followup
cleanup fails with

[ ERROR ] Failed to execute stage 'Clean up': 'NoneType' object has no attribute 'cleanup'

This patch moves the corresponding cleanup call from STAGE_CLEANUP to
STAGE_CLOSEUP.  STAGE_CLOSEUP only runs if we didn't fail and runs
non-critical stuff that we can't, or don't want to, rollback.  This
way, we can be sure that the cleanup is not run before the given
environment item is assigned.
@mz-pdm
Copy link
Member Author

mz-pdm commented Jul 8, 2022

/ost

@mz-pdm
Copy link
Member Author

mz-pdm commented Jul 11, 2022

Seems to be working. @didib WDYT, can we take it?

@didib didib merged commit 39c83f2 into oVirt:master Jul 18, 2022
@mz-pdm mz-pdm deleted the setup-nonetype# branch August 9, 2022 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants