feat(module/git): allow module.git use cached content when failed to fetch #5712

hainenber · 2023-11-04T05:42:19Z

PR Description

Allow module.git loader to use cached contents when failed to fetch during startup/update. This should be transient as the next poll interval, if fetch failure got resolved, the Git content will be up-to-date.

Which issue(s) this PR fixes

Fixes #5708

Notes to the Reviewer

Do let me know if something more format is needed, like a unit test or so.

PR Checklist

CHANGELOG.md updated
Tests updated

rfratto

Thanks! I have a comment about the approach but I think you only need a few small tweaks to get this ready for merging.

component/module/git/git.go

rfratto · 2023-11-11T16:16:20Z

component/module/git/internal/vcs/git.go

+	fetchRepoErr := repo.FetchContext(ctx, &git.FetchOptions{
 		RemoteName: "origin",
 		Force:      true,
 		Auth:       opts.Auth.Convert(),
 	})
-	if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) {
-		return nil, UpdateFailedError{
+	if fetchRepoErr != nil && !errors.Is(fetchRepoErr, git.NoErrAlreadyUpToDate) {
+		err = UpdateFailedError{
 			Repository: opts.Repository,
-			Inner:      err,
+			Inner:      fetchRepoErr,


Hm, I'd rather the error handler here return instead of falling back to Reset, that way we don't touch the repo at all in case a pull fails; something like

if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) { workTree, workTreeErr := repo.Worktree() if workTreeErr != nil { return nil, workTreeErr } return &GitRepo{ opts: opts, repo: repo, workTree: workTree, }, UpdateFailedError{ Repository: opts.Repository, Inner: err, } }

(This can probably be refactored a bit so the GitRepo object is constructed before doing a pull to reduce duplication with the successful path)

Good point here. I've amended in latest commit.

rfratto · 2023-11-11T16:16:56Z

component/module/git/internal/vcs/git.go

+		err = UpdateFailedError{
 			Repository: repo.opts.Repository,
-			Inner:      err,
+			Inner:      fetchRepoErr,


Ditto here: I'm not sure we should be falling back to resetting the git repo when the update fails, and I think we should just return directly here.

(Also, this instance of err is always overwritten by the assignment on line 125 below)

Good point here. I've amended in latest commit.

rfratto · 2023-11-11T16:21:30Z

component/module/git/git.go

+	// Failure to update repository makes the module loader temporarily use cached contents on disk
+	var updateVcsErr vcs.UpdateFailedError
 	if c.repo == nil || !reflect.DeepEqual(repoOpts, c.repoOpts) {
 		r, err := vcs.NewGitRepo(context.Background(), repoPath, repoOpts)
 		if err != nil {
-			return err
+			if errors.As(err, &updateVcsErr) {
+				level.Error(c.log).Log("msg", "failed to update repository", "err", err)
+				c.updateHealth(err)
+			} else {
+				return err
+			}


Hm, I'm still not sure if the error check needs to happen here in the Update or in the call to pollFile.

It seems like it would be sufficient if New made the check instead, since we're really only interested about fixing component construction to use a cache:

// New creates a new module.git component. func New(o component.Options, args Arguments) (*Component, error) { m, err := module.NewModuleComponent(o) if err != nil { return nil, err } c := &Component{ opts: o, log: o.Logger, mod: m, argsChanged: make(chan struct{}, 1), } // Only acknowledge the error from Update if it's not a // vcs.UpdateFailedError; vcs.UpdateFailedError means that the Git repo // exists but we were just unable to update it. if err := c.Update(args); err != nil && !errors.Is(err, &vcs.UpdateFailedError{}) { return nil, err } return c, nil }

I think adding that check in New will allow you to leave Update and pollFile unchanged. WDYT?

I was implementing this logic until finding out a crash due to nil pointer as repo attribute isn't populated due to removal of assignment at Update method

To proceed, I think we can remove changes at pollFile and keep the change at Update.

There's no panic when there's only changes at Update method and the module's debug info is pretty clear

My bad, pollFile also returns vcs.UpdateFailedError so the New method also has to have the changes as well. I've finalized in the latest commit

hainenber · 2023-11-11T17:29:12Z

GH checks is failing due to degraded performance on DockerHub's side. Please help rerunning when it's better, Robert. Thanks!

rfratto

Sorry for the delay between reviews, there was a holiday yesterday. This is getting much closer!

rfratto · 2023-11-14T18:35:19Z

component/module/git/git.go

-		return nil, err
+		if errors.As(err, &vcs.UpdateFailedError{}) {
+			level.Error(c.log).Log("msg", "failed to update repository", "err", err)
+			c.updateHealth(err)


This shouldn't be necessary since the call to Update will already update the health for you if it returns an error.

Oh, I wasn't aware of this internal detail :D

Removed updateHealth in latest commit

rfratto · 2023-11-14T18:36:45Z

component/module/git/git.go

+			if errors.As(err, &vcs.UpdateFailedError{}) {
+				level.Error(c.log).Log("msg", "failed to update repository", "err", err)
+				c.updateHealth(err)
+			} else {
+				return err
+			}


I'm not sure I'm following whether this is necessary still; with the latest change, it's currently the callers responsibility for whether UpdateFailedError needs separate handling or not.

Am I missing something that this check is changing?

The err check for UpdateFailedError in New method is to check for returned error from pollFile method.

This check right here is to inform Flow controller component's unhealthiness from failing to update Git repo and to continue assigning the Repo object and options to internal attributes. Returning early will leave the attributes, namely r.repo empty and can cause null pointer exception, I'd wager.

rfratto · 2023-11-14T18:41:01Z

I can't run the CI because of merge conflicts, can you resolve those?

…fetch Signed-off-by: hainenber <[email protected]>

Signed-off-by: hainenber <[email protected]>

hainenber · 2023-11-16T16:11:30Z

I can't run the CI because of merge conflicts, can you resolve those?

Thanks for the heads-up, done resolving on my side!

rfratto

Thanks!

…fetch (grafana#5712) Signed-off-by: hainenber <[email protected]> (cherry picked from commit 39b78a8)

* packaging: fix grafana-agent-flow.river (#5802) (#5809) * packaging: fix grafana-agent-flow.river (#5802) Add needed label in default `grafana-agent-flow.river` default configuration used in downstream packages. * Update CHANGELOG.md (cherry picked from commit eac661f) * misc: fix changelog entry (#5812) PR #5802 accidentally added the changelog entry to a published release. (cherry picked from commit bc3babc) * feat(module/git): allow module.git use cached content when failed to fetch (#5712) Signed-off-by: hainenber <[email protected]> (cherry picked from commit 39b78a8) * Fix converter output for prometheus.exporter.windows to not unnecessarily add empty blocks (#5817) (cherry picked from commit 165914b) * delete obsolete broken windows test (#5818) (cherry picked from commit bcbc7ab) * prepare for release v0.38.0-rc.1 (#5821) (cherry picked from commit 7d775c1) --------- Co-authored-by: Brice Waegeneire <[email protected]> Co-authored-by: Đỗ Trọng Hải <[email protected]> Co-authored-by: Erik Baranowski <[email protected]>

hainenber requested a review from a team as a code owner November 4, 2023 05:42

rfratto self-assigned this Nov 10, 2023

rfratto reviewed Nov 10, 2023

View reviewed changes

component/module/git/git.go Outdated Show resolved Hide resolved

rfratto reviewed Nov 11, 2023

View reviewed changes

hainenber force-pushed the allow-module-git-to-use-cached-content branch from 3c49ca7 to 6cd7029 Compare November 11, 2023 17:26

rfratto reviewed Nov 14, 2023

View reviewed changes

hainenber added 6 commits November 16, 2023 23:10

feat(module/git): allow module.git use cached content when failed to …

85dd9c2

…fetch Signed-off-by: hainenber <[email protected]>

fix(module/git): only return non-update err when failed to fetch

5c364ea

Signed-off-by: hainenber <[email protected]>

fix(module/git): set comp as unhealthy for failed git pull

47458c7

Signed-off-by: hainenber <[email protected]>

fix(module/git): not reset worktree when failed git pull

4ee653c

Signed-off-by: hainenber <[email protected]>

refactor(module/git): tighten cache usage logic

bff48c1

Signed-off-by: hainenber <[email protected]>

chore(comp/git): remove redundant health updater

d5e93a0

Signed-off-by: hainenber <[email protected]>

hainenber force-pushed the allow-module-git-to-use-cached-content branch from 6cd7029 to d5e93a0 Compare November 16, 2023 16:11

rfratto approved these changes Nov 20, 2023

View reviewed changes

rfratto merged commit 39b78a8 into grafana:main Nov 20, 2023
8 checks passed

rfratto pushed a commit to rfratto/agent that referenced this pull request Nov 20, 2023

feat(module/git): allow module.git use cached content when failed to …

a51f1dd

…fetch (grafana#5712) Signed-off-by: hainenber <[email protected]> (cherry picked from commit 39b78a8)

github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(module/git): allow module.git use cached content when failed to fetch #5712

feat(module/git): allow module.git use cached content when failed to fetch #5712

hainenber commented Nov 4, 2023

rfratto left a comment

rfratto Nov 11, 2023

hainenber Nov 11, 2023

rfratto Nov 11, 2023

hainenber Nov 11, 2023

rfratto Nov 11, 2023

hainenber Nov 11, 2023

hainenber Nov 11, 2023

hainenber Nov 11, 2023

hainenber commented Nov 11, 2023

rfratto left a comment

rfratto Nov 14, 2023

hainenber Nov 16, 2023

rfratto Nov 14, 2023

hainenber Nov 16, 2023

rfratto commented Nov 14, 2023

hainenber commented Nov 16, 2023

rfratto left a comment

feat(module/git): allow module.git use cached content when failed to fetch #5712

feat(module/git): allow module.git use cached content when failed to fetch #5712

Conversation

hainenber commented Nov 4, 2023

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

rfratto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hainenber commented Nov 11, 2023

rfratto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfratto commented Nov 14, 2023

hainenber commented Nov 16, 2023

rfratto left a comment

Choose a reason for hiding this comment