fix: add guard against NodeStatus. Fixes #11102

[isubasinghe]

Fixes #11102 and also issues in the same class (such as #10285)

Motivation

Adds a guard to all NodeStatus accesses, this enforces users to check the HashMap before using a value in it.
This is needed because the HashMap will return a default initialised NodeStatus. Default initialised NodeStatus have triggered edge case bugs for our customers. This fix existed internally in Pipekit for a while now and was able to solve the customers issues with container sets.

Modifications

Fairly simple changes were added.

• panic when we are absolutely certain a NodeStatus should exist but we see an error
• bubble up errors instead of relying on default initalised NodeStatus

Verification

• Manual testing

[isubasinghe]

Simple changes, just makes error handling mandatory.

We encountered too many random failing container sets without these fixes.

Now invalid access errors are just bubbled up, instead of relying on default initialised structs.

[isubasinghe]

Blocking #11493

[Joibel]

Looks good to me.

[agilgur5]

LGTM. Simple change, but so many places needed changing; nice work getting through them!

For my own edification, did the edge case bugs primarily happen on retried workflows? Reading through this code, it seems that retried workflows are the only place where Nodes get deleted?
Please correct me if I'm wrong, just trying to understand when/how this class of bugs would happen

[isubasinghe]

@agilgur5 some areas of the code weren't checking if the NodeStatus actually existed, thanks to Go's (in)sane way of dealing with missing entries (zero initialised), we were getting valid structs.

x := make(map[string]NodeStatus)
assert x["blah"] == DefaultInitialisedNodeStatus() // function doesn't actually exist, just making a point 

This didn't just happen for retried workflows, for some reason workflows with container sets pretty much randomly failed all the time. I believe this was because some code path was falsely relying on the default initalised values.

This change now forces you to explicitly handle the case where a NodeStatus is missing.

[agilgur5]

@agilgur5 some areas of the code weren't checking if the NodeStatus actually existed, thanks to Go's (in)sane way of dealing with missing entries (zero initialised), we were getting valid structs.

This change now forces you to explicitly handle the case where a NodeStatus is missing.

Yea I understood the purpose of the PR. Another Go gotcha 😕

This didn't just happen for retried workflows, for some reason workflows with container sets pretty much randomly failed all the time. I believe this was because some code path was falsely relying on the default initalised values.

This is the part I was curious about. As I would think the only time an uninitialized value would be accessed would be if the key previously existed, i.e. it was deleted. Otherwise, I would think that the Nodes map would only grow (until it is GC'd), and so a non-existent key should never occur.
Or to put it differently, trying to understand the root cause of when this would happen. Not just that the node could not exist, but why it would not exist

[isubasinghe]

This is the part I was curious about. As I would think the only time an uninitialized value would be accessed would be if the key previously existed, i.e. it was deleted. Otherwise, I would think that the Nodes map would only grow (until it is GC'd), and so a non-existent key should never occur.
Or to put it differently, trying to understand the root cause of when this would happen. Not just that the node could not exist, but why it would not exist

This is a good question, unfortunately one I do not have a definitive answer to. I believe that the codebase is somewhat loosely consistent, code paths are allowed to fail as long as eventually they succeed. That is my educated guess anyway.
I dealt with this issue for months before realising that this was the issue. This simple change was likely the most difficult PR I opened in the codebase.

[agilgur5]

This is a good question, unfortunately one I do not have a definitive answer to.

Gotcha. Was hoping you might know. Guess we still have to watch out for the root cause somewhere (which may give this new error now at least!). If it's not just impacting retries, there might be a thread safety issue somewhere 🤔

This simple change was likely the most difficult PR I opened in the codebase.

I know that feeling all too well 😅 Like to use these types of fixes as great examples that amount of code is often not the most impactful contribution!

[agilgur5]

Jotting down some notes here as I attempted to find the root cause:

1. Searched the codebase for places where NodeStatus could have been accessed or removed (keywords: "delete" and "Status.Nodes").
2. delete indeed seems only used during retries
3. The only other place I could find where nodes are removed is in this line in compressWorkflow, which I believe is only used for node status offload.
   • Which does lead to a question if this has been reproduced without node status offload?
4. The only other potential anomaly is that some places seem to lock when modifying the nodes and others don't. I did not inspect carefully why that is though.
   • I also thought that the goroutines did not share a workflow between each other (roughly this codepath), so thought locking wouldn't be necessary. But I may have misinterpreted the code; the controller is the most complex part of the codebase after all.
   • It would be nice if the Go compiler had more advanced race condition detection (like Rust, Pony, etc). I also don't think(?) we use Go's race detector
   • Even if there were some race conditions with modifying, that should never result in a Node not existing, so those cases don't really explain it either 😕