Hacker News Reader

Comment by nine_k | Hacker News Reader

* The unwrap() in production code should have never passed code review. Damn, it should have been flagged by a linter.

* The deployment should have followed the blue/green pattern, limiting the blast radius of a bad change to a subset of nodes.

* In general, a company so much at the foundational level of internet connectivity should not follow the "move fast, break things" pattern. They did not have an overwhelming reason to hurry and take risks. This has burned a lot of trust, no matter the nature of the actual bug.

View on HN

Unless you work at Cloudflare it seems very unlikely that you have enough information about systems and tradeoffs there to make these flat assertions about what "should have" happened. Systems can do worse things than crashing in response to unexpected states. Blue/green deployment isn't always possible (eg due to constrained compute resources) or practical (perhaps requiring greatly increased complexity), and is by no means the only approach to reducing deploy risk. We don't know that any of the related code was shipped with a "move fast, break things" mindset; the most careful developers still write bugs.

Actually learning from incidents and making systems more reliable requires curiosity and a willingness to start with questions rather than mechanically applying patterns. This is standard systems-safety stuff. The sort of false confidence involved in making prescriptions from afar suggests a mindset I don't want anywhere near the operation of anything critical.

Indeed, I never worked at Cloudflare. Still I have some nebulous idea about Cloudflare, and especially their scale.

Systems can do worse things than crashing in response to unexpected states, but they can also do better to report them and terminate gracefully. Especially if the code runs on so many nodes, and the crash renders them unresponsive.

Blue/green deployment isn't always possible, but my imagination is a bit weak, and I cannot suggest a way to synchronously update so many nodes literally all over the internet. A blue/green deployment happens in large distributed systems willy-nilly. It's better when it happens in a controlled way, and the safety of a change that affects basically the entire fleet is tested under real load before applying it everywhere.

I do not even assume that any of Cloudflare's code was ever shipped with the "move fast, break things" mindset; I only posit that such a mindset is not optimal for a company in the Cloudflare's position. Their motto might rather be "move smooth, never break anything"; I suppose that most of their customers value their stability higher than their speed of releasing features, or whatnot.

Starting with questions is a very right way, I agree. My first question: why calling unwrap() might ever be a good idea in production code, and especially in some config-loading code, which, to my mind, should be resilient, and ready to handle variations in the config data gracefully? Certain mechanical patterns, like "don't hit your finger with a hammer", are best applied universally by default, with the rare exceptional cases carefully documented and explained, not the other way around.

I appreciate that this comment is much less prescriptive. I don't think I disagree with you about any general best practices here (although I do think unwrap can be fine when you can locally verify the error or nil case is unreachable but proving that to the compiler is impractical.)

* The step in front of this query created updates to policies. It should have been limited in the number of changes it would do at once (and ideally per hour and per day and so on), and if it goes over that limit, stop updating, alert and wait until explicitly unblocked. DO NOT generate invalid config and start using that invalid config, use the previous one that worked and alert.

If this happens during startup use a default one.

That would still create impact (customers and developers would not see updates propagate), but would avoid destroying the service. When it comes to outages, people need to learn to go over what happens in the case of violating an invariant and look at what gets sacrificed in those cases, to make sure the answer isn't "the whole service".

If I get to be impolite, you do this because software architects, as seems to be the case here, often choose "crash and destroy the service" when their invariants are violated instead of "stop doing shit and alert" when faced with an unknown problem, or a problem they can't deal with.

This also requires test-crashing. You introduce an assert? Great! The more the merrier, seriously, you should have lots of them. BUT you will be including a test that the world doesn't end when your assert is hit.

unwrap() and the family of methods like it are a Rust anti-pattern from the early days of Rust. It dates back to before many of the modern error-handling and safety-conscious features of the language and type system.

Rust is being pulled in so many different directions from new users that the language perhaps never originally intended. Some engineers will be fine with panicky behavior, but a lot of others want to be able to statically guarantee most panics (outside of perhaps memory allocation failures) cannot occur.

We need more than just a linter on this. A new language feature that poisons, marks, or annotates methods that can potentially panic (for reasons other than allocation) would be amazing. If you then call a method that can panic, you'll have to mark your own method as potentially panicky. The ideal future would be that in time, as more standard library and 3rd party library code adopts this, we can then statically assert our code cannot possibly panic.

As it stands, I'm pretty mortified that some transitive dependency might use unwrap() deep in its internals.

> unwrap() and the family of methods like it are a Rust anti-pattern from the early days of Rust. It dates back to before many of the modern error-handling and safety-conscious features of the language and type system.

I think your argument would be more effective if you dropped this angle.

Unwrap is assert. No more, no less. It's tremendously useful to have it in the language for situations when the cost of encoding some invariant in your program's types is far larger than the benefit you'd gain from doing so. It's not some vestigial thing from way back before anyone received enlightenment that they could use sum types to discriminate errors. It's just a different tool.

I completely agree there are systems and situations where you want to be able to statically verify an absence of panics, but the way you're describing the situation sounds similar to when I hear folks describe anything that came before as "legacy" with a denigrating inflection.

Does #[no_panic] do it for you? https://docs.rs/no-panic/latest/no_panic/

> As it stands, I'm pretty mortified that some transitive dependency might use unwrap() deep in its internals.

You'll have to go without std and even the `core` library then.

This is starting to sound a lot like checked exceptions in Java.

Checked exceptions were a great idea which are still, to this day, unfairly maligned.

I don't necessarily disagree, I wasn't making a judgement positively or negatively.

The scale of the outage was so big and global, that the biggest failure was indeed the blast radius.

I wish they do burn a lot of trust to show up in their financial reports. Otherwise it is like "we do not like it but gonna use it anyway".

> the blue/green pattern

This specific terminology was new to me, too: https://en.wikipedia.org/wiki/Blue%E2%80%93green_deployment

Reply on news.ycombinator.com