-
Notifications
You must be signed in to change notification settings - Fork 187
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
libgit2: decommission unmanaged transport #819
Conversation
|
||
// GitManagedTransport implements a managed transport for GitRepository | ||
// objects that use the libgit2 implementation. | ||
// | ||
// When enabled, improves the reliability of libgit2 reconciliations, | ||
// by enforcing timeouts and ensuring libgit2 cannot hijack the process | ||
// and hang it indefinitely. | ||
GitManagedTransport = "GitManagedTransport" |
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.
In terms of phasing out feature gates we probably have two options:
- Completely remove the option (as per the current change).
- Warn an option is no longer supported and remove it a few minors down the line.
The former is the easiest, but it will cause folks upgrading with said option to fail indefinitely until they change their setup. The latter is more work, and would be needed for post GA.
Given how short-lived this feature gate was, and that we are pre-GA. I would favour the former approach, with the caveat that we should add a note on the release.
As part of this, we probably want to rename the tests with |
6adf2b3
to
6976b15
Compare
6976b15
to
b7133d7
Compare
This is what a GitRepository object's conditions looks like if the transports aren't enabled (manually tested by removing the call to
|
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.
LGTM. Thanks @aryan9600 🙇
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.
LGTM
Thanks @aryan9600 🙇
We need to update the spec docs where we mention about the managed transport feature flag - Managed transport for libgit2 Git implementation . |
782b0d1
to
b6b0886
Compare
pkg/git/libgit2/checkout.go
Outdated
// and transport level code. | ||
// Performing all fetch operations with the TransportOptionsURL as the URL, lets the managed | ||
// transport action use it to fetch the registered transport options which contains the | ||
// _actual_ target URL and the correct credentials to use. |
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.
This description seems to be for what's happening in registerManagedTransportOptions()
. It may be better to move it there.
I too did some manual testing. With an existing working GitRepo, when I restart with failing transport, the status shows: conditions:
- lastTransitionTime: "2022-07-08T12:29:43Z"
message: libgit2 managed transport not initialized
observedGeneration: 2
reason: Libgit2TransportNotEnabled
status: "True"
type: Stalled
- lastTransitionTime: "2022-07-08T12:29:43Z"
message: libgit2 managed transport not initialized
observedGeneration: 2
reason: Libgit2TransportNotEnabled
status: "False"
type: Ready
- lastTransitionTime: "2022-07-08T12:16:43Z"
message: stored artifact for revision 'master/b76b1a38c9c03c763dc56e996daf216606f4c088'
observedGeneration: 2
reason: Succeeded
status: "True"
type: ArtifactInStorage
contentConfigChecksum: sha256:fcbcf165908dd18a9e49f7ff27810176db8e9f63b4352213741664245224f8aa
observedGeneration: 2
url: http://source-controller.flux-system.svc.cluster.local./gitrepository/flux-system/podinfo/latest.tar.gz After restarting with working transport, the status shows: conditions:
- lastTransitionTime: "2022-07-08T12:31:15Z"
message: stored artifact for revision 'master/b76b1a38c9c03c763dc56e996daf216606f4c088'
observedGeneration: 2
reason: Succeeded
status: "True"
type: Ready
- lastTransitionTime: "2022-07-08T12:16:43Z"
message: stored artifact for revision 'master/b76b1a38c9c03c763dc56e996daf216606f4c088'
observedGeneration: 2
reason: Succeeded
status: "True"
type: ArtifactInStorage
contentConfigChecksum: sha256:fcbcf165908dd18a9e49f7ff27810176db8e9f63b4352213741664245224f8aa
observedGeneration: 2
url: http://source-controller.flux-system.svc.cluster.local./gitrepository/flux-system/podinfo/latest.tar.gz So, the conditions are handled well. But the artifact URL is advertised even when there's nothing in the storage in stalled state. This results in a kustomization that uses this GitRepo to fail with 404 status: {"level":"error","ts":"2022-07-08T12:34:31.572Z","logger":"controller.kustomization","msg":"Reconciliation failed after 7.484495ms, next try in 5m0s","reconciler group":"kustomize.toolkit.fluxcd.io","reconciler kind":"Kustomization","name":"podinfo","namespace":"flux-system","revision":"master/b76b1a38c9c03c763dc56e996daf216606f4c088","error":"failed to download artifact from http://source-controller.flux-system.svc.cluster.local./gitrepository/flux-system/podinfo/b76b1a38c9c03c763dc56e996daf216606f4c088.tar.gz, status: 404 Not Found"} Looks like we'll have to run |
b6b0886
to
4fe4e74
Compare
Moving the managed transport enabled check to GitRepo conditions when managed transport enabled check passes:
GitRepo conditions after the controller restarts and the check isn't satisfied:
|
As part of the decommissioning, we should move into using the base image |
This change will be made in a follow up PR, as this PR is already targeting quite a few things now. |
Please hold merging until we release 0.25.10, this PR should be included in SC 0.26.0 |
1133bd7
to
4c261d2
Compare
4c261d2
to
8411e54
Compare
8411e54
to
4c4429c
Compare
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.
LGTM!
Thanks a lot.
@aryan9600 can you rebase please? |
Decommission libgit2 unmanaged transport and remove the related feature gate, making managed transport the default. Signed-off-by: Sanskar Jaiswal <[email protected]>
Update initRepoWithRemote() so that it overwrites the remote url with the provided url if the remote already exists, instead of erroring out. Signed-off-by: Sanskar Jaiswal <[email protected]>
Attach context to HTTP requests at the transport level to honour timeouts. Signed-off-by: Sanskar Jaiswal <[email protected]>
Signed-off-by: Sanskar Jaiswal <[email protected]>
…on knowledge Signed-off-by: Sanskar Jaiswal <[email protected]>
4c4429c
to
0978a7a
Compare
Decommission libgit2 unmanaged transport and remove the related feature
gate, making managed transport the only available option.
Update
initRepoWithRemote()
so that it overwrites the remote url withthe provided url if the remote already exists, instead of erroring out.
Attach context to http requests to enforce timeouts.
Relates to: #782
Fixes: #697 (since the flaky test was a part of the unmanaged transport)
Signed-off-by: Sanskar Jaiswal [email protected]