RHBZ#1956387 tuned-adm: add --verbose option to verify command #351

pjps · 2021-05-31T10:59:56Z

'tuned-adm verify' command validates system settings against the
active tuned(8) profiles. But it does not show detailed validation
results to the user. User is left to sift through the tuned(8) log
file to figure it out.

Adding '--verbose' option to the tuned-adm command addresses
this issue. It enables tuned(8) daemon to collect and return
detailed validation results back to the tuned-adm program.
tuned-adm displays these results on the console.

It enables users to confirm that active tuned(8) profile settings
are effective on the system.

Fixes: RHBZ#1956387

yarda

Also please don't close PRs and reopen new ones for the same SUBJ. This way the history is lost (or it's very hard to navigate through it). Just checkout your fork, switch to the branch RHBZ#1956387, make the changes, then:

git commit -as --amend
git push -f

yarda · 2021-06-01T13:29:58Z

tuned/plugins/base.py

@@ -447,6 +449,13 @@ def _verify_all_non_device_commands(self, instance, ignore_missing):
 			if new_value is not None:
 				if self._verify_non_device_command(instance, command, new_value, ignore_missing) == False:
 					ret = False
+			cvalue = None


Shouldn't this go to _verify_non_device_command()?

Or to _verify_value() near the log.info().

No, don't think. It has local scope, it's only used for adding it to vLog.

Sorry for confusion, I didn't mean just the cvalue = None but the whole code bellow.

Done. Moved vlog.add_log() and related changes to the _verify_value() function.

yarda · 2021-06-01T14:07:06Z

tuned/plugins/base.py

 			for device in devices:
+				ret = True


This is similar to the _verify_non_device_command() above, now with _verify_device_command().

Yes, here I set ret=True, because while traversing devices say cpu0, cpu1, cpu2 ...
if _verify_device_command() returns False for 'cpu1', 'ret' stays False for subsequent
'cpu2', 'cpu3' even if _verify_device_command() returned True.

This is the same as my comment to line 452.

Moved vlog.add_log() and related changes to the _verify_value() function should address this too.

yarda · 2021-06-01T14:11:59Z

tuned/plugins/plugin_script.py

@@ -68,9 +70,12 @@ def _instance_verify_static(self, instance, ignore_missing, devices):
 			args += ["ignore_missing"]
 		if self._call_scripts(instance._scripts, args) == True:
 			log.info(consts.STR_VERIFY_PROFILE_OK % instance._scripts)
+			msg = consts.STR_VERIFY_PROFILE_OK % instance._scripts


What about putting msg above log.info() and using the msg in the log.info()? Or wrap log.info() and log.error() to e.g. vlog_info(), vlog_error() and call both log.* and vlog.add_log() in it?

Okay, will do => log.info(msg), log.error(msg).

yarda · 2021-06-01T14:13:49Z

tuned/plugins/plugin_sysctl.py

@@ -76,6 +78,8 @@ def _instance_verify_static(self, instance, ignore_missing, devices):
 			if value is not None:
 				if self._verify_value(option, self._cmd.remove_ws(value), self._cmd.remove_ws(curr_val), ignore_missing) == False:


I think _verify_value() could have optional instance_name parameter and could handle the vlog. It would simplify the code on multiple places.

Hmmn, will try.

yarda · 2021-06-01T14:27:52Z

tuned/verifylog.py

+		self.vLog = {}
+		return vlog
+
+	def _display_device(self, log):


I am still not comfortable that it distinguishes plugin type and chooses presentation method according to it. Could it just take lines of strings, i.e. formatted (or preformatted) by the 'plugins' and just output them (or beautify and output)? I.e. leave the basic formatting on the individual plugins. I think there shouldn't be much overhead for transporting the text through the D-Bus.

Actually presentation format is same for all plugins.

plugin_modules and plugin_scripts log has {module: "current_status_message"} format,
rather than (key, expected_value, current_value) format, so they both are presented
via _display_modules() method.

I think for uniformity sake it is better that individual plugins don't format messages but only log values.
And presentation/formatting is handled via vlog._display() method.

yarda · 2021-06-21T08:04:31Z

tuned/plugins/base.py

 		ret = False
 		if current_value is None and ignore_missing:
 			if device is None:
 				log.info(consts.STR_VERIFY_PROFILE_VALUE_MISSING % name)
 			else:
 				log.info(consts.STR_VERIFY_PROFILE_DEVICE_VALUE_MISSING % (device, name))
-			return True
+			ret = True


This change can result in line 559 calling the self._log_verification_result(), which will log additional PROFILE_VALUE_OK, which previously didn't happen.

yarda · 2021-06-21T08:13:12Z

tuned/verifylog.py

+
+		return ret
+
+	def _display_net(self, log):


Isn't the name misleading? It is not only for net, correct?

Changed it to _display_log()

yarda · 2021-06-21T08:17:13Z

tuned/verifylog.py

+		instances = json.loads(self.put_log())
+		for key in instances.keys():
+			print("[%s]" % key)
+			if key in ["modules", "script"]:


I still have problem with the harcoded plugin names. If the log output is different from the "net", could the plugin itself set the logging type? My point is when somebody adds new plugin (or 3rd party upstream unsupported plugin) she or he shouldn't edit verifylog.py (in case of 3rd party plugins she or he even cannot).

Removed the _display_modules() routine and related check if key in ["modules", "script"].
Now there's no check for hard coded plugin names.

If somebody adds a new plugin, calling vlog.add_log() should help it to display verification logs.

Thank you.

yarda · 2021-07-07T18:04:36Z

@pjps could it wait for the next release, i.e. tuned-2.17.0? We have quite tight schedule now for the 2.16.0 (we are a bit behind the schedule). I would like to tag 2.16.0 today or tmrw. Even if not in the 2.16.0 release it's possible to get it to the RHEL by the downstream patch (through the RHEL bugzilla).

pjps · 2021-07-08T08:52:28Z

@pjps could it wait for the next release, i.e. tuned-2.17.0? We have quite tight schedule now for the 2.16.0 (we are a bit behind the schedule). I would like to tag 2.16.0 today or tmrw. Even if not in the 2.16.0 release it's possible to get it to the RHEL by the downstream patch (through the RHEL bugzilla).

I see, okay. Should I rebase this patch on 2.16.0 version?

Thank you.

yarda · 2021-07-08T09:07:46Z

@pjps it will be great (and cleaner) if you can rebase, but it should work even without the rebase.

pjps · 2021-08-16T19:58:52Z

@pjps it will be great (and cleaner) if you can rebase, but it should work even without the rebase.

Done. Revised patch on the latest upstream (v2.16.0) base.

Thank you.

yarda · 2021-08-24T08:06:05Z

/packit rebuild

yarda · 2021-08-30T07:21:26Z

/packit build

yarda

Thanks for rebasing.

One structural change request in the inline comment.

Also could you please add wrapper (e.g. verify_log_add() method or verify_log class with the init and add methods) which would simplify the things for the plugins writers? E.g. the lines b72ead5#diff-73e8896a2ff9deb0d36cdd8200f6f36feffb20b21b3c48016e0c1e247ece2aa3R91-R92 (lines 91-92) I think this could be unified to the one wrapper method (e.g. verify_log_add) and similarly on the other places. I think the logging should be unified where possible (i.e. the same 'msg' to the log and the vlog).

yarda · 2021-08-30T07:37:12Z

tuned/plugins/plugin_scheduler.py

@@ -1055,6 +1057,9 @@ def _verify_all_irq_affinity(self, correct_affinity, ignore_missing):
 						correct_affinity,
 						current_affinity):
 					res = False
+				srq = "IRQ %s %s" % (irq, irqs[irq]["users"])


Could this and the following lines go to the _verify_irq_affinity()? I think it logically belongs there.

'tuned-adm verify' command validates system settings against the active tuned(8) profiles. But it does not show detailed validation results to the user. User is left to sift through the tuned(8) log file to figure it out. Adding '--verbose' option to the tuned-adm command addresses this issue. It enables tuned(8) daemon to collect and return detailed validation results back to the tuned-adm program. tuned-adm displays these results on the console. It enables users to confirm that active tuned(8) profile settings are effective on the system. Fixes: RHBZ#1956387

yarda

Sorry for the delay. It's getting better and better, but I think there are still things to improve, see the inline comments.

yarda · 2022-10-03T08:42:19Z

tuned/daemon/controller.py

@@ -258,17 +260,19 @@ def recommend_profile(self, caller = None):
 			return ""
 		return self._daemon.profile_recommender.recommend()

-	@exports.export("", "b")


We are breaking API here, some other projects rely on it. Could you add new method, like e.g. verify_profile_verbose() or similar?

yarda · 2022-10-03T08:43:56Z

tuned/admin/admin.py

+			if (not verbose):
+				print(" * Use 'tuned-adm verify --verbose' option "
+				"to know differing system settings.")
+			print(" * Sometimes (if some plugins like bootloader are "


I think you should also mention TuneD restart which could also fix things without reboot.

yarda · 2022-10-03T08:58:58Z

tuned/plugins/plugin_modules.py

@@ -119,19 +121,28 @@ def _instance_verify_static(self, instance, ignore_missing, devices):
 			mpath = "/sys/module/%s" % module
 			if not os.path.exists(mpath):
 				ret = False
-				log.error(consts.STR_VERIFY_PROFILE_FAIL % "module '%s' is not loaded" % module)
+				emsg = consts.STR_VERIFY_PROFILE_FAIL % "module '%s' is not loaded" % module
+				log.error(emsg)


Could we simplificate/consolidate this a bit? Because it's code duplication - there are multiple calls of the log.* and vlog.* with the same (e/i) msg (in all plugins, not just here). Could you e.g. add to the base class e.g. methods:

verify_log_pass

verify_log_fail

verify_log_info

and wrap the both log/vlog calls inside it?

yarda requested changes Jun 1, 2021

View reviewed changes

pjps force-pushed the RHBZ#1956387 branch 2 times, most recently from b514a73 to 1398bcc Compare June 16, 2021 09:06

yarda requested changes Jun 21, 2021

View reviewed changes

pjps force-pushed the RHBZ#1956387 branch from 1398bcc to b3390e2 Compare July 7, 2021 17:35

pjps force-pushed the RHBZ#1956387 branch from b3390e2 to b72ead5 Compare August 16, 2021 19:54

yarda requested changes Aug 30, 2021

View reviewed changes

pjps force-pushed the RHBZ#1956387 branch from b72ead5 to cb2e084 Compare September 15, 2022 11:09

yarda requested changes Oct 3, 2022

View reviewed changes

		@@ -76,6 +78,8 @@ def _instance_verify_static(self, instance, ignore_missing, devices):
		if value is not None:
		if self._verify_value(option, self._cmd.remove_ws(value), self._cmd.remove_ws(curr_val), ignore_missing) == False:

RHBZ#1956387 tuned-adm: add --verbose option to verify command #351

Are you sure you want to change the base?

RHBZ#1956387 tuned-adm: add --verbose option to verify command #351

Conversation

pjps commented May 31, 2021

yarda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarda Jun 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarda commented Jul 7, 2021

pjps commented Jul 8, 2021

yarda commented Jul 8, 2021

pjps commented Aug 16, 2021

yarda commented Aug 24, 2021

yarda commented Aug 30, 2021

yarda left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarda Jun 2, 2021 •

edited

yarda left a comment •

edited