Code Review in Kubernetes

1. Code Review in Kubernetes Oct, 2022 Tim Hockin <thockin@google.com> @thockin (c) Google LLC
2. Kubernetes has become a phenomenon • One of the most active OSS projects EVAR! • Hundreds of repos on github • Contributors from every timezone and almost every continent (Antarcticans - if you’re out there, holla!) We receive scores to hundreds of Pull Requests per day Every single PR needs someone to review it! Why do I care?
3. Good code review is HARD
4. Read the associated bugs or KEP(s) • We really do not want major changes without “doing the homework” • KEPs are how we think through the hard problems • There are very few non-hard problems left Understand WHY the change is being made • ...and why it is being made this particular way Before starting
5. Read and understand the change Understand the context: the surrounding code and adjacent features matter a lot Think about: • Test cases • APIs • Comments and docs • Maintainability over time Start the hard work
6. Every change makes the code either better or worse
7. Is the bug cited? Is the bug real and worth fixing? Does this actually fix it? Does this prove that it fixes it (tests, benchmarks)? Is this the “right” fix? If not - it’s OK to push back Motivation: Bug fix
8. Do we really want the feature? How does it interact with other features? Is the community (SIGs) OK with the design? Where’s the KEP? Motivation: Feature
9. Is this worth cleaning up? Is this actually cleaner? Does this make future changes or other PRs harder? If not - it’s OK to say “no, thanks” Motivation: Cleanup
10. Is it the simplest way to express the solution? Is the code as clear as possible? Do these changes fit the mission of the context? Can the change be broken down any further? • Smaller, focused PRs are better Is it a complete solution (don’t merge half-complete)? Is it exposing details that callers don’t need to know? • Can it be pre-factored to reduce scope? Clarity, cohesion, scope
11. If code does ANYTHING non-obvious, it needs to explain WHY Does the code handle errors? Think about corner-cases and “pathological” situations • Think antagonistically - how will someone abuse this? Does it fail gracefully? • Will it retry? Is this clear in the code and comments? • How will the user know something failed? Comments and errors
12. Does it log significant actions and errors? • Are the logs useful and/or actionable or spammy? If the PR involves config fields, APIs, CLIs, or flags: • Think about how they will be used: do they feel natural? • Are they consistent with other instances? • Would they impede or confuse future work? • Principle Of Least Surprise • If we were starting it all over, what we would want? Observability and UX
13. Almost all code changes require test changes • Any code PR without tests is an immediate smell • Even if tests didn’t exist before, we need to do better Many PRs require knowledge of the context • What has been done in the past • Why things were done the way they were • How other features will interact with this change • What we want to do in the future Testing and context
14. If you need context you do not have: GO GET IT! • Go exploring - software archaeology is fun • git grep is your friend • Do experiments or patch in the change and try it • Ask for a consult • Ask PR author to document the results in comments • links to previous issues / changes / discussions are VERY helpful for future others I’ve seen too many PRs approved without sufficient context that need to be fixed later • I have been guilty of this, too Domain specific knowledge
15. If you don’t know how a change interacts with other code or features, or how it fits into the subsystem: DO NOT APPROVE! Domain specific knowledge
16. Like refactoring, but done BEFORE making changes • Arranges code to better accommodate a future change • Reduces the size and scope of the real change • Should be no-op code changes, plus tests Prefactoring is not just acceptable - encourage it! • Any significant change will almost certainly require it • Can be separate PRs or commits in a larger PR Prefactoring
17. PRs are not really represented in git history, but commits are the permanent record! PR comments should summarize • Explain what the PR is about and why • Link to relevant docs (e.g. KEPs) and bugs Commit comments should be thorough and detailed • Enough for git log to be useful • WHY > HOW PR and commit comments
18. Process
19. DO: Ask questions and seek to understand • “Am I following this correctly?” • “Why was this choice made?” • If you send every comment you write, you are probably doing it wrong DO: Explain your thinking • “If X is true, then Y can’t be true, but this doesn’t handle that case” Communication
20. DO: Be clear about nits vs. requirements vs. questions • Nit: A small thing that could be improved • Required: “I will not /lgtm until you fix this” DON’T: Pick a bunch of nits if the real problem is that the PR has the wrong approach • Architectural guidance should be first priority Requesting changes
21. DO: Indicate progress • Be clear about whether you’re finished reviewing or not, or what state you think the PR is in overall • It’s frustrating to get batch after batch of comments when you thought you were done DON’T: Approve because of deadlines or guilt • There’s always another release • Again: There’s. Always. Another. Release. Progress
22. DON’T: Be timid about asking for changes • You are the defenders of this codebase • THIS IS YOUR JOB (for the moment) DO: Be reasonable • They won’t do everything exactly how you would have DO: Be willing to be wrong; trust, but verify DO: Be willing to acquiesce on reasonable debate Discussion
23. Critique CODE not PEOPLE
24. DO NOT BE A RUBBER STAMP I’d rather we slow the whole project down than merge garbage
25. TO PR AUTHORS Code review time is exponential to the PR size Smaller PRs have many more chances to schedule!