Code Review in Kubernetes
Oct, 2022 Tim Hockin
(c) Google LLC
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?
Good code review is
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
Read and understand the change
Understand the context: the surrounding code and
adjacent features matter a lot
• Test cases
• Comments and docs
• Maintainability over time
Start the hard work
makes the code
better or worse
Is the bug cited?
Is the bug real and worth ﬁxing?
Does this actually ﬁx it?
Does this prove that it ﬁxes it (tests, benchmarks)?
Is this the “right” ﬁx?
If not - it’s OK to push back
Motivation: Bug ﬁx
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?
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”
Is it the simplest way to express the solution?
Is the code as clear as possible?
Do these changes ﬁt 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
If code does ANYTHING non-obvious, it needs to
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
Does it log signiﬁcant actions and errors?
• Are the logs useful and/or actionable or spammy?
If the PR involves conﬁg ﬁelds, APIs, CLIs, or ﬂags:
• 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
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
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 suﬃcient
context that need to be ﬁxed later
• I have been guilty of this, too
Domain speciﬁc knowledge
If you don’t know how a change
interacts with other code or features, or
how it ﬁts into the subsystem:
DO NOT APPROVE!
Domain speciﬁc knowledge
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 signiﬁcant change will almost certainly require it
• Can be separate PRs or commits in a larger PR
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
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”
DO: Be clear about nits vs. requirements vs. questions
• Nit: A small thing that could be improved
• Required: “I will not /lgtm until you ﬁx this”
DON’T: Pick a bunch of nits if the real problem is that
the PR has the wrong approach
• Architectural guidance should be ﬁrst priority
DO: Indicate progress
• Be clear about whether you’re ﬁnished 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.
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
Critique CODE not
DO NOT BE A
I’d rather we slow the whole
project down than merge
TO PR AUTHORS
Code review time is
exponential to the PR size
Smaller PRs have many more
chances to schedule!