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

[core] Fix decode errors from searchkit #861

Closed

Conversation

pponnuvel
Copy link
Member

When UTF-8 decoding fails, searchkit throws an exception.

Passing decode_errors='backslashreplace' to cope with that.

Fixes #860.

@xmkg
Copy link
Contributor

xmkg commented May 10, 2024

@pponnuvel For the files that import FileSearcher from hotsos.core.search, i.e. from hotsos.core.search import (FileSearcher), we can pass decode_errors directly to the super().__init__ :

super().__init__(*args,
max_parallel_tasks=HotSOSConfig.max_parallel_tasks,
max_logrotate_depth=max_logrotate_depth,
**kwargs)

@pponnuvel
Copy link
Member Author

@pponnuvel For the files that import FileSearcher from hotsos.core.search, i.e. from hotsos.core.search import (FileSearcher), we can pass decode_errors directly to the super().__init__ :

super().__init__(*args,
max_parallel_tasks=HotSOSConfig.max_parallel_tasks,
max_logrotate_depth=max_logrotate_depth,
**kwargs)

Ah, I didn't realise there's another FileSearcher in hotsos itself! So that would simplify this change. But is there any situation where we do want to see the decode errors and/or handle it? I don't envisage any - just wonder as this woud prevent such things.

@pponnuvel
Copy link
Member Author

@dosaboy pponnuvel@97b7d05 is the other way. But I've a question.

@dosaboy
Copy link
Member

dosaboy commented May 13, 2024

I had had issues in the past when using decode error handlers since the injected characters can yield unexpected results from the constraints logic. Having said that I think with the current implementation is reasonably safe. I have however in recent times very seldom comes across this kind of error, would you be able to share anything about what file/output it was that failed? I wonder if a more selective approach would be better i.e. not doing this globally.

@pponnuvel
Copy link
Member Author

pponnuvel commented May 13, 2024

@dosaboy
Changing these two is sufficient for the specific sosreport I've issues with:

 hotsos/core/plugins/kernel/kernlog/common.py
 hotsos/core/plugins/lxd/common.py           

So we can limit to just to these two too.

@dosaboy
Copy link
Member

dosaboy commented May 14, 2024

As a "safer" alternative what do you think about putting this behind a config option and making it optional e.g. --handle-utf8-decode-errors. Overkill?

@pponnuvel
Copy link
Member Author

As a "safer" alternative what do you think about putting this behind a config option and making it optional e.g. --handle-utf8-decode-errors. Overkill?

That's an option. But you need to know it first to ignore it. What you'd see is "failed-parts" in hotsos output which could be due to anything - not necessarily because "decode-errors".

Another question is what could be "unsafe" if we ignore it globally. Do we expect only UTF-8 and ignore anything else? or do we want to support multiple encodings?

It depends on what we could do when we hit decode errors . If the answer is "nothing as we only "grep" for ASCII text in all log/config files, so ignore it" then I'd prefer the global change (pponnuvel@97b7d05).

@pponnuvel pponnuvel force-pushed the fix_decode_errors_from_searchkit branch from 3c4b4a4 to 7884f78 Compare May 22, 2024 14:39
@pponnuvel pponnuvel force-pushed the fix_decode_errors_from_searchkit branch 4 times, most recently from 2ce7298 to e206fa5 Compare May 31, 2024 07:41
When UTF-8 decoding fails, searchkit throws an exception.

Passing decode_errors='backslashreplace' to cope with that.

Fixes canonical#860.

Signed-off-by: Ponnuvel Palaniyappan <[email protected]>
@pponnuvel pponnuvel force-pushed the fix_decode_errors_from_searchkit branch from e206fa5 to e2c247b Compare June 14, 2024 18:41
@pponnuvel
Copy link
Member Author

@dosaboy Can we get this in? I hit this with every sosreport from a user...

@pponnuvel pponnuvel closed this Jul 28, 2024
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.

searchkit doesn't handle unicode errors
3 participants