-
Notifications
You must be signed in to change notification settings - Fork 270
openstack: handle when metadata service returns empty data #2177
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
Conversation
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.
Code Review
This pull request correctly addresses an issue where empty metadata from the OpenStack service was not handled, leading to errors. The fix is straightforward and effective, adding a check for errors.ErrEmpty. The inclusion of a new test case to cover this scenario is a great addition. I have one suggestion to further improve the robustness of the error handling.
|
So I dont think this is the correct way to handle our issue unfortunately. We will need release notes under fixes or bug fixes as we are fixing a bug in an older release. Regarding the contents of the changes. An easy way to remove the whole unmarshaling thing and pull the thread of what needs to change would be to remove the "encoding/json" import. That should highlight the unmarshaling code.
The former needs to return unaltered data and the latter needs a signature change "return []byte" and also ensure unaltered data. |
96f4dd2 to
12f142f
Compare
…g/json Remove encoding/json, changed fetchConfigFromMetadataServiceIPv4Only and fetchConfigParallel now returning raw data
12f142f to
d0c5d54
Compare
|
@prestist Thanks for all information. I update the code to return raw bytes from fetchConfigParallel and fetchConfigFromMetadataServiceIPv4Only with no marshalling. Now we are not using FetchConfigDualStack because it returns a parsed types.Config anymore. I guess this is enough to fix the bug, but I need to test it manually and run the kola tests locally to be sure. Can you take a look and see if this new changes makes sense? |
Sure taking a look now :) |
prestist
left a comment
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 is about what I was expecting, the main thing is to ensure dual stack still works (it should not really a big diffrence here) and then verify it fixed the kola test that was failing.
Overall LGTM, thank you for fixing this so quick!
Once you have tested it I will approve :)
|
@marmijo and @yasminvalim verified the efficacy of these changes and we can land them. |
The fix removes the marshalli, now the code return raw bytes directly instead of marshalling parsed configs.