Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extend Run() signature to include baseDir; unify agentInit options #3914

Merged
merged 3 commits into from
May 16, 2024

Conversation

deitch
Copy link
Contributor

@deitch deitch commented May 12, 2024

A few changes in this PR, closely related. None should change functionality, but let's let CI be the judge of that.

1. Extend the pillar service Run() signature to include baseDir

There are a few activities that take place inside start of each service, notably creating a pidfile.

That has been hard-coded to /run/<agentName>.pid in the past. The previous PR #3911 created an option pass a different base directory than /run/.

This PR plumbs the option for the basedir through to each agent via adding an option to Run(), and then passes it on when creating the pidfile. In normal use case, nothing changes. This creates override options.

2. Unify creation of pidfile

Each service does pidfile creation a wee bit differently:

  • Some services use agentbase.Init(agentbase.WithPidFile())
  • Some run agentbase.Init() without that option, and then call pidfile.CheckAndCreatePidfile() a few lines later.
  • Still others do not create a pidfile
  • Others create a pidfile unless a "no pidfile" option is set (on a struct that is create a few lines above and therefore cannot be set, but let's ignore that here)

This unifies all of those to call WithPidFile() unless they do not create a pidfile. Those that have it optional, set the argument optionally.

3. Create a WithBaseDir() option to agentbase.Init()

With this, agentbase.Init() can know where to create the pidfile.

4. Remove unused options from services

The pkg/pillar/cmd/* services all have options for -v for version and -p for "do not check for pid file. None of these is used, at all. I clarified this with @eriknordmark (to give credit, he pointed out to me that these are unused), so they have been removed, in 2 separate commits.

@deitch
Copy link
Contributor Author

deitch commented May 12, 2024

yetus found some minor nits in the modified files. I fixed them. It also found some false positives, which I ignored.

Copy link

codecov bot commented May 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.51%. Comparing base (d864826) to head (e46e62e).
Report is 100 commits behind head on master.

❗ Current head e46e62e differs from pull request most recent head bd5b033. Consider uploading reports for the commit bd5b033 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3914   +/-   ##
=======================================
  Coverage   17.51%   17.51%           
=======================================
  Files           3        3           
  Lines         805      805           
=======================================
  Hits          141      141           
  Misses        629      629           
  Partials       35       35           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@christoph-zededa
Copy link
Contributor

A few changes in this PR, closely related. None should change functionality, but let's let CI be the judge of that.

1. Extend the pillar service Run() signature to include baseDir

There are a few activities that take place inside start of each service, notably creating a pidfile.

That has been hard-coded to /run/<agentName>.pid in the past. The previous PR #3911 created an option pass a different base directory than /run/.

This PR plumbs the option for the basedir through to each agent via adding an option to Run(), and then passes it on when creating the pidfile. In normal use case, nothing changes. This creates override options.

2. Unify creation of pidfile

Each service does pidfile creation a wee bit differently:

* Some services use `agentbase.Init(agentbase.WithPidFile())`

* Some run `agentbase.Init()` _without_ that option, and then call `pidfile.CheckAndCreatePidfile()` a few lines later.

* Still others do not create a pidfile

* Others create a pidfile unless a "no pidfile" option is set (on a struct that is create a few lines above and therefore cannot be set, but let's ignore that here)

This unifies all of those to call WithPidFile() unless they do not create a pidfile. Those that have it optional, set the argument optionally.

3. Create a WithBaseDir() option to agentbase.Init()

With this, agentbase.Init() can know where to create the pidfile.

can you please put this text also into the commit message?

@deitch
Copy link
Contributor Author

deitch commented May 13, 2024

can you please put this text also into the commit message?

Sure, made a short-form version of it. Done.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

The yetus complaints are about unmodified code, so ignoring those.

Re-running eden tests.

@eriknordmark
Copy link
Contributor

A go test failure:
zedbox/zedbox.go:96:27: cannot use zedkube.Run (value of type func(ps *pubsub.PubSub, loggerArg *logrus.Logger, logArg *"github.com/lf-edge/eve/pkg/pillar/base".LogObject, arguments []string, baseDir string) int) as "github.com/lf-edge/eve/pkg/pillar/types".AgentRunner value in struct literal

@deitch deitch force-pushed the run-extensible branch 2 times, most recently from af04e13 to a8bfd5b Compare May 15, 2024 14:30
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

There is also a Version global variable and a line in the Makefile to set main.Version which makes sense removing as part of removing versionPtr.

@deitch
Copy link
Contributor Author

deitch commented May 15, 2024

There is also a Version global variable and a line in the Makefile to set main.Version which makes sense removing as part of removing versionPtr.

I removed it from the Makefile.

@deitch deitch force-pushed the run-extensible branch 4 times, most recently from d251b7c to d26bd25 Compare May 15, 2024 15:14
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

@deitch
Copy link
Contributor Author

deitch commented May 15, 2024

Well, some eden tests pass, which is a distinct improvement.

…agent.Init; remove pidfile creation from all pillar services, replacing with call to agent.Init(WithPidfile()); passed inline or not explicitly to Run()

Signed-off-by: Avi Deitcher <avi@deitcher.net>
Signed-off-by: Avi Deitcher <avi@deitcher.net>
Signed-off-by: Avi Deitcher <avi@deitcher.net>
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Run eden again

@eriknordmark eriknordmark merged commit c2bf448 into lf-edge:master May 16, 2024
21 of 30 checks passed
@deitch deitch deleted the run-extensible branch May 17, 2024 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants