Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Code Review in Kubernetes

Tim Hockin
October 12, 2022

Code Review in Kubernetes

Tim Hockin

October 12, 2022
Tweet

More Decks by Tim Hockin

Other Decks in Technology

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