On Mon, Jul 11, 2022, 9:46 PM Peter Delevoryas <[email protected]> wrote:
> On Mon, Jul 11, 2022 at 04:56:28PM -0400, John Snow wrote: > > On Thu, Jul 7, 2022 at 2:46 PM Peter Delevoryas <[email protected]> wrote: > > > > > > On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote: > > > > On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote: > > > > > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote: > > > > > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote: > > > > > > > I noticed that I can't run any avocado tests on macOS because > the QMP > > > > > > > unix socket path is too long: > > > > > > > > > > > > > > > > > > > I think the path limit for unix sockets on macOS might be 104 > [1] > > > > > > > > > > > > All platforms have a very limited path limit, so it isn't really > > > > > > a macOS specific problem, rather.... > > > > > > > > > > > > > > > > > > > > /* > > > > > > > * [XSI] Definitions for UNIX IPC domain. > > > > > > > */ > > > > > > > struct sockaddr_un { > > > > > > > unsigned char sun_len; /* sockaddr len including > null */ > > > > > > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > > > > > > char sun_path[104]; /* [XSI] path name (gag) */ > > > > > > > }; > > > > > > > > > > > > > > The path we're using is exactly 105 characters: > > > > > > > > > > > > > > $ python > > > > > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01) > > > > > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on > darwin > > > > > > > Type "help", "copyright", "credits" or "license" for more > information. > > > > > > > >>> > len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock') > > > > > > > > > > > > It is a problem related to where the test suite is creating the > > > > > > paths. > > > > > > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/ > > > > > > > > > > > > is way too deep a directory location. > > > > > > > > > > That's a good point. > > > > > > > > > > > > > > > > > It seems we just create this location using > 'tempfile.TemporyDirectory' > > > > > > to get a standard tmp dir. > > > > > > > > > > > > Do you know why python is choosing > > > > > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > > > > > > > as the temp dir ? Is that a standard location on macOS or is it > > > > > > from some env variable you have set ? > > > > > > > > > > Hmmm yeah it is odd, I'm not really sure why it's created there or > if > > > > > standard glibc tmpfile creation goes there too, I'll go experiment > and > > > > > report back. And yeah, maybe I'll double check any environment > variables or > > > > > other things. > > > > > > > > > > The macOS system I use happens to be a Facebook work laptop, which > could > > > > > also be related now that I think about it. > > > > > > > > Hmmm yeah looks like this is because my TMPDIR is weird. > > > > > > > > $ echo $TMPDIR > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > > > I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷 > > > > > > > > Thanks for responding, I'll just use TMPDIR=/tmp for now. It's > probably > > > > something wrong with the Facebook development environment. > > > > > > > > Peter > > > > > > Update: Actually, this might not be a Facebook-work-laptop specific > > > thing. I asked my non-engineer friend to print out $TMPDIR on his > > > macbook and he got the same thing. > > > > > > > https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > > > > > > I guess this person suggests it's just to separate the permissions for > > > each user's /tmp directory, for better isolation. > > > > > > I'll resubmit this patch with the suggestions you had, because perhaps > > > this is actually affecting other macOS users too. > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/python/qemu/machine/machine.py > b/python/qemu/machine/machine.py > > > > > > > index 37191f433b..93451774e3 100644 > > > > > > > --- a/python/qemu/machine/machine.py > > > > > > > +++ b/python/qemu/machine/machine.py > > > > > > > @@ -157,7 +157,7 @@ def __init__(self, > > > > > > > self._wrapper = wrapper > > > > > > > self._qmp_timer = qmp_timer > > > > > > > > > > > > > > - self._name = name or > f"qemu-{os.getpid()}-{id(self):02x}" > > > > > > > + self._name = name or f"{os.getpid()}{id(self):02x}" > > > > > > > > > > > > I don't think this is the right fix really, because IMHO the > problem > > > > > > is the hugely long path, rather than the final socket name. > > > > > > > > > > True, yeah let me try to investigate the directory placement. > > > > > > > > > > > > > > > > > That said, there is redundancy in the path - avocado is passing > in > > > > > > a dierctory created using 'tempfile.TemporyDirectory' so there > is no > > > > > > reason why we need to add more entropy via the POD and the > 'id(self)' > > > > > > hex string. > > > > > > > > > > Oh good point, I hadn't thought about that. > > > > > > > > > > > > > > > > > IMHO avocado should pass in the 'name' parameter explicitly, > using a > > > > > > plain name and thus get a shorter string. > > > > > > > > > > I see, yeah that makes sense. Maybe I can include a couple patches > for this, > > > > > one fixing the directory location, and one refactoring the > temporary file > > > > > name template (If I'm understanding your suggestion correctly). > > > > > > > > > > Thanks for the review! I really appreciate it. > > > > > Peter > > > > I agree with Dan: I believe the correct solution here is for Avocado > > to provide its own less redundant name; but the default name that > > machine.py provides is not *that* long and provides adequate > > protection against collisions with multiple instances of the VM > > utility within a single python process. If Avocado is creating its own > > directories that guard against that redundancy, Avocado should provide > > a shortened name for the VM. > > Hmmm, I see. > > > > > Note that the QEMUMachine process also provides a sock_dir parameter > > that was introduced for precisely this reason; it should be possible > > to instruct the avocado tests to use a shorter path for sock_dir. > > > > I'm not clear on what the best "just works" solution will be when > > certain operating environments choose a tmp dir that's quite long to > > begin with; maybe we need a different default sockfile naming strategy > > that avoids the instance collision problem in machine.py, too. Ideas? > > I guess I don't really understand why QEMU is attempting to create a > unique path instead of just using mktemp(), right? This seems like > something that the OS can provide. If we want some semblance of meaning > to the filename, we can provide a short template, right? > Yeah, should be good. The "name" of the VM should be left unique for logging purposes, so the name should remain as-is, IMO. However, if you want to change how we create the sockfile by using a new, temporary name (and not the vm name), that works for me as far as I can remember. I think there was some desire to have the temp file obviously correlate to the instance, but that's not a hard requirement, so feel free to change that part. Just make sure that come hell or high water the sockfile is cleaned up on exit. > -- Peter > > > > > > --js > > > > > > > > > > > > > > > > > > > > self._temp_dir: Optional[str] = None > > > > > > > self._base_temp_dir = base_temp_dir > > > > > > > self._sock_dir = sock_dir > > > > > > > @@ -167,7 +167,7 @@ def __init__(self, > > > > > > > self._monitor_address = monitor_address > > > > > > > else: > > > > > > > self._monitor_address = os.path.join( > > > > > > > - self.sock_dir, f"{self._name}-monitor.sock" > > > > > > > + self.sock_dir, f"{self._name}.sock" > > > > > > > ) > > > > > > > > > > > > > > self._console_log_path = console_log > > > > > > > -- > > > > > > > 2.37.0 > > > > > > > > > > > > > > > > > > > > > > > > > > With regards, > > > > > > Daniel > > > > > > -- > > > > > > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > > > > > > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > > > > > > |: https://entangle-photo.org -o- > https://www.instagram.com/dberrange :| > > > > > > > > > > > > > > > > > > > > > >
