nsqd: improve log when data-path does not exist

[mdh67899]

#1217

[jehiah]

@mdh67899 what is the specific error you are getting? If you can share that it might help to better understand how to structure this error to handle the normal case (Another nsqd is running) and the case it sounds like you encountered (filesystem support)

[mdh67899]

@jehiah If the directory specified by --data-path does not exist, then nsqd startup will fail when trying to execute n.dl.Lock ().

In addition, if there are other nsqd processes locking the --data-path , an error will also be reported

nsq/internal/dirlock/dirlock.go

Lines 22 to 33 in 5b67f58

	func (l *DirLock) Lock() error {
	f, err := os.Open(l.dir)
	if err != nil {
	return err
	}
	l.f = f
	err = syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB)
	if err != nil {
	return fmt.Errorf("cannot flock directory %s - %s", l.dir, err)
	}
	return nil
	}

If the n.dl.Lock () operation fails, the error should be printed in the log for troubleshooting.
or do you think it is a good idea to create data path when --data-path is not exist?

[mdh67899]

@jehiah in issue #1217, it's not a filesystem support problem, the reason is the --data-path=/data/nsq is no exist in docker container

[ploxiln]

I think it is a good idea to add the underlying error text here. I think it could be worded in a simpler way, maybe just "failed to lock data-path: %v", and we may not need to include the path explicitly, nor the "(possibly by another instance ..." part . The main error possibilities are (according to my testing just now with this branch and go-1.13 on macOS):

• "cannot flock directory ./tmp - resource temporarily unavailable"
• "open ./tmp2: no such file or directory"
• "open ./tmp2: permission denied"

We could add the "possibly by another instance" explanation just for the flock error ...

[jehiah]

thanks for the context @ploxiln. I think it would be good to keep explicit guidance in the flock error case that nsqd doesn't support multiple instances running w/ the same data directory.

[mdh67899]

@jehiah @ploxiln Do you think it is appropriate to adjust the code this way?

nsq/internal/dirlock/dirlock.go

 func (l *DirLock) Lock() error { 
 	f, err := os.Open(l.dir) 
 	if err != nil { 
 		return fmt.Errorf("cannot open directory %s - %s", l.dir, err) 
 	} 
 	l.f = f 
 	err = syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) 
 	if err != nil { 
 		return fmt.Errorf("cannot flock directory %s - %s (possibly by another instance of nsqd)", l.dir, err)
  	} 
 	return nil 
 } 

nsqd/nsqd.go

	err = n.dl.Lock()
	if err != nil {
		return nil, fmt.Errorf("failed to lock data-path: %v", err)
	}

[ploxiln]

Yeah, adding the "possibly by another instance" to the Flock error return works for me. With the other changes, I would rephrase it "(possibly in use by another instance of nsqd)" here.

I don't think the Open error needs any additions, the text seems to already include "open {dir}".

I tested briefly in a linux VM and saw the same error text from Open and Flock.

[mdh67899]

@ploxiln PTAL

[ploxiln]

looks good to me