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

[BUG] gke-deploy / PodDisruptionBudget #725

Closed
feitnomore opened this issue Oct 6, 2020 · 1 comment
Closed

[BUG] gke-deploy / PodDisruptionBudget #725

feitnomore opened this issue Oct 6, 2020 · 1 comment

Comments

@feitnomore
Copy link
Contributor

feitnomore commented Oct 6, 2020

Failing to use PodDisruptionBudget

When using PodDisruptionBudget with spec: maxUnavailable, although resource is being deployed, the builder can't identify it as ready/healthy.

Affected builder image

gcr.io/cloud-builders/gke-deploy

Expected Behavior

Trigger should pass however builder fails do identify the PodDisruptionBudget is working fine.

Actual Behavior

According to resource/ready.go under podDisruptionBudgetIsReady, one of the things that is being checked is status.desiredHealthy == spec.minAvailable, however if we deploy something using maxUnavailable instead of minAvailabe, that is supported on Kubernetes 1.17+, builder will always return false here:

minAvailable, ok, err := unstructured.NestedInt64(obj.Object, "spec", "minAvailable")
if err != nil {
	return false, fmt.Errorf("failed to get spec.minAvailable field: %v", err)
}
if !ok {
	return false, nil
}

Steps to Reproduce the Problem

Use maxUnavailable instead of minAvailable.

apiVersion: policy/v1beta1
kind: PodDisruptionBudget
metadata:
  name: nodejs04
  namespace: default
spec:
  maxUnavailable: 1
  selector:
    matchLabels:
      app: nodejs04

Additional Info

Suggestion here is to remove status.desiredHealthy == spec.minAvailable and minAvailable retrieval, and leave only checking is status.currentHealthy >= status.desiredHealthy as I think it should be sufficient.

If you are ok with the suggestion, I can try to submit a PR myself.

Thanks

@feitnomore
Copy link
Contributor Author

PR merged into master.

leeonlee pushed a commit that referenced this issue Oct 19, 2020
Removing minAvailable form podDisruptionBudgetIsReady
and updating the testing files and yamls accordingly
to reflect the changes made to the component logic.
The update to the tests are due to roll back of
PR #726

[Ticket: #725]

Co-authored-by: feitnomore <feitnomore@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants