Soruce code for Persistent Volume implementation within Kubernetes. See lines 55-56 for the context on why the code is written in a very verbose / space shuttle style.
Stories with similar links:
- "Space shuttle style" programming via siddhantgoel 4 years ago | 32 points | 30 comments
It has a grandiose claim and and tries to attach itself to a well-respected coding standard, but it smells like a post-hoc justification for the unpalatable state of the code.
The code looks like a state machine. And a state machine can be written either as a spaghetti code of
ifs, with omg-space-shuttle-will-crash-if-you-forget-an-elsefear, or as a table with state transitions, which by construction ensures everything is accounted for.I’ve the feeling that most developers (with a CS degree) forgot about this. And on the other side, there’s also a big component of lack of education on the topic. I’m not sure how many common programmers have been instructed or invested time learning about state machines.
That said, it is definitely a good mentoring/training topic. I think it will be well received by my team, and in any case should start circulating the knowledge more. Does anyone have good resources on this?
I read this and thought, well, could you unit test this code to ensure correctness? I know unit testing threading behavior is tough, but if this is space shuttle levels of risk, might that effort be worth it?
Perhaps. Perhaps you just crashed the ship.
Nope. Not buying it. This is cheesy schtick covering up some very questionable coding practices.
I am a late stage beginning programmer struggling towards journeyman, and even I must ask “Why not AT LEAST use methods to collapse some of these 10 level deep conditional nests?”.
Good software engineering practice strives to keep code easy to reason about and thus more readable and maintainable. As much as we all love to be entertained by seeing HERE BE DRAGONS in source code, nobody actually thinks this is a GOOD idea.
This is an invitation to deviation of normalcy, and I can’t see any good at all coming out of it.
I think the received wisdom about small functions and methods has gotten somewhat muddled. The small functions style has become an aesthetic preference (which I adopted and still observe in myself) that is applied arbitrarily without any objective understanding of its effects.
For things that are actually functions in the mathematical sense (i.e., pure functions) a granular separation of functionality simplifies testing and composition. But procedures that mutate state or coordinate multiple stateful systems are not testable and composable in the same way. In this context, the small functions/methods style is actually an obstacle to understanding the system and ensuring correctness.
See: http://number-none.com/blow/john_carmack_on_inlined_code.html
Again, I am but an acolyte, but from my super simplistic perspective, having 8 levels of conditional nesting makes code SUPER hard to reason about, and when you break that into methods that signal the intent of the code contained within you increase readability.
I guess I’d thought of that as beyond argument. I’ll read the Carmack article, thanks for the link.
Yeah that’s definitely the accepted dogma but I’ve observed the opposite in large systems I’ve worked on (although it took a while for me to see it). If you look at game engines, which do some of the most complex coordination of stateful systems anywhere, you will see the large procedure/nested conditional style. This doesn’t come from an ignorance of the ability to make small methods.
The intent communicated by factoring code into small methods is that these calls can be rearranged and reused at will, but for stateful calls this most often isn’t true.
I can also imagine that in game engines simply eating the overhead induced by a method call (stack, heap, etc.) could be problematic.
Lesson for me here is that there are almost no hard and fast rules where code is concerned, but I still think that for the class of very non computationally intensive process control (Devops) work I do, having small, readable, clearly named methods rather than giant nesting structures is a best practice I’ll stand by.
I think it’s a mistake to think of it in terms of performance optimization. From the above article:
Also super interesting how people throw around down votes like candy.
How can my failure to buy into the argument being purveyed by the author possible be incorrect ?
I’d bet a dollar that everybody who says this is too complicated/messy and tries to simplify it will end up having a bunch of bugs. I’d bet a second dollar that fixing those bugs leads to an equally complicated/messy file in the “simplified” style.
Sometimes things are just hard.
Hey Hillel,
I’ve watched a few of your videos and am a fan of your work. Tbh I was surprised to see you – as a best-practices guy, a property-based testing advocate, and author of a book on TLA+ – defending this code. Just looking at function like this:
https://github.com/kubernetes/kubernetes/blob/ec2e767e59395376fa191d7c56a74f53936b7653/pkg/controller/volume/persistentvolume/pv_controller.go#L320
… ensuring that’s correct by hand seems like a fraught endeavor. You are asking that a reader painstakingly turn himself into an interpreter, concentrate really hard, and hope all goes well. Sure, after the fact of having written it, had multiple humans slowly verify it, and presumably test it as thoroughly as possible, I can see the argument for “it works, just back away slowly, don’t touch it.”
But it seems to me something has gone terribly wrong in the process if this is where we’ve found ourselves.
First of all, thank you! I’m glad you enjoy my work :D
So I can see two ways of defending this. The first is not to defend the code, but the team. Right now we’re all outsiders looking in. Most of us aren’t domain experts. Most of us haven’t read through the entire file to see how it all fits together. It might seem huge and ugly, but if I read the whole thing, I might decide that it’s actually the best solution. Or maybe I’d think there was a better way, and discover it used to be the “simple” way before and they deliberately changed it into the current style. Until I’ve put in the effort to understand it, I don’t want to pass judgement on it. I don’t trust my self enough to do that.
Second… what if this actually is decent code? Not saying it is, just exploring that idea, but it seems feasible. What’s the control flow for the function you linked?
I was able to see that control flow really quickly. I spent more time figuring out how to represent it here (I’d prefer a flowchart or decision table) than figuring out the flow itself. What would be a better way to represent this control flow? I guess you could try to bind anyway and raise a typed exception if it fails, but go doesn’t support that, and it would break the logic locality you have here.
Thanks for the detailed reply.
First, I agree with the claim you cannot confidently pass judgement without more context. Or without knowing what their priorities are, etc. So I grant you it’s possible that, knowing more, I’d say, “This is a decent solution, all things considered.”
But I think it’s unlikely.
The summary that you have is exactly what I want to see in code. The fundamental problem is, in order to have confidence in this code, I have to know what it does, completely, but I cannot see that directly in the code. I have to first perform the translation that you performed above. As does every reader of the code.
And how confident are you that it’s correct?
Could you have missed something? It’s making calls to other functions and methods, ie, stuff like
ctrl.volumes.findBestMatchForClaim(claim, delayBinding). Do those have side effects? How will you verify this?And remember that you’ll need to follow the same process for every function here. So you’re looking at a whole bunch OR’ed probabilities of possible mistakes.
In all the cases it seems the answer is: be very careful, read it multiple times, and follow every function and method call outward as far as the path leads you and read all that code too.
I recall noticing you were a fan of Nancy Leveson when you looked up your web page (because I am as well)…. I bring that up because we seem to have a lot of evidence that telling human beings to be very careful and disciplined is never a good strategy for safety.
I found this out by skimming all of the comments in the code. I also double checked they lined up, which was pretty quick.
Isn’t that the case of all code? This is complicated, and you have to go over it carefully, but I don’t think there’s a natural way to rewrite this so that it didn’t require that. Usually, when people bring up long functions, they talk about breaking it up into many smaller functions. In this case, doing that would mean you’d have to jump around all of the smaller functions and verify they were all correct, too.
I don’t think they’re just relying on discipline here. The code comes with a lot of unit and E2E tests, you just can see it from this file. I mean, I’d love to see a formal model or generative testing, too, but I don’t think that’s directly related to the code.
This is true, but at least it makes the high-level logic clear. And generally verifying the two things are separate tasks: 1. verify the high-level logic is correct 2. verify that each step, independently, is correct. It also forces you (or at least encourages you) to break it down in such a way that there is no interconnection between the parts that shouldn’t be there (you have to verify this manually in a long function where local variables essentially become “global-like” within the context of the function). This is much easier to verify if each part is a stateless function, or at least a function whose only side-effectful dependencies are clear from the arguments.
But I’m not necessarily even saying that breaking it down in that way would improve things here. I wouldn’t want to make that claim without really delving into the code. I’m just very suspicious of the claim that “this is the clearest you could possibly write this code.”
The position staked out by the comment is essentially: “Hey, you think you’re clever, well you’re not. Smart people tried simplifying this and we couldn’t, so trust us, this is the best it gets here.”
While you’re probably correct that simplifying it is harder than the haters think it is, I’d also be pretty shocked if it was actually impossible to significantly improve the readability of this code (while still keeping it correct, ofc).
I think that’s all totally fair! But I think the illegibility of one long function vs many short ones is driven by us not writing good tooling to support the former case. Like if I could easily flip between a flowchart view and the code view, or have free floating comments, it would be much easier to understand long functions. But that’s neither here nor there.
100% agree with everything here.
Also totally fair, and something I’ve thought about too. I think you could even say that “breaking down longer functions into smaller ones is just our best current hack for getting both the high-level view and low-level view of things.”
The essential act of programming is transforming oneself into an interpreter.
Better that we write programs with this essence in mind.
Funnily enough, I’ve recently watched one of your videos as well, the industrial programming one, and enjoyed that too.
Re: your claim above, it’s hard to tell exactly what you’re arguing, but it seems to be a claim that a strict, explicit procedural paradigm is the best way to program. If that is the claim, I disagree.
If we’re just getting philosophical, I’d have the opposite take. I want the program to service my way of thinking rather than the other way around. Step by step interpretation of instructions is not what we excel at. Of course, trivially, what we program eventually becomes steps for the computer to execute on hardware, but if that’s the argument then why not program in binary directly?
EDIT: also, hey, i just checked your about me… and I quote :)
Yes, and, funnily enough, I consider the OP’s style of explicit and verbose imperativism to be a great example of exactly what I mean by that quote.
I don’t really agree with this. Programs are much more similar to recipes than art or mathematical expressions. When I’m reading a recipe, I want step-by-step direction.
The quote is Abelson, from SICP, and I’m fairly confident that’s not how he meant it. He is no fan of “verbose imperitivism” (great term, btw). Just a couple of anti-imperative tidbits from the book (separate quotes, not a single sequence of paragraphs):
Even when he discusses the essential “step by step” nature of programming, he points out that the purpose of high-level languages is to subordinate detail appropriately and “free the user” from precisely the kind of concerns that “verbose imperitivsim” insists that you deal with at every step:
I guess I would say that, in this example, the “high level language” of the controller’s API, and specifically the syncUnboundClaim method we’re all discussing, is precisely that atom which “provide[s] … a substantial amount of methodological knowledge that frees the user from concern with numerous details of how [the] specified computation will progress.”
Or, maybe I can frame this discussion a slightly different way. If you believe that this method is (waves hands) bad, I think the implication is that you believe it is introducing unnecessary/accidental complexity in order to implement the business goal. But I don’t think that’s true. I think the complexity of this method as it exists on the page is pretty much symmetric with the necessary/true complexity of the business goal it’s implementing. Which is to say the business goal is actually, truly, properly expressed as an imperative recipe, a flow chart, and not a state machine. When Kubernetes maintainers are talking about how this thing should work around a conference table, they are probably using language equivalent to the words in the comments, and probably not drawing out state transitions. (Especially when considering the churn of maintainers this code has seen, and will continue to see!)
That’s rigged to fail, though. The kinds of Lobsters that are capable of pulling it off are also smart enough to reject slave wages for such a dirty job. :)
I don’t think pay is the primary incentive for most open source contributions.
I’m kidding around…
Link to previous discussion
Top rated comment in that:
IIRC brute-force “if then” statements without any loops or complex conditionals is exactly how NASA codes.
EDIT: Here’s one of their papers on the subject.
Supporting your claim, this article shows they throw lots of labor, documentation, coding guidelines, and code review at the program. They also keep the same people that understand it vs the churn common in most organizations.
You can link directly to the lines you want on GitHub.
Or even link to multiple lines.
I thought this was satire. Kubernetes is a bloated mess. What does it help when the space shuttle is made out of styrofoam and duct tape?
I have been writing in a very verbose / space shuttle style for years. I find it helps immensely when working with teams and/or when revisiting code in six months time. On average LOC will be about 50% of my files lines with the rest consisting of documentation in the form of comments that make no assumption of the readers experience with the codebase as a whole.
Thank you for this.
I have a job that occasionally requires I review codebases I’ve never interacted with before; in my free time, I’m often trying to parse through ML code that’s dense with complicated and unexplained one-liners, single-letter variable names, and the like. I get that working with these is a necessary skill in most code-related industries, but I deeply appreciate any code base that’s clearly designed with a new reader in mind because it means I can quickly solve the problem I’m trying to solve, instead of spending a week steeping in the local culture’s traditions before I understand where I am and what I’m doing.
Yet, the Space Shuttle is grounded.
Some of this is just limitations of languages like Go. I like how in Scala you can compress huge series of if/switch/conditions using monads; collecting all your errors/failure conditions at one place or returning the successful thing.
Scala has a bunch of other wars, and library writers with their terrible DSLs and implicites everywhere can go die in a fire, but this is one thing that language gets right; and which I’d like to see in more languages.
Writing 100% Go 100% of the time these days, this doesn’t honestly look that unusual/bad outside of the two Bound/Unbound functions.
This is the key part of the comment. Recording business context is extremely important and highly underrated. For a codebase to be maintainable, someone has to be able to answer not just “how does this work?” but also “why does it work this way?”. Cleaner code helps with “how”, but isn’t always worth it at the cost of obscuring “why”.
Let’s say this could be refactored as a state machine. Does that match the semantic model of the thing we’re trying to do? Are you sure? What about in 12 months? If it doesn’t match the semantic model, will that structural choice obscure the answer to “why”? What if other knowledge is lost through people leaving the project or forgetting? It seems to me that there are many plausible scenarios where the verbose style is the more appropriate choice.
Well, I don’t think there is a lot of problems with this, apart from the flood of comments on the code. On that regard, it might be more interesting to consider using literate programming at once. Even Emacs has great support for these things; the code is tangled into a bloat-free file, and the whole document can also be exported to PDF, printed, and/or kept as documentation.
I wish, I wish! Cue a musical where we sing about the joys of literate programming in the real world.
Do any businesses successfully integrate literate programming style?
One problem with it, which becomes apparent in most large scale literate programs, is that boilerplate ends up needing a literate justification. It’s hard to explain what I mean… basically, if you read that one famous literate programming book that implements a C compiler, you’ll notice a lot of sections that are necessarily repetitive because that’s how the program needs to be written. And in that context, having English prose for every piece of source code ends up adding complexity.
It might be a good idea overall, but I’d be interested in any examples of companies using it successfully in practice. In a team setting, it requires devs be both excellent writers and excellent developers, which is relatively rare.
I really don’t know. But the thing is, should the idea be discarded because apparently no one else seems to be doing it successfully? I think not.
I’ve seen people using Jupyter notebooks for things like machine learning, and the notebooks would obviously include prose between code blocks, but that wasn’t like tangling the code blocks into a single file.
I completely agree, and I understand what you mean. If you really need that bunch of multiline comments on your code, common sense dictates that either something is wrong about your code, or you need better documentation. But what I meant was that if keeping the comments inside the code file is so important, then literate programming is the alternative.
Though I agree, I think the same can be said for excessive comment lines. The code itself seems busy and, if you scroll a little through it, you might notice that even syntax highlighting doesn’t help with legibility. The advantage of literate programming is that you can immediately identify where code and prose are, plus the parts are disposed in a topic-based approach, which makes browsing easier. That is not what you have in a plain code file; you’d have to Ctrl+F your way through it.
This code implicitly asks a deep question: Is there any non-trivial computable problem, whose solution is beneficial to people, that does not have a solution that is amenable to understanding through decomposition and good naming of its parts?
I continue to think that the answer is: No.
It’s interesting to me that one of the qualities that makes this codebase purportedly shuttle-esque is “Every ‘if’ statement has a matching ‘else’ (exception: simple error checks for a client API call)”. In a language like Haskell its impossible for if statements to lack a matching else, and pattern-matching is the primary way that control flow is managed anyway. Go was designed to be deliberately simple, with a definition of simplicity that involved eschewing a lot of the innovations from functional programming that make programs easier to analyze and more concise to write, and that lack really shows up in code like this. I agree that some problems are just complicated and need a lot of code to handle correctly, but there’s a lot of boilerplate around error handling that could be done much more concisely and with just as much correctness in languages with better abstractions.