[release/9.0] Make resource HealthStatus computed from HealthReports (#6368)

[adamint]

Backport of #6368 to release/9.0

Customer Impact

• Removes the ability to set HealthStatus directly on a resource, and removes the field from the resource service protocol
• Fixes a bug where health reports will not be updated if the overall health of the resource does not change
• Fixes a bug where the health status is not shown in resource details while waiting for health checks to initially run

Testing

Much manual testing, as well as additional unit tests for computing health status.

Risk

Medium. It is possible that this breaking change may impact existing projects that set HealthStatus, and may impact customer assumptions during resource updates - for example, resources now start as unhealthy if they are running and wait on a health check, instead of null, as it previously was.

Regression?

No

Microsoft Reviewers: Open in CodeFlow

Microsoft Reviewers: Open in CodeFlow

[davidfowl]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[joperezr]

Thanks @adamint, I also understand this is something that ACA team needs. Before we mark it as approved though, do you mind expanding a bit on the risk? So as opposed to just saying Medium, elaborate a bit more on what the potential issues are with this?

[radical]

Do we need tests for other states like Exited, or FailedToStart, building etc?

[adamint]

We don’t, all states other than Running are treated the same

[radical]

We don’t, all states other than Running are treated the same

Maybe tests to check for that?

[adamint]

That's definitionally true, but I can add these tests. Assuming they just need to go into main?

[adamint]

added this in #6461

[radical]

Fixes a bug where health reports will not be updated if the overall health of the resource does not change
Fixes a bug where the health status is not shown in resource details while waiting for health checks to initially run

Can we have tests for these?

[adamint]

Thanks @adamint, I also understand this is something that ACA team needs. Before we mark it as approved though, do you mind expanding a bit on the risk? So as opposed to just saying Medium, elaborate a bit more on what the potential issues are with this?

@joperezr sorry, the edit didn’t save. I’ve added this information now

[adamint]

Fixes a bug where health reports will not be updated if the overall health of the resource does not change

I've added this in #6461

Fixes a bug where the health status is not shown in resource details while waiting for health checks to initially run

There is already a test for this

[joperezr]

Thanks for adding the additional info, @adamint . I'm trying to parse your risk assessment and specifically to understand who might be broken by this. Am I understanding correctly that in order to be in a broken state is if I am explicitly setting a status for a resource inside my AppHost code? If they wanted to continue to do that, how can they go about it? Also, assuming there is no way to do this in a non-breaking way?

[adamint]

Am I understanding correctly that in order to be in a broken state is if I am explicitly setting a status for a resource inside my AppHost code?

It's a breaking code change; previously, you were able to directly set the HealthStatus and HealthReports for a resource yourself, either through setting the initial state or publishing updates. Now, you are no longer able to do either. Instead, you must add health checks and then the AppHost will compute the health reports and status for you.

If they wanted to continue to do that, how can they go about it? Also, assuming there is no way to do this in a non-breaking way?

We have made the decision to disallow that behavior, so they cannot continue to do that. Now that the only component that can set the HealthStatus is the AppHost, there is only one source of truth.

@joperezr

[joperezr]

Got it. Last question. Do we know how common is it for people to set their own Health status? Given this is a major version, it is technically ok to make this sort of breaking change, but it does feel like the type of change you don't want to take this late after snap as there will be less time (basically none) to react to feedback. I'm wondering if we considered quirking this, i.e. allowing the users to set an app context switch that allows them to continue setting the status and marking the API call as obsolete, and then fully removing it in the next major. This essentially is to reduce friction in adoption from 8 to 9.

[davidfowl]

This change needs to go in as it has API changes to both the gRPC proto file and the app model. It also fixes bugs with the current health checks implementation that affect the new dashboard in ACA.

[joperezr]

Any comments around whether or not we should quirk this? How concerned are we about breaking existing customers?

[davidfowl]

No we're not quirking this. This should go in as is.

[joperezr]

Ok. Merging as is, but let's keep an eye out for feedback on this.