Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch request should update resource to indicate patch operation is in progress #109490

Closed
muang0 opened this issue Apr 14, 2022 · 8 comments
Closed
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/apps Categorizes an issue or PR as relevant to SIG Apps. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@muang0
Copy link

muang0 commented Apr 14, 2022

What happened?

A 'patch' request to a statefulset doesn't update the resource status (this doesn't happen until the associated k8s controller picks up the change and performs an 'update'). Because of this, we're encountering a race condition between helm and the k8s statefulset controller during installs. Helm checks the resource status to determine readiness after applying the patch, which can happen before the k8s controller has the chance to update the statefulset status. When a 'patch' request is handled, there should be some update on the resource status that indicates a patch operation is in progress. Unfortunately I can't share logs because this is something I am encountering in my cluster at work (highly restrictive fintech).

Associated issue opened to helm here

What did you expect to happen?

A 'patch' request should also update the resource status (when applicable).

How can we reproduce it (as minimally and precisely as possible)?

NA, this is a potential design bug with how 'patch' requests are handled

Anything else we need to know?

No response

Kubernetes version

1.21.5

Cloud provider

AWS EKS

OS version

NA

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

eksctl

Container runtime (CRI) and version (if applicable)

NA

Related plugins (CNI, CSI, ...) and versions (if applicable)

NA

@muang0 muang0 added the kind/bug Categorizes issue or PR as related to a bug. label Apr 14, 2022
@k8s-ci-robot k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Apr 14, 2022
@k8s-ci-robot
Copy link
Contributor

@muang0: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Apr 14, 2022
@muang0
Copy link
Author

muang0 commented Apr 14, 2022

/sig sig-api-machinery

@k8s-ci-robot
Copy link
Contributor

@muang0: The label(s) sig/sig-api-machinery cannot be applied, because the repository doesn't have them.

In response to this:

/sig sig-api-machinery

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@muang0
Copy link
Author

muang0 commented Apr 14, 2022

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 14, 2022
@liggitt
Copy link
Member

liggitt commented Apr 18, 2022

/remove-sig api-machinery

this isn't specific to patch requests, but to update requests in general, relative to the status reported by the controller
/sig apps

@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Apr 18, 2022
@liggitt
Copy link
Member

liggitt commented Apr 18, 2022

A patch API request is written atomically... there's no persisted "in progress" intermediate state for a patch API request

@liggitt
Copy link
Member

liggitt commented Apr 18, 2022

specifically for StatefulSet, is status.observedGeneration what you are wanting? that indicates the .metadata.generation which the controller had observed when it most recently updated .status. If .metadata.generation != .status.observedGeneration, that means status is stale

@liggitt liggitt added kind/feature Categorizes issue or PR as related to a new feature. triage/needs-information Indicates an issue needs more information in order to work on it. and removed kind/bug Categorizes issue or PR as related to a bug. labels Apr 18, 2022
@muang0
Copy link
Author

muang0 commented Apr 19, 2022

Thanks for the quick response Jordan! A generational readiness check should solve the issue we're facing. Currently helm only checks the number of replicas and their readiness status. Thanks again for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/apps Categorizes an issue or PR as relevant to SIG Apps. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
None yet
Development

No branches or pull requests

3 participants