Discord outage postmortem
discord.statuspage.io
discord.statuspage.io
Whichever engineer wrote puts too much weight on the theoretical aspects of HTTP.
The reality is that any of Discord's software could have had erroneous code that announced an empty service list. This would have crashed all of Discord regardless -- transient network errors and mismatched HTTP Content-Length headers need not apply.
A resilient HTTP server will try to handle whatever the HTTP client sends it, and then the specific endpoint can enforce whatever levels of strictness are needed by the application. It's nice to be able to point a finger at "not my code", but how many popular web server libraries (not standalone web servers like nginx or apache) out there will actually flat out reject the entire request because of a simple Content-Length mismatch, rather than handing that decision off to the endpoint?
If an empty string is not a valid value for your service discovery system, you should reject those from the database with a CHECK CONSTRAINT if possible, or with the code that sits in front of the database if CONSTRAINTS don't exist (like I believe is the case for etcd). It's also not a bad idea for the clients to validate what they receive and filter out values they can't handle to enhance robustness.
You definitely shouldn't rely on nothing ever attempting to put a bad value into the database.
The web server's willingness to accept "strictly" malformed requests when the engineers were unaware of this fact was certainly a contributing factor, but I don't think it was the root cause in this case, as they're heavily trying to suggest.
(I think it would be neat to have a "strict" mode that you could turn on for every web server, especially when being used as an internally-facing service, but most companies want to make it as easy as possible for their customers/partners to integrate with their service... and that means accepting requests that aren't always academically perfect.)
It's a deeper problem than that because etcd doesn't know what Dicord considers as a "right" value, etcd stores whatever you send to it, empty values etc ...
The question is why a single bad formated JSON object created an entire fleet of healthy nodes to fail. There is some JSON parsing somewhere that went really wrong.
Though my first exposure to (serious) exception handling was through the common lisp signal/restart system. So I view software has layers and the communication of errors up and down those layers. Rust for example provides this through Result types and either composing or implementing error types.
My point being that the parsing of a record of a service discovery system should crash just that parser process. Not the whole system of processes, the manager of those parser processes should note (log) that the parser crashed on a record and then ignore it (probably as part of a broader refresh system I would imagine). That would be the correct way to handle errors up and down because the point of a process is to do a computation and communicate through reliable channels. Crashing can be a reliable way to communicate (it is communication of an unreliable state, but the point of letting it crash is that the resulting crash can be seen as a reliable communication) and the manager of the parsing process should have had a case for it that wasn't just crash.
However, I think you are dead on the money. Etcd is not validating the values but Discord’s software seems to assume the etcd values must be valid. That seems like a recipe for future outages if the mentality were kept this way.
Supervision trees really do need to be considered, but what are you expected to do when the service discovery system is unreachable, broken, or filled with bad data? There's certainly options, giving up isn't necessarily the wrong one.
Different things based on which one of these actually happened!
I would assume the code that caused this issue was along the lines of:
var addresses []string{}
for k, el := range etcdEntries {
var element data.Service
addr, err := json.Unmarshal(&element, el)
if err != nil {
return fmt.Errorf("could not unmarshal service entry %q: %w", k, err)
}
addresses = append(addresses, addr)
}
var addresses []string{}
for k, el := range etcdEntries {
var element data.Service
addr, err := json.Unmarshal(&element, el)
if err != nil {
metrics.ServiceEntryBroken("key", k).Up()
glog.Errorf("Could not unmarshal service entry %q: %v", k err)
continue
}
addresses = append(addresses, addr)
}
Naturally, if your entire SD service was down, you would fail the entire thing much earlier. That is indeed the thing about Go errors being plain variables - you are intended to write error handling logic (or at least are forced to consider it), not just pack the buck down by default, via either optional types or exceptions.
It’s probably an unpopular opinion at this point but I definitely appreciate Go’s approach to error handling here. Error cases are in-your-face by default for the most part, leaving nearly only programming errors and critical faults to panic ideally.
Of course, restarting the service discovery client may not help, especially when the service discovery system's state is stable but broken. I don't know what you're supposed to do if you're dependent on services, and the service discovery system you use to find the services is not reachable or is returning garbage information. Better logging is always nice, but you can't serve the traffic.
Of course BNS is at least designed for this specific use case. So this particular error, which imo is schema validation related, is not likely to occur.
I am sure some seasoned Google SREs have a good grasp on what would happen if BNS was spitting out junk, but my guess is it would at least cause outages of some kind if not crashloops.
Discord should've been more defensive.
The net/http devs should file this under "mistakes to admit to over beer to cheer up newbies who've just done something really dumb." (I have a number of such mistakes under my belt, we all make mistakes this stupid eventually ;)
Do you think so? I checked the Go http server code and my not-so-careful reading seems to confirm their suspicions: it just reads from the body io.Reader, which doesn't seem to validate length. (They DO use a limit reader to prevent DoSing with huge bodies, at least.)
Now if they were phoning out to a remote server, I’d assume some kind of fabric or load balancer. But I assume they’ve got a local etcd on each node or something along those lines so it is very possible that it’s all local networking.
Though in most cases, outside of this use case, I’d agree that this is not particularly useful because it would never be terribly reliable. I know for a fact Amazon ELB rewrites the entire request, as an example...
The implementer can always compare the read bytes to the content-length themselves. I have a hard time believing without evidence that etcd's implementation is wrong at this stage..
It seems like TCP fragmentation would be handled at a much lower level in the Python networking stack.
a) write the whole request in a single write call like MarkSweep said.
b) use tcp socket options to change behavior of multiple writes, TCP_NOPUSH on BSD, or TCP_CORK on Linux.
Option a is preferable in this case because it's a lot fewer system calls; although it's a once a minute job, so system calls on the client side don't really matter.
Is it a good idea? In this case, certainly --- if the whole request is small enough to fit in the network MTU, it avoids a protocol error on the server side.
In the more general case of a HTTP request where the content is known and in memory at the time of header generation, yes, I would say it's more efficient to send the content with a single context switch (buffer space permitting) and fewer network packets. With a caveat to be aware of, that if the larger network packets trigger MTU problems, the experience will be negative.
If the content size is known, but the content isn't loaded into memory, such as when you're uploading a file, so fstat gave you the size, but you haven't read it yet, sending off just the headers with content-size is probably better --- if the content takes time to load, you'll get the request to the server to validate it sooner than if you waited for a full packet, and that improves processing time in case the server rejects the request, or has to do something time consuming before reading the content.
If the content size is unknown, so presumably the content hasn't been chosen or generated yet, sending headers soon is usually better for a similar reason.
Clearly, the etcd server should be changed to properly process HTTP requests that arrive over multiple TCP segments, but it was likely more expedient to "fix" the client as part of the rapid response. But, this client change could still be useful, as it should reduce packet processing load on the etcd server, and the various network equipment. Probably not a big impact, unless there's a ton of clients, but similar changes where the usage is higher can make a big difference.
They were relying on the server side to verify the http Content-Length header to avoid partial writes, which never happened and they consider this to be a bug in the golang http handler.
Quoted from the writeup: """ We determined that the connection was reset after sending the first packet, but before the second packet could be sent. """
My bet is when they say "packet" here they really mean write/send. They describe the old version as doing one write for the http headers and one for the body. The new version makes just one write.
Assuming an mtu of 1500, ~40 byte TCP header, and a little bit of HTTP/1.1 headers, that leaves probably plenty of room for the etcd PUT to not get fragmented, I think.
Edit: and it would probably break DNS as well.
Notice from the write up that they already also modified their code to be resilient against the bug being hit at all, so "reducing the number of times the resilience code needs to be invoked by 95%" on top of that is a net win.
Everything runs much smoother, voice chat is like with TeamSpeak back in the days. Solid product.
I've had this with Boost.Serialization. Not fun. No exception, just a crash.
A daily reminder that one can do anything and everything in JavaScript, except display information in UTC.
Wait, no, Discord just aped that word to gaslight users into behaving in a way which makes them more money.
Also, Discord internally calls them guilds.
I'd wager that most of Discord's clientele don't have any specific expectations for the word "server". Regardless, I think "server" was carried over from the Mumble / Ventrilo / Teamspeak gaming community.
It is a pure marketing ploy. Internally they call the "server" thing a "guild" AFAIK, which is technically just as much BS as the "server" moniker, but for other reasons (not every community using Discord is a "guild" in the commonly accepted meaning of that word in the gaming world, which is a long-term organizational group doing stuff together in an MMORPG).
People on the internet really love abusing this word, especially Twitter.
Lately, I've seen "he's gaslighting the world!" used in place of a public lie.
Internally they name them "guilds" or something.