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

Add an option to add Loki to testpachd #9903

Merged
merged 6 commits into from Apr 5, 2024

Conversation

jrockway
Copy link
Member

If set, this option starts Loki and sends all "pachd" logs to it. That means running pachctl logs attached to this testpachd returns the same thing you'd see if a real pachd were running.

I also enabled RPC logs for test pachd, at level Debug (hidden by default, but sent to Loki and the log file).

@robert-uhl robert-uhl changed the title Add and option to add Loki to testpachd Add an option to add Loki to testpachd Mar 29, 2024
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 95.79832% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 58.08%. Comparing base (9697f86) to head (f02a218).
Report is 1 commits behind head on master.

Files Patch % Lines
src/internal/lokiutil/testloki/logger.go 96.00% 3 Missing ⚠️
src/internal/pachd/full.go 96.87% 1 Missing ⚠️
src/internal/pachd/testpachd.go 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9903      +/-   ##
==========================================
+ Coverage   58.00%   58.08%   +0.07%     
==========================================
  Files         608      609       +1     
  Lines       73924    74040     +116     
==========================================
+ Hits        42883    43008     +125     
+ Misses      30485    30475      -10     
- Partials      556      557       +1     

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

src/internal/lokiutil/testloki/logger.go Show resolved Hide resolved
src/internal/lokiutil/testloki/logger.go Show resolved Hide resolved
src/internal/lokiutil/testloki/logger.go Show resolved Hide resolved
src/internal/lokiutil/testloki/logger.go Show resolved Hide resolved
src/testing/testpachd/main.go Show resolved Hide resolved
ctx, cancel := pctx.Interactive()
runCtx := ctx
Copy link
Contributor

Choose a reason for hiding this comment

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

The interleaved usage of ctx and runCtx is very confusing. There should be a single context in any particular scope (modulo a few low-level situations, of course☺), either received as an argument from the caller or created.

Looking at main as a whole, I think that it could be refactored. Perhaps the useLoki conditional could be moved into the pachd run goroutine, or perhaps it could be used to set an array of options which are then used within that goroutine to mutate its context (to the extent that MutateContext is a good idiom …)?

Perhaps main has gotten large enough that it stands to be refactored a bit, perhaps into private setup/run/cleanup functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here are the constraints.

  1. We want everything to cleanly shut down when someone presses C-c. That's pctx.Interactive().

  2. We want to set up pachd and any related services controlled by the cmdline flags. If Loki is one of those flags, we want the rest of the process to send their logs to Loki, which is done via a zapcore inside the context. If Loki isn't one of those flags, we don't want to do anything to the context.

  3. We want to clean up when everything is done. The only way this can really be done is by someone cancelling the root context. Thus, we need a new background context to handle cleanup, because the original context is cancelled but we don't actually want to cancel cleanup work.

So this is why there are three contexts. Building pachd and running pachd are both gated by the Interactive constraint. That way pachd shuts down cleanly when you request it. We use the original context to build pachd because it's what's there at the time. We use runCtx so that if certain OPTIONAL pachd options are specified, pachd.Run() is called with a different context, specifically one that sends all logs from Run to Loki. This way, GetLogs returns pachd logs, just like in the real pachd. We just happen to implement it through a much different mechanism in production.

Overall, I don't think main is oversized. It does this: 1) read the options 2) build pachd 3) run pachd in the background 4) connect to pachd and adjust the config file 5) wait for pachd to stop 6) cleanup the ephemeral data that is outside of the process. That's all it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

If Loki is one of those flags, we want the rest of the process to send their logs to Loki, which is done via a zapcore inside the context.

Is it intentional that that test-Loki pachd.TestPachdOption’s MutateContext gets called twice, with the exact same context, just threaded through two ways: once at https://github.com/pachyderm/pachyderm/pull/9903/files#diff-de6ff7639f65fe2721a8d9d51d4011c6ce1ac07bbdf28729bd1a0b8560b2eaa2R79 and once at https://github.com/pachyderm/pachyderm/pull/9903/files#diff-90e079f0d60c1da7e6170f20220bc81ffc758332c348dc02000c25ed3bd456d0R178? As a result, there will be two different faux-pods (heh), due to regenerating templateHash and procHash.

Copy link
Contributor

@robert-uhl robert-uhl left a comment

Choose a reason for hiding this comment

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

I think that this works for this particular story, but I intend to get #9913 in shape soon as a fast-follow PR.

@jrockway jrockway force-pushed the jonathan/core-2238-loki-in-testpachd branch from 37b4b6f to f02a218 Compare April 5, 2024 02:50
@jrockway jrockway merged commit 59099bc into master Apr 5, 2024
21 checks passed
@jrockway jrockway deleted the jonathan/core-2238-loki-in-testpachd branch April 5, 2024 03:27
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

2 participants