Fix issues in the code review

- Set the default values of Origin/Cache_SelfTestInterval
- Move self-monitoring.go to xrootd pkg, avoid creating a new pkg
- Use filepath.Join to construct the path
- Other minor improvements

Author: h2zh

config/config.go

@@ -1042,6 +1042,8 @@ func SetServerDefaults(v *viper.Viper) error {
 	v.SetDefault(param.Registry_RequireKeyChaining.GetName(), true)
 	v.SetDefault(param.Origin_StorageType.GetName(), "posix")
 	v.SetDefault(param.Origin_SelfTest.GetName(), true)
+	v.SetDefault(param.Origin_SelfTestInterval.GetName(), 15*time.Second)
+	v.SetDefault(param.Cache_SelfTestInterval.GetName(), 15*time.Second)
 	v.SetDefault(param.Origin_DirectorTest.GetName(), true)
 	// Set up the default S3 URL style to be path-style here as opposed to in the defaults.yaml because
 	// we want to be able to check if this is user-provided (which we can't do for defaults.yaml)

launchers/cache_serve.go

@@ -40,7 +40,6 @@ import (
 	"github.com/pelicanplatform/pelican/lotman"
 	"github.com/pelicanplatform/pelican/metrics"
 	"github.com/pelicanplatform/pelican/param"
-	"github.com/pelicanplatform/pelican/self_monitor"
 	"github.com/pelicanplatform/pelican/server_structs"
 	"github.com/pelicanplatform/pelican/server_utils"
 	"github.com/pelicanplatform/pelican/web_ui"
@@ -109,12 +108,12 @@ func CacheServe(ctx context.Context, engine *gin.Engine, egrp *errgroup.Group, m
 	cache.LaunchFedTokManager(ctx, egrp, cacheServer)
 
 	if param.Cache_SelfTest.GetBool() {
-		err = self_monitor.InitSelfTestDir()
+		err = xrootd.InitSelfTestDir()
 		if err != nil {
 			return nil, err
 		}
 
-		self_monitor.PeriodicSelfTest(ctx, egrp, false)
+		xrootd.PeriodicSelfTest(ctx, egrp, false)
 	}
 
 	// Director and origin also registers this metadata URL; avoid registering twice.

launchers/origin_serve.go

@@ -39,7 +39,6 @@ import (
 	"github.com/pelicanplatform/pelican/oa4mp"
 	"github.com/pelicanplatform/pelican/origin"
 	"github.com/pelicanplatform/pelican/param"
-	"github.com/pelicanplatform/pelican/self_monitor"
 	"github.com/pelicanplatform/pelican/server_structs"
 	"github.com/pelicanplatform/pelican/server_utils"
 	"github.com/pelicanplatform/pelican/web_ui"
@@ -110,7 +109,7 @@ func OriginServe(ctx context.Context, engine *gin.Engine, egrp *errgroup.Group,
 	}
 
 	if param.Origin_SelfTest.GetBool() {
-		self_monitor.PeriodicSelfTest(ctx, egrp, true)
+		xrootd.PeriodicSelfTest(ctx, egrp, true)
 	}
 
 	privileged := param.Origin_Multiuser.GetBool()

xrootd/launch.go

@@ -39,6 +39,7 @@ import (
 	"github.com/pelicanplatform/pelican/config"
 	"github.com/pelicanplatform/pelican/daemon"
 	"github.com/pelicanplatform/pelican/param"
+	"github.com/pelicanplatform/pelican/server_utils"
 )
 
 type (
@@ -159,9 +160,9 @@ func makeUnprivilegedXrootdLauncher(daemonName string, configPath string, isCach
 		result.ExtraEnv = append(result.ExtraEnv, "XRDHTTP_PELICAN_CERT_FILE="+filepath.Join(xrootdRun, "copied-tls-creds.crt"))
 		result.ExtraEnv = append(result.ExtraEnv, "XRDHTTP_PELICAN_INFO_FD="+strconv.Itoa(result.fds[1]))
 
-		basePath := param.Cache_NamespaceLocation.GetString() + "/pelican/monitoring/selfTest"
-		testFileLocation := basePath + "/self-test-cache-server.txt"
-		testFileCinfoLocation := basePath + "/self-test-cache-server.txt.cinfo"
+		basePath := filepath.Join(param.Cache_NamespaceLocation.GetString(), server_utils.MonitoringBaseNs, "selfTest")
+		testFileLocation := filepath.Join(basePath, "self-test-cache-server.txt")
+		testFileCinfoLocation := filepath.Join(basePath, "self-test-cache-server.txt.cinfo")
 		result.ExtraEnv = append(result.ExtraEnv, "XRDHTTP_PELICAN_CACHE_SELF_TEST_FILE="+testFileLocation)
 		result.ExtraEnv = append(result.ExtraEnv, "XRDHTTP_PELICAN_CACHE_SELF_TEST_FILE_CINFO="+testFileCinfoLocation)
 	}

self_monitor/self_monitor.go xrootd/self_monitor.goself_monitor/self_monitor.go renamed to xrootd/self_monitor.go

@@ -16,7 +16,7 @@
  *
  ***************************************************************/
 
-package self_monitor
+package xrootd
 
 import (
 	"context"
@@ -41,7 +41,6 @@ import (
 	"github.com/pelicanplatform/pelican/server_utils"
 	"github.com/pelicanplatform/pelican/token"
 	"github.com/pelicanplatform/pelican/token_scopes"
-	"github.com/pelicanplatform/pelican/xrootd"
 )
 
 const (
@@ -179,7 +178,7 @@ func generateTestFileViaPlugin() (string, error) {
 	selfTestBirthplace := filepath.Join(param.Monitoring_DataLocation.GetString(), "selfTest")
 	err = config.MkdirAll(selfTestBirthplace, 0750, user.Uid, user.Gid)
 	if err != nil {
-		return "", errors.Wrap(err, "failed to create selftest directory")
+		return "", errors.Wrap(err, "failed to create self-test directory")
 	}
 
 	// Create a test file and its cinfo in the birthplace
@@ -234,11 +233,15 @@ func generateTestFileViaPlugin() (string, error) {
 		return "", errors.Wrap(err, "failed to seek to beginning of cinfo file")
 	}
 
-	// Transplant the test file and cinfo from birthplace to the selfTestDir via xrdhttp-pelican plugin
-	if err = xrootd.SelfTestFileCopy(4, file); err != nil {
+	// Transplant the test file to selfTestDir using the xrdhttp-pelican plugin.
+	// Command "4" instructs the plugin to put the test file into the designated location, which is specified in `xrootd/launch.go`.
+	// Check `src/XrdHttpPelican.cc` in https://github.com/PelicanPlatform/xrdhttp-pelican for the counterpart in the plugin.
+	if err = SelfTestFileCopy(4, file); err != nil {
 		return "", errors.Wrap(err, "failed to copy the test file to the self-test directory")
 	}
-	if err = xrootd.SelfTestFileCopy(5, cinfoFile); err != nil {
+	// Transplant the cinfo file to selfTestDir using the xrdhttp-pelican plugin.
+	// Command "5" instructs the plugin to put the cinfo file into the designated location, which is specified in `xrootd/launch.go`.
+	if err = SelfTestFileCopy(5, cinfoFile); err != nil {
 		return "", errors.Wrap(err, "failed to copy the test cinfo file to the self-test directory")
 	}
 
@@ -418,9 +421,14 @@ func PeriodicSelfTest(ctx context.Context, ergp *errgroup.Group, isOrigin bool)
 		customInterval = param.Origin_SelfTestInterval.GetDuration()
 	}
 
-	if customInterval == 0 {
+	// Fallback to a 15s interval, if the user sets a very small or negative interval
+	if customInterval < 1*time.Second {
 		customInterval = 15 * time.Second
-		log.Error("Invalid config value: both Origin.SelfTestInterval and Cache.SelfTestInterval are 0. Fallback to 15s.")
+		serverType := "Cache"
+		if isOrigin {
+			serverType = "Origin"
+		}
+		log.Warningf("Invalid config value: %s.SelfTestInterval. Fallback to 15s.", serverType)
 	}
 	ticker := time.NewTicker(customInterval)
 	firstRound := time.After(5 * time.Second)