-
Notifications
You must be signed in to change notification settings - Fork 156
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
Use git partial clone and worktree to reduce network/file io #5412
Changes from 11 commits
e6a8296
fed7b3a
0dcae1d
971d426
51ce6f3
99f25e7
2a0ee56
4b8a7e3
bb90ea0
951826e
d0c77ee
51e9c2c
c5216c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,6 +191,13 @@ func (p *provider) copy(lw io.Writer) (*DeploySource, error) { | |
return nil, err | ||
} | ||
|
||
// To avoid the git operation on copied source. | ||
// TODO: do not copy the .git directory to improve the performance. | ||
if err := os.RemoveAll(filepath.Join(dest, ".git")); err != nil { | ||
fmt.Fprintf(lw, "Unable to remove the .git directory from the copied deploy source (%v)\n", err) | ||
return nil, err | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about using a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I'll try. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use the alpine as the base image for piped, so I checked the tar command in the piped image. Note: I checked
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the version without -P, the more universal solution
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OKay, thank you for providing this. I'll use this script. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried some tar options and finally reached this commit. |
||
|
||
return &DeploySource{ | ||
RepoDir: dest, | ||
AppDir: filepath.Join(dest, p.appGitPath.Path), | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,8 @@ var ( | |
type Repo interface { | ||
GetPath() string | ||
GetClonedBranch() string | ||
Copy(dest string) (Repo, error) | ||
Copy(dest string) (Worktree, error) | ||
CopyToModify(dest string) (Repo, error) | ||
Comment on lines
+38
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [IMO] How about using the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It sounds clear, but I want to leave here as-is. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I got your point! I agree with it. |
||
|
||
ListCommits(ctx context.Context, visionRange string) ([]Commit, error) | ||
GetLatestCommit(ctx context.Context) (Commit, error) | ||
|
@@ -52,6 +53,17 @@ type Repo interface { | |
CommitChanges(ctx context.Context, branch, message string, newBranch bool, changes map[string][]byte, trailers map[string]string) error | ||
} | ||
|
||
// Worktree provides functions to get and handle git worktree. | ||
// It is a separate checkout of the repository. | ||
// It is used to make changes to the repository without affecting the main repository. | ||
// Worktree always does checkout with the detached HEAD, so it doesn't affect the main repository. | ||
type Worktree interface { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this to restrict operation at worktree. |
||
GetPath() string | ||
Clean() error | ||
Copy(dest string) (Worktree, error) | ||
Checkout(ctx context.Context, commitish string) error | ||
} | ||
|
||
type repo struct { | ||
dir string | ||
gitPath string | ||
|
@@ -60,6 +72,55 @@ type repo struct { | |
gitEnvs []string | ||
} | ||
|
||
// worktree is a git worktree. | ||
// It is a separate checkout of the repository. | ||
type worktree struct { | ||
base *repo | ||
worktreePath string | ||
} | ||
|
||
func (r *worktree) runGitCommand(ctx context.Context, args ...string) ([]byte, error) { | ||
cmd := exec.CommandContext(ctx, r.base.gitPath, args...) | ||
cmd.Dir = r.worktreePath | ||
cmd.Env = append(os.Environ(), r.base.gitEnvs...) | ||
return cmd.CombinedOutput() | ||
} | ||
|
||
func (r *worktree) Copy(dest string) (Worktree, error) { | ||
// garbage collecting worktrees | ||
if _, err := r.runGitCommand(context.Background(), "worktree", "prune"); err != nil { | ||
// ignore the error | ||
} | ||
|
||
if out, err := r.runGitCommand(context.Background(), "worktree", "add", "--detach", dest); err != nil { | ||
return nil, formatCommandError(err, out) | ||
} | ||
|
||
return &worktree{ | ||
base: r.base, | ||
worktreePath: dest, | ||
}, nil | ||
} | ||
|
||
func (r *worktree) GetPath() string { | ||
return r.worktreePath | ||
} | ||
|
||
func (r *worktree) Clean() error { | ||
if out, err := r.base.runGitCommand(context.Background(), "worktree", "remove", r.worktreePath); err != nil { | ||
return formatCommandError(err, out) | ||
} | ||
return nil | ||
} | ||
|
||
func (r *worktree) Checkout(ctx context.Context, commitish string) error { | ||
out, err := r.runGitCommand(ctx, "checkout", "--detach", commitish) | ||
if err != nil { | ||
return formatCommandError(err, out) | ||
} | ||
return nil | ||
} | ||
|
||
// NewRepo creates a new Repo instance. | ||
func NewRepo(dir, gitPath, remote, clonedBranch string, gitEnvs []string) *repo { | ||
return &repo{ | ||
|
@@ -81,22 +142,55 @@ func (r *repo) GetClonedBranch() string { | |
return r.clonedBranch | ||
} | ||
|
||
// Copy does copying the repository to the given destination. | ||
// Copy does copying the repository to the given destination using git worktree. | ||
// The repository is cloned to the given destination with the detached HEAD. | ||
// NOTE: the given “dest” must be a path that doesn’t exist yet. | ||
// If you don't, it copies the repo root itself to the given dest as a subdirectory. | ||
func (r *repo) Copy(dest string) (Repo, error) { | ||
cmd := exec.Command("cp", "-rf", r.dir, dest) | ||
out, err := cmd.CombinedOutput() | ||
if err != nil { | ||
// If you don't, you will get an error. | ||
func (r *repo) Copy(dest string) (Worktree, error) { | ||
// garbage collecting worktrees | ||
if _, err := r.runGitCommand(context.Background(), "worktree", "prune"); err != nil { | ||
// ignore the error | ||
} | ||
|
||
if out, err := r.runGitCommand(context.Background(), "worktree", "add", "--detach", dest); err != nil { | ||
return nil, formatCommandError(err, out) | ||
} | ||
|
||
return &repo{ | ||
return &worktree{ | ||
base: r, | ||
worktreePath: dest, | ||
}, nil | ||
} | ||
|
||
// CopyToModify does cloning the repository to the given destination. | ||
// The repository is cloned to the given destination with the . | ||
// NOTE: the given “dest” must be a path that doesn’t exist yet. | ||
// If you don't, you will get an error. | ||
func (r *repo) CopyToModify(dest string) (Repo, error) { | ||
cmd := exec.Command(r.gitPath, "clone", r.dir, dest) | ||
if out, err := cmd.CombinedOutput(); err != nil { | ||
return nil, formatCommandError(err, out) | ||
} | ||
|
||
cloned := &repo{ | ||
dir: dest, | ||
gitPath: r.gitPath, | ||
remote: r.remote, | ||
clonedBranch: r.clonedBranch, | ||
}, nil | ||
gitEnvs: r.gitEnvs, | ||
} | ||
|
||
// because we did a local cloning so set the remote url of origin | ||
if err := cloned.setRemote(context.Background(), r.remote); err != nil { | ||
return nil, err | ||
} | ||
|
||
// fetch the latest changes which doesn't exist in the local repository | ||
if out, err := cloned.runGitCommand(context.Background(), "fetch"); err != nil { | ||
return nil, formatCommandError(err, out) | ||
} | ||
|
||
return cloned, nil | ||
} | ||
|
||
// ListCommits returns a list of commits in a given revision range. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Warashi
it seems that the
dir
is removed after executing theloadHeadServiceManifest
So we might not call
repo.Clean()
. WDYT?https://github.com/pipe-cd/pipecd/blob/c5216c91bffc906f57b30133f3d0e557f1b1888f/pkg/app/piped/driftdetector/kubernetes/detector.go#L270C1-L281C22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ffjlabo
I think we should call
repo.Clean()
here because the 'repo.Clean()' not only does the directory deletion.IMO, we should call some methods like
Clean()
orClose()
everywhere because the caller doesn't know what happens with such methods. The caller only knows the cleanup methods exist.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Warashi
I got it. The
git worktree
needs additional operation other than deleting the directory to keep the consistency of the worktree as you said.Also I agree with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little off topic, but the detector reuses the one repo.
loadHeadServiceManifest
also uses it as an arg. So I feel a little bit confused about cleaning at this point.But I noticed that this section is used temporarily and different from the actual repo passed as arg. 🙏