-
Notifications
You must be signed in to change notification settings - Fork 6.7k
fix: error sending generate manifest metadata cmp server #25891
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
base: master
Are you sure you want to change the base?
fix: error sending generate manifest metadata cmp server #25891
Conversation
Signed-off-by: Patroklos Papapetrou <[email protected]>
Signed-off-by: Patroklos Papapetrou <[email protected]>
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
| conn, cmpClient, connFound = cmpSupports(ctx, pluginSockFilePath, appPath, repoPath, file.Name(), env, tarExcludedGlobs, false) | ||
| conn, cmpClient, connFound, lastErr = cmpSupports(ctx, pluginSockFilePath, appPath, repoPath, file.Name(), env, tarExcludedGlobs, false) |
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.
I might be misunderstanding the intent here, so I just want to make sure I understand correctly.
In discovery mode, iterate over multiple socket files. If the first probe returns an error like EOF, but a later probe returns connfound=false, lastErr=nil (not supported), does that later result overwrite lastErr and effectively drop the earlier error?
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.
@choejwoo yes that is correct
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.
@choejwoo yes that is correct
Thanks for confirming, LGTM.
Closes #25511
This PR has a two-level fix
The first commit addresses the actual issue as described below
reposerver tries to detect the CMP sidecar (
DetectConfigManagementPlugin). If the detection fails, it returns: "could not find cmp-server plugin with name %q supporting the given repository". That is exactly the message in the user's logs.If detect succeeds, reposerver opens a GenerateManifest stream:
generateManifestStream, err := cmpClient.GenerateManifest(ctx, grpc_retry.Disable())It then calls
SendRepoStreamto send metadata, but the code currently passes thisgenerateManifestStream.Context()as context. If this context is by any reason cancelled/closed early on the server-side (for any reason) the client-side will return an error (sometimes io.EOF), which is wrapped and logged as: "error sending generate manifest metadata to cmp-server: EOF". The PR now uses the original context.The second commit improves the diagnostic/debugging level of
DetectConfigManagementPluginfunction as belowCurrently,
DetectConfigManagementPluginreturns a generic error when a named plugin is not found/usable: "could not find cmp-server plugin with name %q supporting the given repository".cmpSupportsfunction logs detailed reasons but does not return the concrete error to the caller.Checklist: