Add internal API and omdb commands to toggle current target blueprint's enabled bit

[jgallagher]

I chose PATCH for the new endpoint; I think that's correct? Although it's the first use of PATCH in omicron AFAICT so maybe it should just be PUT?

There are a couple related-but-not-required changes as a part of this:

• omdb nexus blueprints list now has an ENA: yes/no column that is only populated for the current target
• We activate the blueprint execution bg task when setting an enabled target or changing the current target to be enabled. (The latter might be superfluous if the target was already enabled, but that's fine.)

I tried this locally against omicron-dev run-all and it all seemed to check out. If folks would be more comfortable with a test on a real system before merging I can do that; otherwise I'll test it next time I take a lap for some other reason.

Closes #5210.

[davepacheco]

For the verb: I'd just go with PUT. PATCH gets a little complicated. I think this paragraph in RFD 4 is the best summary of what I know:

RFC 5789 defines the HTTP PATCH verb for allowing clients to specify only what’s changed, rather than having to specify the entire contents of the resource. RFC 6902 defines JSON Patch, a JSON-based format for describing changes to JSON documents similar to diffs and patches for text files. Future versions of the API could support HTTP PATCH using JSON Patch if that’s deemed useful, and careful design may allow libraries to implement this atop the existing GET and PUT implementations for a resource, but we’ll defer this feature for now. Our expectation is that in most cases, clients would want to make changes to resources using Etags and HTTP conditional requests to avoid clobbering changes made by other clients. Further, in most cases it’s likely easier for clients to generate an entirely new copy of the resource than a diff from the original. As a result, there’s not much advantage to a diff-based format. (We also considered RFD 7396 (JSON Merge Patch), but it’s more restricted than RFC 6902 and wouldn’t support modifying either arrays or any object where null is meaningful.)

That reflects more hope than it seems was warranted (given the current lack of support for etags and conditional requests). But the relevant bit is that if we use PATCH, we have to decide if the format we're using is "a subset of what I would have sent for a full PUT" (which is basically RFC 7396 JSON Merge Patch) or something richer like RFC 6902 (JSON Patch). This isn't the external API, so it's not like we can't change this, but I'm reluctant to introduce something without being careful about which one we pick, checking the appropriate content-type, etc., lest we copy/paste this pattern somewhere where it would be much more disruptive to get it wrong and then have to change it. I think it would be easiest to continue punting on this question and just using PUT.

[jgallagher]

I think it would be easiest to continue punting on this question and just using PUT.

👍 done in 490e97a

[davepacheco]

I feel like it's worth a comment explaining why this needs to be different from the other code path for setting the target. I assume the reason is that that code path would fail because it verifies that the parent blueprint is the current target? Assuming that's the reason, did you consider having that succeed if the current blueprint matches the one we're trying to set?

[jgallagher]

Hmm, I thought I already addressed that (if briefly) in the doc comments stating the requirements of the target. I can expand those, but maybe you're asking for something different?

I did consider just funneling this through blueprint_target_set_current and changing its requirements, and opted not to for two reasons:

• These really are different operations, so I think it makes sense for them to have error conditions that are specific to each. In particular, I think it's useful to have a "you tried to set a new target blueprint but you can't because it already is the target" error possibility. Maybe that's wrong and we'd like it to be more idempotent? I am not sure about this.
• The InsertTargetQuery CTE is kind of hairy already. Changing its checks to allow the current blueprint matching in addition to the parent is certainly doable, but would be more work than making them separate.

[davepacheco]

That makes sense, although:

These really are different operations, so I think it makes sense for them to have error conditions that are specific to each. In particular, I think it's useful to have a "you tried to set a new target blueprint but you can't because it already is the target" error possibility. Maybe that's wrong and we'd like it to be more idempotent? I am not sure about this.

I do think it'd be a little better if it were idempotent. I raised this because as a caller it seems like it could be a little annoying that you can't just construct a BlueprintTarget matching whatever you (or the user) currently wants. You have to invoke a different function (or not invoke it) based on what changed about the struct. But given your other point about the hairy CTE (which I totally get), I don't think it makes sense to change any of this now. We can always revisit if/when we make this part of the external API -- we're not committing to much here.

The doc comment explains the restriction, but not really why. I'm just thinking something like:

// Although this function is like `blueprint_target_set_current()` in that both store the given `BlueprintTarget` into the table, the functions are distinct because the preconditions and error cases are different. 

(and it's not a big deal either way -- it was just a small point of confusion for me reading the code)