Code Review in Kubernetes

Transcript

1. Code Review in Kubernetes
   Oct, 2022 Tim Hockin
   @thockin
   (c) Google LLC
   

   View Slide

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?
   

   View Slide

3. Good code review is
   HARD
   

   View Slide

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
   

   View Slide

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
   

   View Slide

6. Every change
   makes the code
   either
   better or worse
   

   View Slide

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
   

   View Slide

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
   

   View Slide

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
   

   View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

18. Process
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

23. Critique CODE not
    PEOPLE
    

    View Slide

24. DO NOT BE A
    RUBBER STAMP
    I’d rather we slow the whole
    project down than merge
    garbage
    

    View Slide

25. TO PR AUTHORS
    Code review time is
    exponential to the PR size
    Smaller PRs have many more
    chances to schedule!
    

    View Slide

26. View Slide