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

Ignore stdout/stderr messages before "yum history list" #158

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

Wescoeur
Copy link
Member

Yum stderr logs as "Warning: RPMDB modified outside of yum." are appended to the SSH output which completely breaks the history parsing. To solve this problem, we need to skip all log lines before transactions.

Yum stderr logs as "Warning: RPMDB modified outside of yum."
are appended to the SSH output which completely breaks the history parsing.
To solve this problem, we need to skip all log lines before transactions.

Signed-off-by: Ronan Abhamon <[email protected]>
@ydirson
Copy link
Contributor

ydirson commented Jul 28, 2023

Maybe explain why those warnings are here ?

@Wescoeur
Copy link
Member Author

This warn is an example. It's not the first time that we have a problem like this with a yum command, this fix is here to handle all cases, not just this one.

But for the context of this example, you can get this trace when you install an RPM without using a yum command.

@ydirson
Copy link
Contributor

ydirson commented Jul 28, 2023

But for the context of this example, you can get this trace when you install an RPM without using a yum command.

This precise case would possibly be better fixed by using yum to install a local RPM file, since that works too.

I'm a bit wary of ignoring input more largely, as this can help us miss some real issues.

@benjamreis
Copy link
Contributor

The example is a benine warning so ignoring it is the correct way to handle.
Also having a specific behavior for every warning would be too complex.
The error is still logged as before so in case a non benine error happened (which hasn't for now) the test would fail and we'd see it in the logs.

@ydirson
Copy link
Contributor

ydirson commented Jul 28, 2023

This specific warning is benign and we know how to avoid it.

My point is that this patch ignores all warnings -- how can we be sure that all the warnings we can get will be benign as well?
I'm still not convinced.

@Wescoeur
Copy link
Member Author

I'm not convinced to abort when there is a warn. A warn is not an error. From my POV, If we can install packages without issue just after this savepoint, it's ok.

Also: the warns were totally ignored before my previous commit and the yum rollback command crashed because we didn't have a valid tid. And of course we didn't have the source of the problem at all. Now we have a log + always a valid tid.

@ydirson
Copy link
Contributor

ydirson commented Jul 28, 2023

A warning for a human operator is not an error, but there is a reason why compiler flags like -Werror exist: they are here to warn the dev that something suspect is happening - as in the case described above, even though the impact of that particular case may be benign.

A warning is something like "this is not necessarily an error, I'm not sure what you're doing, please be more specific or fix the problem if it was not intended". Else there would be no warning in the first place.

@benjamreis
Copy link
Contributor

Until now all warning has been benign.
If something else comes up we'll treat the specific case.

@ydirson
Copy link
Contributor

ydirson commented Jul 28, 2023

Until now all warning has been benign.

"jusqu'ici tout va bien" :D

If something else comes up we'll treat the specific case.

If something else comes up, my understanding is it be silently dropped and we won't see we have another specific case to handle - or did I miss something?

@benjamreis
Copy link
Contributor

tested and validated.

@benjamreis benjamreis merged commit 81db96b into master Jul 31, 2023
4 checks passed
@benjamreis benjamreis deleted the fix-stderr-get_last_yum_history_tid branch July 31, 2023 09:01
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.

4 participants