Expose TuneD API to the Unix Domain Socket. #470

CZerta · 2022-10-26T14:32:00Z

TuneD is listening on paths specified in config in option unix_socket_paths and send signals to paths in option unix_socket_signal_paths. Example call:

ssock_file = "/tmp/tuned_socket"
csock_file = "/tmp/tuned_socket_client"
if os.path.exists(csock_file):
os.remove(csock_file)
s=socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM) s.bind(csock_file)
try:
s.sendto(pickle.dumps(("switch_profile", "balanced")), ssock_file)
print(pickle.loads(s.recvfrom(4096)[0]))
except Exception as e:
print("E: %s" % e)
s.close()

This PR also introduce possibility to disable dbus API in main TuneD config.

Resolves: rhbz#2113900

Signed-off-by: Jan Zerdik jzerdik@redhat.com

lgtm-com · 2022-10-26T14:38:46Z

This pull request introduces 3 alerts when merging 9e71402 into 420267f - view on LGTM.com

new alerts:

3 for Unused import

yarda · 2022-11-08T21:25:57Z

tuned-main.conf

@@ -49,3 +49,22 @@ log_file_max_size = 1MB
 # It can be used to force tuning for specific architecture.
 # If commented, "/proc/cpuinfo" will be read to fill its content.
 # cpuinfo_string = Intel
+
+# Enable TuneD listening on dbus
+# dbus = 1


Maybe use "control_dbus" and "control_unix_socket" or similar to explicitly mark that these two options are related to controlling.

yarda · 2022-11-08T21:38:24Z

tuned-main.conf

+# Enable TuneD listening on unix domain socket
+# unix_socket = 1
+
+# Paths to sockets for TuneD to listen separated by , or ;


Do we need multiple sockets? I am afraid this could lead to conflicts. I think enough is communication with just the one process which holds the single control socket opened.

One socket, but multiple clients for sure.

yarda · 2022-11-08T21:38:56Z

tuned-main.conf

+# unix_socket_permissions = 0o444
+
+# Length of datagrams received/send through sockets
+# unix_socket_byte_len = 4096


No new line at EOF.

yarda · 2022-11-08T21:52:56Z

tuned/exports/unix_socket_exporter.py

+			data = pickle.loads(data)
+			if type(data) not in (tuple, list) or len(data) < 1:
+				log.error("Wrong format of call through unix domain socket")
+				conn.sendto(pickle.dumps("Wrong format of call through unix domain socket"), address)


Maybe send to the socket just "Wrong format of call", the current string seems unnecessary long.

yarda · 2022-11-08T22:15:39Z

tuned/exports/unix_socket_exporter.py

+		if os.path.exists(csock_file):
+			os.remove(csock_file)
+		s=socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM)
+		s.bind(csock_file)


With only one socket the client should use the s.connect(SOCKET_ADDR)

yarda · 2022-11-08T22:16:04Z

tuned/exports/unix_socket_exporter.py

+		s=socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM)
+		s.bind(csock_file)
+		try:
+			s.sendto(pickle.dumps(("switch_profile", "balanced")), ssock_file)


And s.send(DATA)

yarda · 2022-11-08T22:16:46Z

tuned/exports/unix_socket_exporter.py

+		# dirty hack that fixes KeyboardInterrupt handling
+		# the hack is needed because PyGObject / GTK+-3 developers are morons
+		signal_handler = signal.getsignal(signal.SIGINT)
+		self._main_loop = GLib.MainLoop()


Do we need another GLib.MainLoop and another thread? Isn't it overhead? Could we use 'select' call in the daemon main event loop?

yarda · 2022-11-08T22:29:51Z

tuned/exports/unix_socket_exporter.py

+		s.close()
+		"""
+		try:
+			data, address = conn.recvfrom(self._byte_len)


What about messages longer than 4096 bytes? Could the profile name overflow it? (IIRC PATH_MAX is 4096 on Linux). Maybe the socket protocol could use int(MSG_SIZE), MSG (i.e. code the packet length, read the packet containing the integer with the fixed size and then read the MSG_SIZE packet) or parse packets for delimiters.

yarda · 2022-11-08T22:47:34Z

tuned/exports/unix_socket_exporter.py

+		s=socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM)
+		s.bind(csock_file)
+		try:
+			s.sendto(pickle.dumps(("switch_profile", "balanced")), ssock_file)


pickle is python specific. It could pose problems if the control client needs to be written in different language. Could it use something more portable? E.g. JSON. Or pure ASCII/UTF-8? And then it can code numbers as strings. In such case you could use newlines as delimiters and you needn't to code the packet length, e.g.:

<switch_profile balanced\n .... >profile_changed\n

yarda · 2022-11-08T22:50:27Z

tuned-main.conf

+# unix_socket = /tmp/tuned_socket
+
+# Paths to sockets for TuneD to send signals to separated by , or ;
+# unix_socket_signal_paths =


Do we need extra signalling socket? I think it's currently used only for signalization that the profile loading is finished. I.e. it should be enough to notify just the controlling process which holds the socket, i.e.:

>switch_profile(balanced) ... <profile_changed

yarda · 2022-11-08T23:10:35Z

tuned/consts.py

+CFG_DEF_UNIX_SOCKET_SIGNAL_PATHS = ""
+# default unix socket permissions
+CFG_DEF_UNIX_SOCKET_PERMISIONS = 0o444
+CFG_FUNC_UNIX_SOCKET_PERMISIONS = "getint"


What is it used for?

This is default value and get function describing type of option. See

tuned/tuned/utils/global_config.py

Line 28 in 420267f

options = [opt for opt in dir(consts)

yarda · 2022-11-08T23:11:08Z

tuned/consts.py

+CFG_FUNC_UNIX_SOCKET_PERMISIONS = "getint"
+# default byte len for unix sockets
+CFG_DEF_UNIX_SOCKET_BYTE_LEN = 4096
+CFG_FUNC_UNIX_SOCKET_BYTE_LEN = "getint"


What is it used for?

yarda · 2022-11-08T23:12:15Z

tuned/consts.py

-
+# default listening on dbus
+CFG_DEF_DBUS = True
+CFG_FUNC_DBUS = "getboolean"


What is it used for?

lgtm-com · 2022-11-21T09:32:14Z

This pull request introduces 3 alerts when merging 1fa7493 into 420267f - view on LGTM.com

new alerts:

3 for Unused import

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

jmencak

Thank you for the PR! I didn't go through the code carefully (yet), but I have a few initial observations. Also, running the demo with this code:

#!/usr/bin/python3

import os
import socket
import pickle
import json
import struct

def sss(d):
	ssock_file = "/tmp/tuned_socket"
	csock_file = "/tmp/tuned_socket_client"
	if os.path.exists(csock_file):
		os.remove(csock_file)
	s=socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM)
	s.bind(csock_file)
	j = json.dumps(d).encode("utf-8")
	try:
		s.sendto(struct.pack("!L", len(j)), ssock_file)
		s.sendto(j, ssock_file)
		d = s.recvfrom(5)
		t, l = struct.unpack("!sL", d[0])
		print("TYPE: %s" % t)
		print("LEN: %s" % l)
		data = json.loads(s.recvfrom(l)[0])
		print("DATA: %s" % data)
	except Exception as e:
		print("E: %s" % e)
	s.close()
	
sss(("profiles",))

Takes more than 7 seconds for me. Is this expected?

[root@b86 tuned]# time ./talk.py
TYPE: b'r'
LEN: 434
DATA: ['accelerator-performance', 'balanced', 'bz', 'chrony', 'cpu-partitioning', 'desktop', 'empty', 'hpc-compute', 'inheritance', 'intel-sst', 'latency-performance', 'loop', 'network-latency', 'network-throughput', 'openshift-realtime', 'optimize-serial-console', 'plus', 'powersave', 'provider-', 'provider-gce', 'provider_', 'realtime', 'scheduler', 'script', 'stalld', 'test', 'throughput-performance', 'virtual-guest', 'virtual-host']

real    0m7.777s
user    0m0.025s
sys     0m0.004s

jmencak · 2022-11-21T11:52:46Z

tuned-main.conf

+
+# Path to socket for TuneD to listen
+# Existing files on given path will be removed
+# unix_socket_path = /tmp/tuned_socket


Wouldn't /run/tuned/tuned.sock be a better default place for the socket? That seems to be what other components such as chrony, cups, ... use.

Wouldn't /run/tuned/tuned.sock be a better default place for the socket? That seems to be what other components such as chrony, cups, ... use.

Depends how we decide.

Takes more than 7 seconds for me. Is this expected?

jarda wanted to implement this without special thread and associated overhead, so it checks for new messages every 10 secs in "main" daemon loop.

There should be inner and outer loops, booth configurable. The outer loop should run each 1 s, the inner each 10 s, I think the code should check the socket in the outer loop, which should speed the response up by factor of ten, which should be OK for most applications.

Sorry, my mistake, I moved the check to proper loop and it should process calls in less then second with default setting.

Even speedup by a factor by 10 might not be sufficient. The socket interface is intended to be in the path of a container being created and synchronous. I.e. it will block/delay the container from starting. How much of an issue this is, I'm not sure at the moment, but this needs discussing with Martin Sivak (don't seem to be able to tag him here).

Imagine trying to start 200 containers on system reboot. One sec is probably OK as long as it does not add up with load. I would have to test how exactly cri-o handles this, but I sure hope at this phase the containers are not sequential. In such case this must support 200 or more simultaneous connections that are then processed in batches of all that arrive within the same second.

Also the latency is usually tied to the power consumption.

Nevertheless, I think on the control interface side there is still a room for improvement. E.g. the select could be used for timing of the main loop and exit sooner than 1s if there is message, process it and sleep for the rest of time (in select).

But it will not improve the profile switch latency.

Imagine trying to start 200 containers on system reboot. One sec is probably OK as long as it does not add up with load. I would have to test how exactly cri-o handles this, but I sure hope at this phase the containers are not sequential. In such case this must support 200 or more simultaneous connections that are then processed in batches of all that arrive within the same second.

This will require some serialization code on the TuneD side, the current PoC PR #477 will not scale well.

Wouldn't /run/tuned/tuned.sock be a better default place for the socket? That seems to be what other components such as chrony, cups, ... use.

Depends how we decide.

I would strongly suggest we decide to use /run/tuned/tuned.sock unless I'm missing some important reason against that location. On modern OSes, /run is also mounted with tmpfs as well as /tmp. Also, to my mind, /tmp is really the last resort to put files important files in not only for security, but also for "keeping things nice and tidy" reason. /run/tuned belongs to TuneD, so why not keep it there.

jmencak · 2022-11-21T11:56:48Z

tuned-main.conf

+# unix_socket_signal_paths =
+
+# Permissions for listening sockets
+# unix_socket_permissions = 0o444


This doesn't seem to work for me. When uncommenting this line, I get:

Traceback (most recent call last): File "/usr/sbin/tuned", line 53, in <module> config = GlobalConfig() File "/usr/lib/python3.6/site-packages/tuned/utils/global_config.py", line 15, in __init__ self.load_config(file_name=config_file) File "/usr/lib/python3.6/site-packages/tuned/utils/global_config.py", line 51, in load_config self._cfg[option] = func(consts.MAGIC_HEADER_NAME, option) File "/usr/lib64/python3.6/configparser.py", line 819, in getint fallback=fallback, **kwargs) File "/usr/lib64/python3.6/configparser.py", line 809, in _get_conv **kwargs) File "/usr/lib64/python3.6/configparser.py", line 803, in _get return conv(self.get(section, option, **kwargs)) ValueError: invalid literal for int() with base 10: '0o444'

Good catch, classic reading from config does not support non 10-base literals, I extended it little bit, so should be fixed.

This is now fixed, thank you. However, the default permissions do not make sense to me. I guest this was only tested with root user permissions. In that case, 444 doesn't really matter. However, to quote:

sending a datagram to a datagram socket likewise requires write permission on that socket.

see: https://man7.org/linux/man-pages/man7/unix.7.html

I'd suggest 0o600, similarly CFG_DEF_UNIX_SOCKET_PERMISIONS = "0o600".

On a related note, configuring access permissions on its own does not make much sense unless you couple it with also the possibility to change the owner/group. That functionality could be useful for others, even though for our use case having only root to use the socket is good enough.

jmencak · 2022-11-22T08:23:33Z

tuned/consts.py

+CFG_DEF_UNIX_SOCKET_PATH = "/tmp/tuned_socket"
+CFG_DEF_UNIX_SOCKET_SIGNAL_PATHS = ""
+# default unix socket permissions
+CFG_DEF_UNIX_SOCKET_PERMISIONS = "0o444"


I'd suggest changing this to 0o600.

jmencak · 2022-11-22T10:10:23Z

During testing, I noticed I can DoS TuneD over the socket like this:

printf '\x00\x00\x00\x03' | nc -u -U /run/tuned/tuned.sock
printf 'DoS' | nc -u -U /run/tuned/tuned.sock

2022-11-22 11:07:55,987 ERROR    tuned.exports.unix_socket_exporter: Failed to load json data 'b'DoS'': Expecting value: line 1 column 1 (char 0)
Exception in thread Thread-1 (_thread_code):
Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 185, in period_check
    data = json.loads(data.decode("utf-8"))
  File "/usr/lib64/python3.10/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/usr/lib64/python3.10/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib64/python3.10/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib64/python3.10/threading.py", line 1016, in _bootstrap_inner
    self.run()
  File "/usr/lib64/python3.10/threading.py", line 953, in run
    self._target(*self._args, **self._kwargs)
  File "/usr/lib/python3.10/site-packages/tuned/daemon/daemon.py", line 221, in _thread_code
    exports.period_check()
  File "/usr/lib/python3.10/site-packages/tuned/exports/__init__.py", line 46, in period_check
    return ctl.period_check()
  File "/usr/lib/python3.10/site-packages/tuned/exports/controller.py", line 54, in period_check
    exporter.period_check()
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 188, in period_check
    self._send_data(self._socket_object, address, "err", "Failed to load json data '%s': %s" % (data, e))
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 131, in _send_data
    s.sendto(struct.pack("!sL", type.encode("utf-8"), len(j)), address)
FileNotFoundError: [Errno 2] No such file or directory

While the client should be trusted and send valid input, it would be nice if the server (TuneD) wouldn't fall over that easily.

CZerta · 2022-11-22T10:24:30Z

...

While the client should be trusted and send valid input, it would be nice if the server (TuneD) wouldn't fall over that easily.

@jmencak Did you pull the newest version? I added some tries around json loading yesterday, so it should handle invalid requests better.

jmencak · 2022-11-22T10:35:22Z

Did you pull the newest version? I added some tries around json loading yesterday, so it should handle invalid requests better.

Yes, I did, you cannot reproduce this?

jmencak · 2022-11-22T11:37:03Z

It would also help documenting the API a bit more. It looks like I cannot send both the payload length header and JSON payload in one go (over one sendto call).

CZerta · 2022-11-22T11:44:00Z

You can, I'll write better example. I found out what's problem with the exception. ncat does not wait for response, deletes it's socket before we process the request and try to send response to non existing file, so it fails.

jmencak · 2022-11-24T08:22:44Z

You can, I'll write better example.

Great, in the end though, we'll probably need a golang client. I managed to write one.

I found out what's problem with the exception. ncat does not wait for response, deletes it's socket before we process the request and try to send response to non existing file, so it fails

Yeah, I can see that now. However, that's no reason for the server to fail like that. Imagine the client doesn't care about the response like the netcat example or just crashes before getting a response back. That's a valid use case for me in my point of view and the server should not fail.

CZerta · 2022-11-24T10:49:36Z

@jmencak Definitely, I added warning log when sending msg fails with reason. Also changed default path and permissions and added ownership option.

jmencak · 2022-11-24T16:39:52Z

tuned-main.conf

+# unix_socket_signal_paths =
+
+# default unix socket ownership
+# (uid and gid, python2 does not support names out of box, -1 leaves default)


While I applaud the effort to go the extra mile to keep things compatible, we should really consider dropping support for python2 if/when that holds us back.

This is above "my pay grade". @yarda ?

We will drop python 2 in the future, but currently we support RHEL-7, thus we shouldn't break it. Once RHEL-7 will go EOL we could probably drop it.

I though about it yesterday and of course I missed obvious solution so I changed it.

yarda · 2022-11-28T14:01:32Z

tuned/daemon/daemon.py

@@ -218,6 +218,7 @@ def _thread_code(self):
 						self._unit_manager.update_monitors()
 						log.debug("performing tunings")
 						self._unit_manager.update_tuning()
+				exports.period_check()


I think there is still a room for improvement. You could replace the self._cmd.wait() above by epoll with timeout and sync the time with timer. I.e. if the epoll exits after 1s, then it will be the same. If the epoll exits before 1s, then process the sockets. When the sockets are processed check the timer and run epoll with the lower timeout set.

jmencak · 2022-12-06T17:46:19Z

tuned/exports/unix_socket_exporter.py

+				if type(data) not in (tuple, list) or len(data) < 1:
+					log.error("Wrong format of call")
+					self._send_data(self._socket_object, address, "err", "Wrong format of call")
+				elif data[0] not in self._unix_socket_methods:


If I understand this correctly, we do not support multiple methods in one JSON. E.g. a single API call to both instance_remove_device and instance_add_device. I wonder if we need to improve this and also document the structure of the JSON to send API requests. Without studying the code it is currently not very clear to me what to send.

Multiple methods are not really needed at long as there is a single method that does this :) But you are right that we will typically do a hotunplug / hotplug together (moving a CPU from one section to another).

Coming back to this subject as I believe API is probably the most important to get right. Code can be fixed afterwards, API not so easily...

I suggest the following (yaml is easier to read, please convert to json on your own):

version: 0.0.1 methods: - args: - arg1 - arg2 - arg3 command: method1 - args: - arg1 - arg2 - arg3 command: method2

or even simpler

version: 0.0.1 methods: - command: - name - arg1 - arg2 - command: - name - arg1 - arg2

We can even drop version and add it later if/when needed, but as the json is structured now, it will be impossible to change/enhance in the future. I'm not even suggesting to implement more than one method. That can even be done later if it is too difficult to implement (would be nice though), but I belive the json structure has to change to be more flexible. Thoughts? @MarSik , @yarda I'm not good with naming things, feel free to suggest better names and structure.

Please please do not reinvent the wheel. I mentioned it before: jsonrpc supports batches - https://www.jsonrpc.org/specification#batch

@MarSik Do you want to implement similar structure to jsonrpc or is it ok as it's now? I f you have any suggestions, we can discuss it on other "faster" channels like IRC

I suggest the #tuned IRC channel if/when Martin has time. IMO the current implementation is not OK as it does not allow future enhancements easily enough.

jmencak · 2022-12-13T09:10:07Z

Definitely, I added warning log when sending msg fails with reason. Also changed default path and permissions and added ownership option.

@CZerta Have you tried testing with socat? There are basically two native tools we can use in our environment with this. nc and socat. nc seems to work for sending, but I do not believe it is possible to receive data through it at the same time. socat seems a lot more complex tool that might be able to do this, but at the moment I'm only able to do the same thing as with nc -- just send data:

socat -ddd - UNIX-CONNECT:/run/tuned/tuned.sock

Edit: additional info. With HAProxy socket, for example, the interaction is as simple as:

echo show env | socat -ddd - UNIX-CONNECT:/var/lib/haproxy/run/haproxy.sock

jmencak · 2022-12-14T16:05:25Z

Good news, sort of. I was able to make this work with socat:

send() {
  local data="$1"
  local len=${#data}
  local b0=$(printf '%02x' ${len})	# TODO: this is limited to the payload length of 255 B
  local b1=00
  local b2=00
  local b3=00
  local socket=/run/tuned/tuned.sock

  printf "\x$b3\x$b2\x$b1\x$b0" | socat - UNIX-SENDTO:$socket,bind=/tmp/client.sock
  printf "$data" | socat - UNIX-SENDTO:$socket,bind=/tmp/client.sock
}

send '["profiles"]'

However, it seems not to always succeed. One example of a failure from TuneD:

2022-12-14 16:55:00,507 ERROR    tuned.exports.unix_socket_exporter: Failed to load data of message: [Errno 11] Resource temporarily unavailable

This needs to be 100% reliable and working with netcat or socat. I wonder if we should switch from SOCK_DGRAM to SOCK_STREAM as HAProxy does, for example, for their stats socket.

/cc @MarSik

MarSik · 2022-12-16T09:57:16Z

tuned/exports/unix_socket_exporter.py

+		try:
+			j = json.dumps(data).encode("utf-8")
+			s.sendto(struct.pack("!sL", type.encode("utf-8"), len(j)), address)
+			s.sendto(j, address)


Why two datagrams here? JSON does not need extra length, moreover recv is reading just one datagram per call and the buff size is the maximum received, extra data will be dropped (as this is datagram not stream).

So you really only need the json one.

See here for an example: https://gist.github.com/MarSik/e68acb69611d23939c835bc2dd6aa85d

tuned/utils/global_config.py

jmencak · 2023-01-17T09:15:38Z

Thank you for the latest commit, Honzo! This is great progress not having to use the payload length and just:

printf '["active_profile"]' | nc -U /run/tuned/tuned.sock  # or
printf '["active_profile"]' | socat - UNIX-CONNECT:/run/tuned/tuned.sock

The default socat call often fails to output any result (with WARNING tuned.exports.unix_socket_exporter: Failed send data '<profile-name>' type 'res': [Errno 32] Broken pipe in TuneD logs). This is due to client disconnects, where nc seems to wait (longer?) for a response, whereas the default 0.5s timeout for socat is just not sufficient for this server-side implementation.

printf '["active_profile"]' | socat -t5 - UNIX-CONNECT:/run/tuned/tuned.sock

seems to work every time.

Testing against HAProxy implementation I had no such issues. So at this point I'm wondering whether:

it is worth/possible/practical improving the server-side (TuneD) implementation to improve the client (user) experience
we should just document this

I think I'm probably fine with the latter. @MarSik , thoughts?

MarSik · 2023-01-17T16:37:46Z

tuned/exports/unix_socket_exporter.py

+	def _send_data(self, s, type, data):
+		"""
+		Sends data via socket s.
+		First byte of message specifies type of message (currently "r" for response, "e" for error message and "s" for signal).


This will make it complicated to write parsers.. why not store the type to the structured json payload?

Btw, the code does not match the documentation. You send "err" and "res" here https://github.com/redhat-performance/tuned/pull/470/files#diff-1b8f90567f86039b1dfdcc66a8857985ba22cf3b129e2a8938c8805c6441bf71R191

If it would be more comfortable for you, it's small change to pack it to something like
{"type": "error", "result": ...}
It'd would be extendable, if we'd need to change it in future, so I'm pro.
For second one. there's type[0] on line 147.

MarSik · 2023-01-17T16:40:28Z

tuned/exports/unix_socket_exporter.py

+			raise Exception("Signal '%s' doesn't exist." % signal)
+		for p in self._socket_signal_paths:
+			try:
+				s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)


Why the change to STREAM? STREAM would have been perfect for a bidirectional channel, but you still use two separate unix sockets here.

Oh, you did, just not for signals it seems.

MarSik · 2023-01-17T16:49:38Z

tuned/exports/unix_socket_exporter.py

+				data = ""
+				while True:
+					rec_data = conn.recv(4096).decode()
+					if not rec_data:


This requires new connection for each separate command right? I think we will easily fill up the 16 waiting slots here even for a trivial workload.

Yeah, unfortunately, this doesn't scale. Even this fails for some of the requests:

send() { local data="$1" printf "$data" | nc -U /run/tuned/tuned.sock } for i in $(seq 1 10) do send '["active_profile"]' & done

Can add this const to config so you can set how many connections to listen to. Can add to hold connection too, but I don't think it would fix this scenario

Can add this const to config so you can set how many connections to listen to. Can add to hold connection too, but I don't think it would fix this scenario

I'm not sure this the only scalability problem we have. I bumped it significantly and still getting the same issues. In order for this PR to be useful, IMO we need to be able to serve at least 250 requests coming within roughly a second. At this point we cannot even do 20.

jmencak · 2023-01-18T10:07:14Z

Reporting a traceback during testing:

Traceback (most recent call last):
  File "/usr/sbin/tuned", line 98, in <module>
    app.run(args.daemon)
  File "/usr/lib/python3.6/site-packages/tuned/daemon/application.py", line 214, in run
    result = self._controller.run()
  File "/usr/lib/python3.6/site-packages/tuned/daemon/controller.py", line 65, in run
    exports.period_check()
  File "/usr/lib/python3.6/site-packages/tuned/exports/__init__.py", line 46, in period_check
    return ctl.period_check()
  File "/usr/lib/python3.6/site-packages/tuned/exports/controller.py", line 54, in period_check
    exporter.period_check()
  File "/usr/lib/python3.6/site-packages/tuned/exports/unix_socket_exporter.py", line 162, in period_check
    r, _, _ = select.select([self._socket_object], (), (), 0)
TypeError: argument must be an int, or have a fileno() method.

So far I've seen it only once

jmencak · 2023-01-18T11:21:55Z

tuned/exports/unix_socket_exporter.py

+		try:
+			s.send(type[0].encode("utf-8") + json.dumps(data).encode("utf-8"))
+		except Exception as e:
+			log.warning("Failed send data '%s' type '%s': %s" % (data, type, e))


Nit: I believe the correct English would be "Failed to send data"

CZerta · 2023-01-18T16:48:20Z

Thank you for the latest commit, Honzo! This is great progress not having to use the payload length and just:
printf '["active_profile"]' | nc -U /run/tuned/tuned.sock  # or
printf '["active_profile"]' | socat - UNIX-CONNECT:/run/tuned/tuned.sock
The default socat call often fails to output any result (with WARNING tuned.exports.unix_socket_exporter: Failed send data '<profile-name>' type 'res': [Errno 32] Broken pipe in TuneD logs). This is due to client disconnects, where nc seems to wait (longer?) for a response, whereas the default 0.5s timeout for socat is just not sufficient for this server-side implementation.
printf '["active_profile"]' | socat -t5 - UNIX-CONNECT:/run/tuned/tuned.sock
seems to work every time.

Testing against HAProxy implementation I had no such issues. So at this point I'm wondering whether:
* it is worth/possible/practical improving the server-side (TuneD) implementation to improve the client (user) experience

* we should just document this
I think I'm probably fine with the latter. @MarSik , thoughts?

Can add different logging, but I don't think socket allow to check if other side is still connected.

CZerta · 2023-01-18T17:01:19Z

Reporting a traceback during testing:

Traceback (most recent call last):
  File "/usr/sbin/tuned", line 98, in <module>
    app.run(args.daemon)
  File "/usr/lib/python3.6/site-packages/tuned/daemon/application.py", line 214, in run
    result = self._controller.run()
  File "/usr/lib/python3.6/site-packages/tuned/daemon/controller.py", line 65, in run
    exports.period_check()
  File "/usr/lib/python3.6/site-packages/tuned/exports/__init__.py", line 46, in period_check
    return ctl.period_check()
  File "/usr/lib/python3.6/site-packages/tuned/exports/controller.py", line 54, in period_check
    exporter.period_check()
  File "/usr/lib/python3.6/site-packages/tuned/exports/unix_socket_exporter.py", line 162, in period_check
    r, _, _ = select.select([self._socket_object], (), (), 0)
TypeError: argument must be an int, or have a fileno() method.

So far I've seen it only once

Could you replicate it or dis it happen again? I couldn't replicate it

jmencak · 2023-01-19T18:27:36Z

So the good news is that after the latest commit I can no longer see the trace above and that the requests are being served reliably. However, one per second. Is this expected?

CZerta · 2023-01-20T09:01:54Z

So the good news is that after the latest commit I can no longer see the trace above and that the requests are being served reliably. However, one per second. Is this expected?

You're right, we can do better.

jmencak · 2023-01-20T09:35:40Z

... we can do better.

To give you some sense of what is expected. On a 112 CPU r750, with the default 50 QPS I created 100 pods in parallel. They were all PodScheduled within 1s delta and the average "startup" time between PodScheduled and Ready was 5.19s . I may try tweaking the QPS, but I doubt it will have much significance in reducing the startup times. I used CPUManager static policy and all containers were assigned to its own CPU.

tuned/exports/unix_socket_exporter.py

jmencak

Thank you for the changes, Honzo! I was able to test them with success, however, it needs a bit more work:

printf "DoS" | nc --send-only -U /run/tuned/tuned.sock

Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 220, in period_check
    data = json.loads(data)
  File "/usr/lib64/python3.10/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/usr/lib64/python3.10/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib64/python3.10/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 140, in _send_data
    s.send(json.dumps(data).encode("utf-8"))
BrokenPipeError: [Errno 32] Broken pipe

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/sbin/tuned", line 98, in <module>
    app.run(args.daemon)
  File "/usr/lib/python3.10/site-packages/tuned/daemon/application.py", line 215, in run
    result = self._controller.run()
  File "/usr/lib/python3.10/site-packages/tuned/daemon/controller.py", line 65, in run
    exports.period_check()
  File "/usr/lib/python3.10/site-packages/tuned/exports/__init__.py", line 46, in period_check
    return ctl.period_check()
  File "/usr/lib/python3.10/site-packages/tuned/exports/controller.py", line 54, in period_check
    exporter.period_check()
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 223, in period_check
    self._send_data(conn, self._create_error_responce(-32700, "Parse error", str(e)))
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 142, in _send_data
    log.warning("Failed to send data '%s' type '%s': %s" % (data, e))
TypeError: not enough arguments for format string

jmencak · 2023-01-27T16:00:13Z

tuned/exports/unix_socket_exporter.py

+
+		Example calls:
+
+		printf '[{"jsonrpc": "2.0", "method": "active_profile", "id": 1}, 1]' | sudo nc -U /run/tuned/tuned.sock


I believe we can provide a better example combining the calls. This one returns "Invalid request" and might confuse people.

Perhaps this one?

'[{"jsonrpc": "2.0", "method": "active_profile", "id": 1}, {"jsonrpc": "2.0", "method": "profiles", "id": 2}]'

I wanted to provide example when call fails, but you're right it'd be unnecessarily confusing

CZerta · 2023-02-01T09:24:54Z

Thank you for the changes, Honzo! I was able to test them with success, however, it needs a bit more work:

printf "DoS" | nc --send-only -U /run/tuned/tuned.sock

Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 220, in period_check
    data = json.loads(data)
  File "/usr/lib64/python3.10/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/usr/lib64/python3.10/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib64/python3.10/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 140, in _send_data
    s.send(json.dumps(data).encode("utf-8"))
BrokenPipeError: [Errno 32] Broken pipe

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/sbin/tuned", line 98, in <module>
    app.run(args.daemon)
  File "/usr/lib/python3.10/site-packages/tuned/daemon/application.py", line 215, in run
    result = self._controller.run()
  File "/usr/lib/python3.10/site-packages/tuned/daemon/controller.py", line 65, in run
    exports.period_check()
  File "/usr/lib/python3.10/site-packages/tuned/exports/__init__.py", line 46, in period_check
    return ctl.period_check()
  File "/usr/lib/python3.10/site-packages/tuned/exports/controller.py", line 54, in period_check
    exporter.period_check()
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 223, in period_check
    self._send_data(conn, self._create_error_responce(-32700, "Parse error", str(e)))
  File "/usr/lib/python3.10/site-packages/tuned/exports/unix_socket_exporter.py", line 142, in _send_data
    log.warning("Failed to send data '%s' type '%s': %s" % (data, e))
TypeError: not enough arguments for format string

Good catch, should be corrected

jmencak · 2023-02-01T13:26:29Z

tuned-main.conf

+
+# Size of connections backlog for listen function on socket
+# Higher value allows to process requests from more clients
+# connections_backlog = 16


After reading on and playing with this I suggest to bump this to 1024. Even 128 is just way too low given how infrequently new connections are processed by TuneD. I'd also advise to add a note about the fact the actual value is capped at /proc/sys/net/core/somaxconn. See the manual page for listen().

yarda · 2023-02-07T23:40:34Z

tuned-main.conf

+# enable_dbus = 1
+
+# Enable TuneD listening on unix domain socket
+# enable_unix_socket = 1


Please change to disabled by default.

TuneD is listening on paths specified in config in option unix_socket_paths and send signals to paths in option unix_socket_signal_paths. Example call: printf '[{"jsonrpc": "2.0", "method": "active_profile", "id": 1}, 1]' | sudo nc -U /run/tuned/tuned.sock printf '{"jsonrpc": "2.0", "method": "switch_profile", "params": {"profile_name": "balanced"}, "id": 1}' | sudo nc -U /run/tuned/tuned.sock This PR also introduce possibility to disable dbus API in main TuneD config. Resolves: rhbz#2113900 Signed-off-by: Jan Zerdik <jzerdik@redhat.com>

CZerta requested a review from yarda October 26, 2022 14:32

CZerta force-pushed the rhbz2113900_unix_socket_api branch from df25b25 to 9e71402 Compare October 26, 2022 14:33

CZerta force-pushed the rhbz2113900_unix_socket_api branch from 9e71402 to 2f61fe5 Compare October 26, 2022 14:48

yarda requested changes Nov 8, 2022

View reviewed changes

yarda reviewed Nov 8, 2022

View reviewed changes

tuned/consts.py Outdated

# default listening on dbus

CFG_DEF_DBUS = True

CFG_FUNC_DBUS = "getboolean"

Copy link

Contributor

yarda Nov 8, 2022

Choose a reason for hiding this comment

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

What is it used for?

CZerta force-pushed the rhbz2113900_unix_socket_api branch from 2f61fe5 to 1fa7493 Compare November 21, 2022 09:24

CZerta force-pushed the rhbz2113900_unix_socket_api branch from 1fa7493 to b89aee6 Compare November 21, 2022 09:53

jmencak reviewed Nov 21, 2022

View reviewed changes

CZerta force-pushed the rhbz2113900_unix_socket_api branch 2 times, most recently from 43c86ed to f4edbaf Compare November 21, 2022 14:37

jmencak reviewed Nov 22, 2022

View reviewed changes

CZerta force-pushed the rhbz2113900_unix_socket_api branch from f4edbaf to 07500f8 Compare November 24, 2022 10:46

jmencak reviewed Nov 24, 2022

View reviewed changes

CZerta force-pushed the rhbz2113900_unix_socket_api branch from 07500f8 to 897afec Compare November 25, 2022 10:30

yarda reviewed Nov 28, 2022

View reviewed changes

jmencak reviewed Dec 6, 2022

View reviewed changes

MarSik reviewed Dec 16, 2022

View reviewed changes

github-advanced-security bot found potential problems Jan 16, 2023

View reviewed changes

tuned/utils/global_config.py Fixed Show fixed Hide fixed

MarSik reviewed Jan 17, 2023

View reviewed changes

jmencak reviewed Jan 18, 2023

View reviewed changes

CZerta force-pushed the rhbz2113900_unix_socket_api branch from 10e6b6d to 96ebeb7 Compare January 19, 2023 16:54

CZerta force-pushed the rhbz2113900_unix_socket_api branch from 96ebeb7 to 3717c5f Compare January 20, 2023 09:01

CZerta force-pushed the rhbz2113900_unix_socket_api branch from 3717c5f to bdf1a7d Compare January 26, 2023 15:56

github-advanced-security bot found potential problems Jan 26, 2023

View reviewed changes

tuned/exports/unix_socket_exporter.py Fixed Show fixed Hide fixed

jmencak reviewed Jan 27, 2023

View reviewed changes

CZerta force-pushed the rhbz2113900_unix_socket_api branch 2 times, most recently from 107d059 to e3ab29f Compare February 1, 2023 09:22

jmencak reviewed Feb 1, 2023

View reviewed changes

CZerta force-pushed the rhbz2113900_unix_socket_api branch from e3ab29f to 4265813 Compare February 1, 2023 14:13

yarda reviewed Feb 7, 2023

View reviewed changes

CZerta force-pushed the rhbz2113900_unix_socket_api branch from 4265813 to 7f60a89 Compare February 8, 2023 10:04

yarda self-requested a review February 8, 2023 10:40

CZerta force-pushed the rhbz2113900_unix_socket_api branch from 7f60a89 to 3e3f467 Compare February 8, 2023 11:22

Merge branch 'master' into rhbz2113900_unix_socket_api

6d08f31

yarda approved these changes Feb 8, 2023

View reviewed changes

yarda merged commit d45a4db into redhat-performance:master Feb 8, 2023


		Example calls:

		printf '[{"jsonrpc": "2.0", "method": "active_profile", "id": 1}, 1]' \| sudo nc -U /run/tuned/tuned.sock

Expose TuneD API to the Unix Domain Socket. #470

Expose TuneD API to the Unix Domain Socket. #470

Conversation

CZerta commented Oct 26, 2022

lgtm-com bot commented Oct 26, 2022

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 Nov 8, 2022 • 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

lgtm-com bot commented Nov 21, 2022

jmencak 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

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

jmencak Nov 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmencak commented Nov 22, 2022

CZerta commented Nov 22, 2022 • edited

jmencak commented Nov 22, 2022

jmencak commented Nov 22, 2022

CZerta commented Nov 22, 2022

jmencak commented Nov 24, 2022 • edited

CZerta commented Nov 24, 2022

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

jmencak commented Dec 13, 2022 • edited

jmencak commented Dec 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmencak commented Jan 17, 2023

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

jmencak commented Jan 18, 2023

jmencak Jan 18, 2023 • edited

Choose a reason for hiding this comment

CZerta commented Jan 18, 2023

CZerta commented Jan 18, 2023

jmencak commented Jan 19, 2023

CZerta commented Jan 20, 2023

jmencak commented Jan 20, 2023

jmencak left a comment

yarda Nov 8, 2022 •

edited

jmencak Nov 22, 2022 •

edited

CZerta commented Nov 22, 2022 •

edited

jmencak commented Nov 24, 2022 •

edited

jmencak commented Dec 13, 2022 •

edited

jmencak commented Dec 14, 2022 •

edited

jmencak Jan 18, 2023 •

edited

jmencak Feb 1, 2023 •

edited