Hacker News Reader

> You definitely shouldn't rely on nothing ever attempting to put a bad value into the database.

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.

I've only used Erlang in hobby projects, but it seems like a consequence of the "let it crash" philosophy being applied in an awkward situation. But I don't have the experience to know if this philosophy is something Erlang/Elixir programmers ignore in a circumstance like this, and if so how they determine when circumstances are such that "let it crash" should be ignored.

I feel like this is a misapplication of that philosophy.

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.

The sort of crash looping that took down their nodes is one of the parts of OTP that I’ve found to be both quite unintuitive and dangerous, for exactly the reasons described. There are controls for max retry attempts within a time window, but no obvious way from what I can tell to require a supervisor to halt rather than propagate a crash loop in a way that is easily auditable. This has caused me bad failures as well, and I’d be curious to hear what others are doing here. It’s not that it’s a hrs problem to solve, but that it’s an easy thing to overlook.

Max restarts is a big sharp edge. I think anybody running Erlang in production must have hit it, probably a couple times. For things that I expect to fail regularly, but have no business bringing the system down, I reflexively sleep in the init, so it would only trigger max restarts if I really messed up. If it's consistently failing, it'll log enough to be visible, but it's not like this is an ideal way to handle it.

I think it would be nice if the HTTP server could at least optionally validate that the length is expected. For example I would really not expect a malformed chunked encoding lacking a terminating zero length chunk to silently succeed.

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.

Yeah, this is reliability engineering 101 - don't ever expect that outside content won't be malformed, even from 'trusted' systems. There was a codepath somewhere that went from an unmarshal error into a crash - and that's something that should not pass any code review.

In Erlang (and Elixir) systems, turning unexpected errors into crashes is what you're supposed to do. Unfortunately, it's pretty easy for that to spiral into node shutdown/restarts, everybody ends up learning that one the hard 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.

This may be true, but malformed input that is unchecked is not, imo, an “exceptional” case but nominally expected, and kind of dangerous since it is one of those things that isn’t likely to happen often since it requires another bug or fault to reveal itself.

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.

The important idea of the Erlang approach is not to leave things unchecked; the idea is that the proper response to most errors is to restart from a known good configuration.

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.

> what are you expected to do when the service discovery system is unreachable, broken, or filled with bad data?

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)
    }

And the issue is the lack of graceful degradation, in the form of not failing the entire conversion if one entry is broken. Instead, this should be something like:

    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)
    }

Of course, this takes much more effort. But such is the nature of writing high quality, reliable software.

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.

Well, let's try to imagine if BNS pooped out a corrupt update. Would every BNS client just silently pass on it, or would they all instantly suicide? I'm honestly not sure it's the former.

I don’t understand this hypothetical perhaps because I don’t know what “BNS” is in this context. However, I think it’s reasonable to assume that if you have a key-value store and you haven’t modified it to validate your scheme, that there’s no guarantee the scheme will be valid across the database, and if your code doesn’t tolerate these conditions, eventually a bug will reveal that.

https://landing.google.com/sre/sre-book/chapters/production-...

I did not know BNS was publicly known so apologies for playing stupid.

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.

Seems it's Borg Naming Service. I guess Borg is some Google-specific thing.

While I agree that discord's code needed to be modified to handle bad data in etcd just in general ... this is not really a theoretical aspect, checking the content length so you don't try and deserialise a partial body and send crap to your business logic is http server writing 101.

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 ;)