- Notifications
You must be signed in to change notification settings - Fork563
refactor: kubernetes unit network info#20909
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
We had an implementation for getting the kubernetes info for a unit thatwas suboptimal.It had incorrect joins, ignored multip ip addresses and was trying tosolve the world in a single query.This commit breaks the problem up and demonstrates the correctimplementation of the state part.
This commit refactors the service call for getting a units kubernetesinformation. This was done so that we can correctly return the rightinformation to the caller and avoid needless bugs in the future.While here I updated the return struct to not use network provider id.The provider id value for a unit is not a network value in this contextand the implication should not be made.The service now relies on the ordering returned by state. It would bebetter if this wasn't the case but it will do for now.
SimoneDutto 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.
looks good.
I saw in the description you said that this implementation was broken.
Why was it broken? It was returning something unexpected, panicking? Or was just missing the ordering per scopeId? I'm curious.
| WHERE u.life_id != $lifeID.life_id | ||
| ` | ||
| unitInfoStmt,err:=st.Prepare(k8sUnitInfoQuery,unitK8s{},lifeInput) |
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 see here we have 3 queries instead of one and then we merge the info in the code.
Is this something we generally do or just because it's handier in this case?
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.
Because i guess here you could do a single query and returning the final result. I prefer a single query.
| LifeID:int(life.Dead), | ||
| } | ||
| lifeInput:=lifeID{LifeID:life.Dead} | ||
| scopeSelector:=ipAddressScopeID{ScopeID:int(domainipaddress.ScopeUnknown)} |
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.
ah, i see what i did wrong, i didn't know of thisScopeId ordering.
| returns.st.GetUnitK8sPodInfo(ctx,name) | ||
| } | ||
| // GetUnitsK8sPodInfo returns information about the k8s pods for all alive units. |
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.
do you think it's necessary to say here that we pick the "most public" address out of the returned?
SimonRichardson 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.
Happy to go with this.
This PR tries to fix a bad implementation we got for fetching a units Kubernetes network information. There were a few possibilities that existed with the implementation and which were not tested. While this change is still not perfect the code is now much safer and more correct with the data model we have.
In an idea world we would have got the network info for a unit in one call no matter what type the model is. There is nothing kubernetes specific about this call that couldn't have just been behind a generic service func. To deal with later.
Checklist
[ ]Integration tests, with comments saying what you're testing[ ]doc.go added or updated in changed packagesQA steps
Unit tests are fine.
Documentation changes
N/A
Links
Jira card: Drive by