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

ZED/zfs-list-cacher.sh: don't exit on ignored event type #11347

Merged
merged 1 commit into from
Dec 18, 2020

Conversation

InsanePrawn
Copy link
Contributor

@InsanePrawn InsanePrawn commented Dec 15, 2020

Motivation and Context

13d6598 excluded history_events from logging by default.
This breaks the zfs-list caching mechanism as it listens for those events to trigger cache updates.

Closes #11164.

Description

This change simply stops ignoring history_event type events again.
I suppose @don-brady wasn't aware of these being used by the zfs-list-cacher -> mount generator?

Alternatively, one could suggest leaving the new default and adding the configuration change to zfs-mount-generator(8), making the instructions to get it up and running a little more obscure-seeming and harder to properly automate when you want to preserve existing excludes in the file, with the advantage of a possibly maybe sleeker UX for everyone else. While I understand the argument, especially now that this isn't just ZFS on Linux anymore, this would not be my preferred solution.

How Has This Been Tested?

Manually in my zed.rc, change works as expected after restarting ZED.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • (since 2.0?) Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

I'd forgotten these were used by the zfs-list-cacher mechanism as well. The idea here as explained in the comment above was to only exclude them from syslog by default. What if instead of changing this default we removed the zed_exit_if_ignoring_this_event check from zfs-list-cacher.sh/in and replaced it with a check to only act on history events.

@behlendorf behlendorf requested a review from don-brady December 16, 2020 20:42
@InsanePrawn
Copy link
Contributor Author

zed_exit_if_ignoring_this_event

Your suggestion sounds like the best of both worlds.
I hadn't even looked into that script since I assumed it wasn't even getting called.

I see no reason not to go that way but I'm also not familiar with ZED's architecture, as proven above.

@behlendorf
Copy link
Contributor

Something like this should do the job, though I haven't tested it. The ZED calls each registered zedlet for each event, so it need only exit. If you could verify this does work and update the PR I think this would be a reasonable way to handle it.

-zed_exit_if_ignoring_this_event 
+[ "$ZEVENT_SUBCLASS" != "history" ] && exit 0

@InsanePrawn
Copy link
Contributor Author

Something like this should do the job, though I haven't tested it. The ZED calls each registered zedlet for each event, so it need only exit. If you could verify this does work and update the PR I think this would be a reasonable way to handle it.

-zed_exit_if_ignoring_this_event 
+[ "$ZEVENT_SUBCLASS" != "history" ] && exit 0

Thanks. I was a little fast on pushing this, but wondered whether the switch case was enough of a check, since the old exit never protected us from non-history events anyway.
Will test and add your suggestion for maximum performance I guess.

Check for the history_event type instead.

The zfs-list-cacher.sh script currently respects the event types
excluded from syslog(!) in ZED_SYSLOG_SUBCLASS_EXCLUDE.
This makes little sense in this single-purpose script and
silently breaks when history_events are excluded from syslog,
which is the default since 13d6598.

Closes openzfs#11164

Signed-off-by: InsanePrawn <insane.prawny@gmail.com>
@behlendorf
Copy link
Contributor

since the old exit never protected us from non-history events anyway.

Yeah I noticed the same thing. It probably would be fine, but I figured we might as well make it explicit and do the check early.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 17, 2020
@behlendorf behlendorf merged commit 77f09e2 into openzfs:master Dec 18, 2020
@InsanePrawn
Copy link
Contributor Author

@behlendorf Thanks for the guidance and merging!
Should there be a staging branch for 2.1 (there won't be a 2.0.1, right?) so we can start tracking backports like this as early as possible?
It might even be a good idea to create one immediately with every release?

@behlendorf
Copy link
Contributor

In fact there will be 2.0.x releases:

#11314 (comment)

I went ahead and created a zfs-2.0.1-staging branch. Yes, I think it would be good to create one with each release.

@InsanePrawn InsanePrawn changed the title ZED: log history_events by default again ZED/zfs-list-cacher.sh: don't exit on ignored event type Dec 18, 2020
InsanePrawn added a commit to InsanePrawn/zfs that referenced this pull request Dec 18, 2020
Check for the history_event type instead.

The zfs-list-cacher.sh script currently respects the event types
excluded from syslog(!) in ZED_SYSLOG_SUBCLASS_EXCLUDE.
This makes little sense in this single-purpose script and
silently breaks when history_events are excluded from syslog,
which is the default since 13d6598.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: InsanePrawn <insane.prawny@gmail.com>
Closes openzfs#11164
Closes openzfs#11347
behlendorf pushed a commit that referenced this pull request Dec 20, 2020
Check for the history_event type instead.

The zfs-list-cacher.sh script currently respects the event types
excluded from syslog(!) in ZED_SYSLOG_SUBCLASS_EXCLUDE.
This makes little sense in this single-purpose script and
silently breaks when history_events are excluded from syslog,
which is the default since 13d6598.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: InsanePrawn <insane.prawny@gmail.com>
Closes #11164
Closes #11347
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Check for the history_event type instead.

The zfs-list-cacher.sh script currently respects the event types
excluded from syslog(!) in ZED_SYSLOG_SUBCLASS_EXCLUDE.
This makes little sense in this single-purpose script and
silently breaks when history_events are excluded from syslog,
which is the default since 13d6598.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: InsanePrawn <insane.prawny@gmail.com>
Closes openzfs#11164
Closes openzfs#11347
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Check for the history_event type instead.

The zfs-list-cacher.sh script currently respects the event types
excluded from syslog(!) in ZED_SYSLOG_SUBCLASS_EXCLUDE.
This makes little sense in this single-purpose script and
silently breaks when history_events are excluded from syslog,
which is the default since 13d6598.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: InsanePrawn <insane.prawny@gmail.com>
Closes openzfs#11164
Closes openzfs#11347
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZED_SYSLOG_SUBCLASS_EXCLUDE="history_event" breaks history_event-zfs-list-cacher.sh
2 participants