[WIP] refactor: replace hardcoded paths with constants from locationconsts.go (#3682) #4965

jeff-zed · 2025-06-12T10:29:34Z

This PR resolves Issue #3682 by replacing hardcoded filesystem paths with centralized constants from locationconsts.go across the pillar modules, improving code maintainability and consistency. The refactoring centralizes path management while preserving existing functionality and maintaining clear separation between EVE-managed paths and system paths.

Currently, 5 modules have been updated pending test verification (see commits for details):

pkg/pillar/types/locationconsts.go
pkg/pillar/agentlog/*
pkg/pillar/base/*
pkg/pillar/hypervisor/*
pkg/pillar/pidfile/*

PR dependencies

How to test and validate this PR

make test pkg/pillar

Changelog notes

This addresses technical debt by centralizing filesystem path management, making future path changes easier to implement and reducing the risk of inconsistent path usage across the codebase.

PR Backports

N/A - This is a refactoring change that improves maintainability without changing functionality.

Checklist

I've provided a proper description
I've added the proper documentation (when applicable)
I've tested my PR on amd64 device(s)
I've tested my PR on arm64 device(s)
I've written the test verification instructions
I've set the proper labels to this PR

jeff-zed · 2025-06-12T11:12:25Z

@eriknordmark this will be a multi-commit PR, module by module.

pkg/pillar/agentlog/agentlog.go

pkg/pillar/agentlog/cmd/psi-collector/README.md

jeff-zed · 2025-06-13T13:01:21Z

@milan-zededa @christoph-zededa

There is a minor opportunity to centralize the path "/run/eve-hv-type"

eve/pkg/pillar/base/kubevirt.go

Lines 15 to 16 in 7ab0c0b

    
           // EveVirtTypeFile contains the virtualization type, ie kvm, xen or kubevirt 
        
           EveVirtTypeFile = "/run/eve-hv-type"

eve/pkg/pillar/hypervisor/hypervisor.go

Line 98 in 7ab0c0b

return bootTimeHypervisorWithHVFilePath("/run/eve-hv-type")

However, pkg/pillar/types/dns.go imports pkg/pillar/base for pkg/pillar/base/logobjecttypes.go

eve/pkg/pillar/types/dns.go

Line 15 in 7ab0c0b

"github.com/lf-edge/eve/pkg/pillar/base"

Because pkg/pillar/base is used in so many modules (not limited to pkg/pillar/base/logobjecttypes.go), I am inclined to leave this as-is.

Alternatively, we could just remove this function and refactor its limited use:

https://github.com/lf-edge/eve/blob/7ab0c0b8b14650228e9ff3508a2606ba889fe8ab/pkg/pillar/hypervisor/hypervisor.go#L96C1-L99C2

milan-zededa · 2025-06-13T13:10:48Z

@jeff-zed I'm not sure if I follow the issue with the pillar/base package.
Are you getting import cycle or some other build issue if you change BootTimeHypervisor to:

func BootTimeHypervisor() Hypervisor {
	return bootTimeHypervisorWithHVFilePath(base.EveVirtTypeFile)
}

?
Or were you considering to move EveVirtTypeFile to pillar/types?

jeff-zed · 2025-06-13T13:12:01Z

@jeff-zed I'm not sure if I follow the issue with the pillar/base package. Are you getting import cycle or some other build issue if you change BootTimeHypervisor to:
func BootTimeHypervisor() Hypervisor {
	return bootTimeHypervisorWithHVFilePath(base.EveVirtTypeFile)
}
? Or were you considering to move EveVirtTypeFile to pillar/types?

cyclical dependency

…go (lf-edge#3682) This will close lf-edge#3682, after each module is refactored - each module will be a separate commit. - pkg/pillar/types/locationconsts.go: • add RunDir, PersistLogDir, PersistTmpDir, HostFSEtcDir constants • add LinuxKitConfigFile, MemoryMonitorPSICollectorDir constants • refactor existing paths to use base constants (RunDir, PersistDir) • fix comment formatting and typos - pkg/pillar/agentlog/agentlog.go: • replace hardcoded /persist paths with PersistDir/PersistLogDir constants • update comments to reference constant names instead of hardcoded paths - pkg/pillar/agentlog/eveversion.go: • replace hardcoded /run/eve.id with types.RunDir constant - pkg/pillar/agentlog/http-debug.go: • replace hardcoded /persist/tmp with types.PersistTmpDir • replace hardcoded /hostfs/etc path with types.HostFSEtcDir • add missing types import - pkg/pillar/agentlog/http-debug_test.go: • replace hardcoded paths with corresponding types constants - pkg/pillar/agentlog/cmd/psi-collector/main.go: • replace hardcoded path with types.MemoryMonitorPSICollectorDir - pkg/pillar/agentlog/cmd/psi-collector/README.md: • update documentation to reflect refactored constants Paths NOT changed: - System/kernel paths (/proc, /sys, /hostfs/sys) - these are fixed system interfaces - Device files (/dev/tty, /dev/null) - standard Unix device files - External software paths (/usr/lib/xen, /usr/local/share) - system-installed software - Test temporary paths - these are ephemeral and not part of EVE's directory structure Closes lf-edge#3682 Signed-off-by: jeff-zed <[email protected]>

…lepath.Join - pkg/pillar/agentlog/agentlog.go: replace concat with filepath.Join - pkg/pillar/agentlog/cmd/psi-collector/README.md: remove extraneous whitespace Signed-off-by: jeff-zed <[email protected]>

OhmSpectator · 2025-06-13T14:31:31Z

The runner deployment script seems to be malfunctioning. @rene is currently working on fixing it.

- pkg/pillar/base/kubevirt.go: annotate why EveVirtTypeFile isn’t moved to types - pkg/pillar/base/touchfile.go: note WatchdogFileDir cyclical-import avoidance - pkg/pillar/hypervisor/hypervisor.go: use types.EveVirtTypeFile & types.SysfsPciDevicesDir - pkg/pillar/hypervisor/kvm.go: switch to types.SysfsPciDevicesDir for PCI resource path - pkg/pillar/hypervisor/pci.go: remove local sysfsPciDevices const, import types - pkg/pillar/pidfile/pidfile.go: replace defaultRundir with types.WatchdogPidDir - pkg/pillar/types/ifnametopci.go: remove pciPath, use SysfsPciDevicesDir + filepath.Join - pkg/pillar/types/locationconsts.go: introduce RunDir, Watchdog*/MemoryMonitor*/SysfsPciDevicesDir, update derived paths Signed-off-by: jeff-zed <[email protected]>

rene · 2025-06-17T07:03:01Z

pkg/pillar/base/touchfile.go


 const (
+	//These contants are available in pkg/pillar/types/locationcnsts.go; however, using them
+	//causes a cyclical dependency.


☝️ Yetus errors to be fixed.

rene · 2025-06-17T07:04:07Z

pkg/pillar/types/locationconsts.go

 	CustomOVMFSettingsDir = "/hostfs/etc/ovmf"

+	// SysfsPciDevices - path in sysfs to the PCI devices
+	SysfsPciDevicesDir = "/sys/bus/pci/devices/"


rene

@jeff-zed apart from the Yetus errors yet to be fixed, it looks good to me...

jeff-zed · 2025-06-17T07:08:50Z

@jeff-zed apart from the Yetus errors yet to be fixed, it looks good to me...

There is still some more to commit. When I think I am done and it is ready for full review, I will add a comment indicating that for the reviewers.

OhmSpectator · 2025-06-17T07:49:38Z

@jeff-zed apart from the Yetus errors yet to be fixed, it looks good to me...

There is still some more to commit. When I think I am done and it is ready for full review, I will add a comment indicating that for the reviewers.

Until a PR is ready for review, we usually mark it as Draft and add [WIP] to the title.

jeff-zed · 2025-06-17T07:53:03Z

@jeff-zed apart from the Yetus errors yet to be fixed, it looks good to me...

There is still some more to commit. When I think I am done and it is ready for full review, I will add a comment indicating that for the reviewers.

Until a PR is ready for review, we usually mark it as Draft and add [WIP] to the title.

Thank you.

- pkg/pillar/base/touchfile.go: updatecomments for WatchdogFileDir and WatchdogPidDir - pkg/pillar/types/locationconsts.go: update comment for SysfsPciDevicesDir Signed-off-by: jeff-zed <[email protected]>

- pkg/pillar/dpcreconciler/genericitems/ssh.go: import types and use types.RunDir - pkg/pillar/ssh/ssh.go: import types and use types.RunDir - pkg/pillar/nireconciler/genericitems/dnsmasq.go: comment for a candidate constant; replace hard-coded "/run/zedrouter" Signed-off-by: jeff-zed <[email protected]>

jeff-zed · 2025-06-17T08:13:06Z

The runner deployment script seems to be malfunctioning. @rene is currently working on fixing it.

Thank you. This should also be helpful for EVE 15.3.0 that @eriknordmark tagged

…utside pillar), cmd/domainmgr, pubsub - pkg/edgeview/src/network.go: import filepath and use Join - pkg/edgeview/src/system.go: use types.EdgeviewPath instead of "/run/edgeview" - pkg/pillar/cmd/domainmgr/domainmgr.go: use types.DomainMgrDirl - pkg/pillar/cmd/domainmgr/processes.go: remove local watch directories and use from pkg\pillar\locationconst.go - pkg/pillar/pubsub/checkmaxtime.go: annotate cyclical-dependency note - pkg/pillar/types/locationconsts.go: add DomainMgrDir = RunDir + "/domainmgr" Signed-off-by: jeff-zed <[email protected]>

jeff-zed · 2025-06-17T10:03:47Z

Hi all. I noticed that pkg-deps.mk is currently ignored via .gitignore, but it needs to be versioned so CI picks up our updated dependency list.

Would you prefer I remove the *.mk (or pkg-deps.mk) entry from .gitignore, or simply force-add this one file?

There is also the question of whether edgeview should be modified to use the pkg/pillar/types/locationconst.go or not in the latest commit.

Thanks!

eriknordmark · 2025-07-02T23:26:22Z

pkg/pillar/agentlog/http-debug.go

 	"bytes"
 	"context"
 	"fmt"
+	"github.com/lf-edge/eve/pkg/pillar/types"


Please put this import below where the other non-std imports are placed.

eriknordmark · 2025-07-02T23:26:58Z

pkg/pillar/agentlog/cmd/psi-collector/main.go

 const (
 	//PIDFile is the file to store the PID
-	PIDFile = types.MemoryMonitorDir + "/psi-collector/psi-collector.pid"
+	PIDFile = types.MemoryMonitorPSICollectorDir + "/psi-collector.pid"


You changed many other places to use filepath.Join but not here. Any particular reason?

I think it does not work for const - #4965 (comment)

eriknordmark · 2025-07-02T23:27:14Z

pkg/pillar/agentlog/eveversion.go


 const (
-	partitionFile = "/run/eve.id"
+	partitionFile = types.RunDir + "/eve.id"


eriknordmark · 2025-07-02T23:29:25Z

pkg/pillar/agentlog/http-debug_test.go

 	}

-	err = os.MkdirAll("/hostfs/etc/", 0755)
+	err = os.MkdirAll(types.HostFSEtcDir, 0755)


This is a test file hence it doesn't run on EVE but in the build environment, hence I'm confused by the references to /hostfs and /persist which only exist in the EVE runtime environment.

eriknordmark · 2025-07-02T23:29:58Z

pkg/pillar/base/kubevirt.go


 const (
 	// EveVirtTypeFile contains the virtualization type, ie kvm, xen or kubevirt
+	//This could be referenced from `pkg\pillar\types\locationconsts.go`, but it would cause


Please replace backslashes by forward slashes.

eriknordmark · 2025-07-02T23:34:26Z

pkg/pillar/pidfile/pidfile.go

 		return fmt.Errorf("checkAndCreatePidfile: %s", description)
 	}
-	rundir := defaultRundir
+	rundir := types.WatchdogPidDir


Incorrect. Should be RunDir

eriknordmark · 2025-07-02T23:34:34Z

pkg/pillar/pidfile/pidfile.go

 	}
 	if opt.baseDir == "" {
-		opt.baseDir = defaultRundir
+		opt.baseDir = types.WatchdogPidDir


Incorrect. Should be RunDir

eriknordmark

Two cases of wrong directory being used which needs to be fixed. See detailed comments.

Also, build is utterly failing due to
14.69 vet: ./network.go:305:50: undefined: types.DomainMgrDir

I think it is better (and sets less of a precendent) to not introduce locationconst values for the various /run/ directories, and it happens to avoid this internal vendoring issue (pkg/pillar being vendored into pkg/edgeview).

Also, any idea why golangcilint has issues on line 1 in pkg/edgeview/src/basics.go?

jeff-zed requested a review from eriknordmark as a code owner June 12, 2025 10:29

christoph-zededa reviewed Jun 12, 2025

View reviewed changes

rene reviewed Jun 12, 2025

View reviewed changes

pkg/pillar/agentlog/cmd/psi-collector/README.md Show resolved Hide resolved

jeff-zed added 2 commits June 13, 2025 15:27

refactor: removing extraneous whitespace and replacing concat with fi…

802ab16

…lepath.Join - pkg/pillar/agentlog/agentlog.go: replace concat with filepath.Join - pkg/pillar/agentlog/cmd/psi-collector/README.md: remove extraneous whitespace Signed-off-by: jeff-zed <[email protected]>

jeff-zed force-pushed the refactor/agentlog-directory-constants-clean branch from 7ab0c0b to 802ab16 Compare June 13, 2025 14:07

jeff-zed requested review from OhmSpectator, rouming and shjala as code owners June 16, 2025 13:07

github-actions bot requested review from rene and rucoder June 16, 2025 13:07

jeff-zed force-pushed the refactor/agentlog-directory-constants-clean branch from 0986749 to 9084371 Compare June 16, 2025 13:09

rene reviewed Jun 17, 2025

View reviewed changes

OhmSpectator marked this pull request as draft June 17, 2025 07:49

OhmSpectator changed the title ~~refactor: replace hardcoded paths with constants from locationconsts.go (#3682)~~ [WIP] refactor: replace hardcoded paths with constants from locationconsts.go (#3682) Jun 17, 2025

refactor: update exported const comments and correct typo in constants

24fe02f

- pkg/pillar/base/touchfile.go: updatecomments for WatchdogFileDir and WatchdogPidDir - pkg/pillar/types/locationconsts.go: update comment for SysfsPciDevicesDir Signed-off-by: jeff-zed <[email protected]>

github-actions bot requested a review from rene June 17, 2025 07:57

github-actions bot requested a review from milan-zededa June 17, 2025 09:04

github-actions bot requested review from christoph-zededa, jsfakian, naiming-zededa and uncleDecart June 17, 2025 09:26

eriknordmark reviewed Jul 2, 2025

View reviewed changes

pkg/pillar/agentlog/eveversion.go

const (

partitionFile = "/run/eve.id"

partitionFile = types.RunDir + "/eve.id"

Copy link

Contributor

eriknordmark Jul 2, 2025

Choose a reason for hiding this comment

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

Ditto

eriknordmark reviewed Jul 2, 2025

View reviewed changes

eriknordmark requested changes Jul 2, 2025

View reviewed changes

[WIP] refactor: replace hardcoded paths with constants from locationconsts.go (#3682) #4965

Are you sure you want to change the base?

[WIP] refactor: replace hardcoded paths with constants from locationconsts.go (#3682) #4965

Conversation

jeff-zed commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR dependencies

How to test and validate this PR

Changelog notes

PR Backports

Checklist

Uh oh!

jeff-zed commented Jun 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeff-zed commented Jun 13, 2025

Uh oh!

milan-zededa commented Jun 13, 2025

Uh oh!

jeff-zed commented Jun 13, 2025

Uh oh!

OhmSpectator commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rene left a comment

Choose a reason for hiding this comment

Uh oh!

jeff-zed commented Jun 17, 2025

Uh oh!

OhmSpectator commented Jun 17, 2025

Uh oh!

jeff-zed commented Jun 17, 2025

Uh oh!

jeff-zed commented Jun 17, 2025

Uh oh!

jeff-zed commented Jun 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eriknordmark left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jeff-zed commented Jun 12, 2025 •

edited

Loading

eriknordmark left a comment •

edited

Loading