Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

Determine app readiness based on deployment spec.Replicas rather than status.Replicas #1589

Merged
merged 2 commits into from
May 12, 2023

Conversation

g-linville
Copy link
Contributor

Before this change, we were checking for app readiness by ensuring that the app's Deployment had matching status.Replicas and status.ReadyReplicas. If for some reason the Deployment fails to create any replicas (such as a mutating webhook that fails, if Istiod is down for example), then both status.Replicas and status.ReadyReplicas are set to 0, and Acorn thinks the app is ready even though it is not running at all.

With this change, we look at the Deployment spec instead to see the desired number of replicas.

Checklist

  • The title of this PR would make a good line in Acorn's Release Note's Changelog
  • The title of this PR ends with a link to the main issue being address in parentheses, like: This is a title (#1216). Here's an example
  • All relevant issues are referenced in the PR description. NOTE: don't use GitHub keywords that auto-close issues
  • Commits follow contributing guidance
  • Automated tests added to cover the changes. If tests couldn't be added, an explanation is provided in the Verification and Testing section
  • Changes to user-facing functionality, API, CLI, and upgrade impacts are clearly called out in PR description
  • PR has at least two approvals before merging (or a reasonable exception, like it's just a docs change)

… status.Replicas

Signed-off-by: Grant Linville <grant@acorn.io>
@@ -481,7 +481,11 @@ func AppStatus(req router.Request, resp router.Response) error {

status := container[containerName]
status.Ready = dep.Status.ReadyReplicas
status.ReadyDesired = dep.Status.Replicas
if dep.Spec.Replicas != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this should be dep.status.Replica?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the status.Replicas has the possibility to be 0 (not set) if something goes wrong. spec.Replicas is a pointer so I have to do the nil check first before proceeding.

Copy link
Contributor

@StrongMonkey StrongMonkey May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But dep.spec.replicas will never be the replica that is ready? It is the desired replica for a deployment, not the ready replica that we are supposed to set on app.status.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait, this is ReadyDesired.... yeah setting spec.replica makes sense...

Copy link
Contributor

@njhale njhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

(tangent: It would be nice to factor transition logic out s.t. it's easier to write unit tests for changes like this)

Signed-off-by: Grant Linville <grant@acorn.io>
@g-linville g-linville merged commit 5253eba into acorn-io:main May 12, 2023
2 checks passed
@g-linville g-linville deleted the fix-app-status branch May 12, 2023 13:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants