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

fix: stop discovery errors from halting chart rendering. #6908

Merged
merged 1 commit into from
Nov 7, 2019
Merged

fix: stop discovery errors from halting chart rendering. #6908

merged 1 commit into from
Nov 7, 2019

Conversation

technosophos
Copy link
Member

This blocks a particular error (caused by upstream discovery client),
printing a warning instead of failing. It's not a great solution, but is
a stop-gap until Client-Go gets fixed.

Closes #6361

Signed-off-by: Matt Butcher matt.butcher@microsoft.com

@technosophos technosophos self-assigned this Nov 7, 2019
@helm-bot helm-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 7, 2019
@technosophos
fix: stop discovery errors from halting chart rendering.
193ff2b 
This blocks a particular error (caused by upstream discovery client),
printing a warning instead of failing. It's not a great solution, but is
a stop-gap until Client-Go gets fixed.

Closes helm#6361

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
@technosophos technosophos merged commit 865c46c into helm:master Nov 7, 2019
@technosophos technosophos deleted the fix/6361-discovery-errors branch November 7, 2019 23:16
jglick
jglick reviewed Nov 8, 2019
View reviewed changes
pkg/action/action.go
// Client-Go emits an error when an API service is registered but unimplemented.
// We trap that error here and print a warning. But since the discovery client continues
// building the API object, it is correctly populated with all valid APIs.
// See https://github.com/kubernetes/kubernetes/issues/72051#issuecomment-521157642
Copy link

Choose a reason for hiding this comment

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

kubernetes/kubernetes#72051 (comment) for the record.

@kenperkins
Copy link
Contributor

I'm not entirely sure this fix is accurate. I've looked through the code and what is documented doesn't appear to be the case:

But since the discovery client continues building the API object, it is correctly populated with all valid APIs.

If you look at https://github.com/helm/helm/blob/master/pkg/action/action.go#L171 you see that any error from client.ServerGroupsAndResources returns the chartutil.DefaultVersionSet, not anything returned from client.ServerGroupsAndResources.

If you look at https://github.com/kubernetes/client-go/blob/bec269661e48cb1e5fbb0d037f356ffe9e9978a0/discovery/discovery_client.go#L254-L282, where the implementation is, it certainly fits the commented behavior, but since the implementation of GetVersionSet disregards the return value, I'm pretty sure this behavior doesn't work as expected.

@bacongobbler bacongobbler mentioned this pull request Nov 22, 2019
Closed
@technosophos
Copy link
Member Author

@kenperkins In our testing, the line that returned the error in question is client.VersionSet(). Are you seeing the same problem as described in #6361 appearing still?

My intention wasn't to trap all cases of that error, but just the one causing #6361

@technosophos
Copy link
Member Author

Also, if you can provide a way to reproduce the error you are seeing, or even the error message that you are seeing, that would help us track down the issue.

@cofyc
Copy link
Contributor

cofyc commented Dec 2, 2019

can this be cherry-picked into helm 2.x branches?

@cofyc cofyc mentioned this pull request Dec 2, 2019
Closed
@cofyc
Copy link
Contributor

cofyc commented Dec 2, 2019

This can fix the problem described in #6361, but another problem is it may return wrong capabilities because it fallbacks to use chartutil.DefaultVersionSet if the client returns GroupDiscoveryFailedError. If it is a problem, I guess we should build VersionSet on partial results returned by ServerGroupsAndResources if it failed to discovery some API groups.

This was referenced Dec 6, 2019
Closed
Merged
@technosophos technosophos added this to the 3.0.2 milestone Dec 17, 2019
@technosophos technosophos added the picked Indicates that a PR has been cherry-picked into the next release candidate. label Dec 17, 2019
@technosophos
Copy link
Member Author

Picked into 3.0.2

deanrock pushed a commit to celtra/helm that referenced this pull request Dec 24, 2019
@technosophos @dejanceltra
fix: stop discovery errors from halting chart rendering. (helm#6908)
ed619cb 
This blocks a particular error (caused by upstream discovery client),
printing a warning instead of failing. It's not a great solution, but is
a stop-gap until Client-Go gets fixed.

Closes helm#6361

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
@sancyx
Copy link

sancyx commented Mar 19, 2020

Any plans to pick this fix in versions 2.15/2.16?

@bacongobbler
Copy link
Member

See #6361 (comment)

@mrajashree mrajashree mentioned this pull request Apr 8, 2020
Merged
@pacoxu pacoxu mentioned this pull request Mar 14, 2022
Closed
@x3nb63 x3nb63 mentioned this pull request Jun 1, 2022
Closed
@drivebyer drivebyer mentioned this pull request Mar 7, 2023
Merged
@pmalek pmalek mentioned this pull request Jul 6, 2023
Closed
@pmalek pmalek mentioned this pull request Jul 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick left review comments

@bacongobbler bacongobbler bacongobbler approved these changes

@adamreese adamreese adamreese approved these changes

Assignees

@technosophos technosophos

Labels
picked Indicates that a PR has been cherry-picked into the next release candidate. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
3.0.2
Development

Successfully merging this pull request may close these issues.

unable to retrieve the complete list of server APIs
8 participants
@technosophos @kenperkins @cofyc @sancyx @bacongobbler @jglick @adamreese @helm-bot