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

Various fixes for managed transport #637

Merged
merged 6 commits into from
Mar 28, 2022
Merged

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Mar 24, 2022

Resolves some known issues:

  • ssh.Dial hangs indefinitely
  • HTTP leaked connections
  • SSH leaked connections
  • Intermittent SSH errors
  • Panic when closing SSH connections

And also improve overall observability.

@pjbgf pjbgf force-pushed the fix-managed-transport branch 4 times, most recently from 486e8ba to 08c3490 Compare March 25, 2022 19:07
Paulo Gomes added 5 commits March 25, 2022 19:08
The experimental managed transport can also leverage TransportPool,
moving it to its own package to accommodate that use case.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Ensure all requests are completely processed and closed,
to prove odds of the underlying connections to be reused.

The transport now is pooled and reused whenever possible.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@pjbgf pjbgf marked this pull request as ready for review March 25, 2022 19:20
@pjbgf
Copy link
Member Author

pjbgf commented Mar 25, 2022

To test the fixes, you must opt-in to the managed transport by setting the environment variable EXPERIMENTAL_GIT_TRANSPORT to true for source-controller's Deployment.
Do the same for image-automation-controller if you have it installed.

Use one of temporary images below or build from this PR:

quay.io/paulinhu/source-controller:v0.22.3-transport@sha256:84538232d2847f31793c14192d98d3e1b4c34ea4a179e1faa9186501afb7ee47

quay.io/paulinhu/image-automation-controller:v0.21.1-transport@sha256:bb0f716179c13add6ba019489b9bbf1e77aa2d0badb6804ffb75401c7a982695

Note that managed transport only works with the libgit2 implementation, and therefore your GitRepository objects must be set accordingly.

The underlying SSH connections are kept open and are reused
across several SSH sessions. This is due to upstream issues in
which concurrent/parallel SSH connections may lead to instability.

golang/go#51926
golang/go#27140
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!
Great work on investigating and fixing all the issues.

@darkowlzz darkowlzz added the area/git Git related issues and pull requests label Mar 28, 2022
@pjbgf pjbgf merged commit 90f34d5 into fluxcd:main Mar 28, 2022
@pjbgf pjbgf deleted the fix-managed-transport branch March 28, 2022 13:02
@pjbgf pjbgf added this to the GA milestone Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/git Git related issues and pull requests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants