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.

Sign up for GitHub

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

Jump to bottom

Add pure SSH LFS support #31516

Merged
merged 9 commits into from
Sep 27, 2024
Merged

Add pure SSH LFS support #31516

merged 9 commits into from
Sep 27, 2024
+945 −53

Conversation

ConcurrentCrab
Copy link
Contributor

@ConcurrentCrab ConcurrentCrab commented Jun 28, 2024 •
edited by lunny
Loading

Fixes #17554
/claim #17554

Docs PR https://gitea.com/gitea/docs/pulls/49

To test, run pushes like: GIT_TRACE=1 git push. The trace output should mention "pure SSH connection".

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 28, 2024
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 28, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/docs modifies/dependencies labels Jun 28, 2024
This was referenced Jun 28, 2024
Closed
Closed
@lunny lunny added this to the 1.23.0 milestone Jun 28, 2024
@ConcurrentCrab
Copy link
Contributor Author

@lunny added copyright headers.

@ConcurrentCrab ConcurrentCrab force-pushed the lfs-ssh branch 2 times, most recently from efe3893 to d506260 Compare June 28, 2024 12:37
@ConcurrentCrab
Copy link
Contributor Author

Improved error handling.

@ConcurrentCrab
Copy link
Contributor Author

Accidentally resolved the conversation about DB connections above, but reiterating:

Pushes and clones work... but there's sometimes (by which I mean decently reproducible) bug where after transferring and verification and everything... the connection just hangs for almost exactly 120 seconds. After which it just ends gracefully and git proceeds with its push.

It's particularly nasty, I have no leads on it yet. The best I can figure out is that it looks like the git-lfs client seems to not send the quit command that it should at the end of the protocol, or that it isn't coming through.

@ConcurrentCrab
Copy link
Contributor Author

Added locking API support.

Well at least I figured out what's causing the hang. Setting lfs.ssh.automultiplex to false on the client side:
https://github.com/git-lfs/git-lfs/blob/8e36a03d85bf05bbca004dd7e8b55b147809b3e0/docs/man/git-lfs-config.adoc?plain=1#L90
gets rid of the hang. The bad news is that it's true by default.

I think this is feature interacting badly with the SSH server?

I traced the traffic, and indeed, something weird is happening on the client side. Git LFS spawns several workers each making its own connection. In a multiplexed connection, only the "first" spawn of the our commands gets a quit message. The rest of them have to wait 120 seconds for an EOF. I'm almost tempted to believe this is a bug on the git-lfs client side.

Can you check and see if this is reproducible in your environments?

@ConcurrentCrab
Copy link
Contributor Author

Yeah, I'm not going crazy. This commit:
git-lfs/git-lfs@44b8801
breaks pure SSH LFS sessions.

And this has history, it isn't even the first time lol:
git-lfs/git-lfs#5537

@ConcurrentCrab
Copy link
Contributor Author

just made: git-lfs/git-lfs#5816

@ConcurrentCrab ConcurrentCrab force-pushed the lfs-ssh branch 2 times, most recently from 5fadb5d to de9a3cf Compare July 1, 2024 20:15
lafriks
lafriks reviewed Jul 2, 2024
View reviewed changes
Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

As pointed in quite some places already but all should be fixed

modules/lfstransfer/backend/backend.go Outdated Show resolved Hide resolved
modules/lfstransfer/backend/backend.go Outdated Show resolved Hide resolved
modules/lfstransfer/backend/lock.go Outdated Show resolved Hide resolved
modules/lfstransfer/backend/lock.go Outdated Show resolved Hide resolved
@ConcurrentCrab
Copy link
Contributor Author

@lafriks adressed the url joins.

@6543
Copy link
Member

6543 commented Sep 20, 2024

I would be fine with the current state, just mention the lfs pull on the docu and the example ini next to tje enable option

@ConcurrentCrab
modules/lfstransfer: add a backend and runner
bc7ae76 
Also add handler in runServ()

Doesn't add support for the locking API yet
@ConcurrentCrab
modules/lfstransfer: add locking support
f819027
@lunny
Copy link
Member

lunny commented Sep 24, 2024 •
edited
Loading

It looks like LFS_ALLOW_PURE_SSH hasn't been read from [lfs] but [server]. Otherwise I can test it successfully. And you need to add an example in app.example.ini file.

@ConcurrentCrab
Copy link
Contributor Author

It looks like LFS_ALLOW_PURE_SSH hasn't been read from [lfs] but [server]. Otherwise I can test it successfully. And you need to add an example in app.example.ini file.

I added it under server because LFS_START_SERVER was there already.

@lunny
Copy link
Member

lunny commented Sep 26, 2024

It looks like LFS_ALLOW_PURE_SSH hasn't been read from [lfs] but [server]. Otherwise I can test it successfully. And you need to add an example in app.example.ini file.

I added it under server because LFS_START_SERVER was there already.

OK. It's a legacy problem. So could you add example in app.example.ini?

@github-actions github-actions bot added the docs-update-needed The document needs to be updated synchronously label Sep 26, 2024
@ConcurrentCrab
Copy link
Contributor Author

@lunny added it to the example config.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 26, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 26, 2024
@techknowlogick techknowlogick merged commit 8a9fd7f into go-gitea:main Sep 27, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 27, 2024
@techknowlogick
Copy link
Member

Thanks so much @ConcurrentCrab!!! <3

@techknowlogick
Copy link
Member

I'll sort out Algora stuff right now. Related, if you have other issues you'd like to suggest for bounties, please get in touch with me, as I'm working on a list right now.

@ConcurrentCrab
Copy link
Contributor Author

ConcurrentCrab commented Sep 27, 2024 •
edited
Loading

Thanks! and I appreciate the tip.

Related, if you have other issues you'd like to suggest for bounties, please get in touch with me, as I'm working on a list right now.

Was this directed at me? If so, I'd be happy to take a shot at more bounties. I quite enjoyed the work part of this.

My only sore point here was the turnaround time on this one. I get that y'all have a business to run. One feature PR isn't exactly at the top of the priority list, I wasn't expecting instant replies or anything. But to me it felt like this was basically completely ignored for quite a while, and as a result took far longer than it should have.

@6543
Copy link
Member

6543 commented Sep 27, 2024 •
edited
Loading

-> mind the documentation ...
... i'm fine with the pull itselve but next to the config flag we should link to the upstream issue

@6543
Copy link
Member

6543 commented Sep 27, 2024 •
edited
Loading

But to me it felt like this was basically completely ignored for quite a while, and as a result took far longer than it should have.

Sorry for that i personnally was just waiting for some references in the docs and you would also have got my lgtm

(#31516 (comment))

@ConcurrentCrab
Copy link
Contributor Author

-> mind the documentation ... ... i'm fine with the pull itselve but next to the config flag we should link to the upstream issue

@6543 does this cover what you mean: https://gitea.com/gitea/docs/pulls/49/files

I thought this was linked in reply to you above somewhere, but the discussion might've been collapsed before you saw it.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 30, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
30e6a94 
* giteaofficial/main:
  Change the code search to sort results by relevance (go-gitea#32134)
  [skip ci] Updated translations via Crowdin
  Add pure SSH LFS support (go-gitea#31516)
@wxiaoguang

This comment was marked as resolved.

Sign in to view
@wxiaoguang wxiaoguang added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Nov 13, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@6543 6543 6543 left review comments

@lafriks lafriks lafriks left review comments

@techknowlogick techknowlogick techknowlogick approved these changes

@lunny lunny lunny approved these changes

@delvh delvh delvh left review comments

Assignees
No one assigned
Labels
docs-update-needed The document needs to be updated synchronously lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/dependencies modifies/go Pull requests that update Go code size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.

Support LFS purely over SSH protocol
9 participants
@ConcurrentCrab @lunny @6543 @techknowlogick @SherryYanmengTang @wxiaoguang @lafriks @delvh @GiteaBot