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.

Sign up for GitHub

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

Jump to bottom

Fix IsPodAvailable() to wait the PodReady is true #72

Merged

clamoriniere merged 1 commit into master from clamoriniere/fixIsPodAvailabled

Dec 28, 2020

Collaborator

clamoriniere commented Dec 28, 2020 •

edited

Loading

What does this PR do?

Fix the function IsPodAvailable() used by the controller to compute the number of available Pods.

Motivation

This behaviour was the origin of a nasty bug with which the controller didn't respect the "MaxUnavailable" parameter.

The controller was considering too many Pods as available, for example, Pods in ImagePullBackoff were considered as available.

Additional Notes

Anything else we should know when reviewing?

Describe your test plan

Create a EDS instance on a multi nodes cluster (you can use the kand and the examples/kind-cluster-configuration.yaml)

apiVersion: datadoghq.com/v1alpha1
kind: ExtendedDaemonSet
metadata:
  name: foo
spec:
  strategy:
    rollingUpdate:
      maxParallelPodCreation: 1
      slowStartIntervalDuration: 1m
      maxUnavailable: 1
  template:
    spec: 
      containers:
      - name: daemon
        image: k8s.gcr.io/pause:3.0
      tolerations:
      - operator: Exists

when the EDS properly deployed, update it with:

apiVersion: datadoghq.com/v1alpha1
kind: ExtendedDaemonSet
metadata:
  name: foo
spec:
  strategy:
    rollingUpdate:
      maxParallelPodCreation: 1
      slowStartIntervalDuration: 1m
      maxUnavailable: 1
  template:
    spec: 
      containers:
      - name: daemon
        image: k8s.gcr.io/pause:42
      tolerations:
      - operator: Exists

The image doesn't exist. so the new pod should in ImagePullBackoff.

wanted behaviour: Only one pod should be updated and in ImagePullBackoff

clamoriniere requested a review from a team as a code owner

December 28, 2020 13:15

github-actions bot requested changes

View reviewed changes

github-actions bot left a comment

This pull request does not contain a valid label. Please add one of the following labels: bug, enhancement, documentation

clamoriniere force-pushed the clamoriniere/fixIsPodAvailabled branch from 7701682 to 559f692 Compare

December 28, 2020 13:24

github-actions bot requested changes

View reviewed changes

github-actions bot left a comment

This pull request does not contain a valid label. Please add one of the following labels: bug, enhancement, documentation

clamoriniere added the bug label

clamoriniere added this to the v0.5 milestone

clamoriniere added the component/controller label

github-actions bot approved these changes

View reviewed changes

github-actions bot left a comment

This pull request contains a valid label.

github-actions bot approved these changes

View reviewed changes

github-actions bot left a comment

This pull request contains a valid label.

clamoriniere force-pushed the clamoriniere/fixIsPodAvailabled branch from 559f692 to 2663570 Compare

December 28, 2020 13:36

github-actions bot approved these changes

View reviewed changes

github-actions bot left a comment

This pull request contains a valid label.

clamoriniere force-pushed the clamoriniere/fixIsPodAvailabled branch from 2663570 to ad7150a Compare

December 28, 2020 13:40

github-actions bot approved these changes

View reviewed changes

github-actions bot left a comment

This pull request contains a valid label.

clamoriniere force-pushed the clamoriniere/fixIsPodAvailabled branch from ad7150a to 5fdb190 Compare

December 28, 2020 13:42

github-actions bot approved these changes

View reviewed changes

github-actions bot left a comment

This pull request contains a valid label.

clamoriniere force-pushed the clamoriniere/fixIsPodAvailabled branch from 5fdb190 to 7135def Compare

December 28, 2020 19:05

github-actions bot approved these changes

View reviewed changes

github-actions bot left a comment

This pull request contains a valid label.


          Fix IsPodAvailable() to wait the PodReady is true

e8ceb8c

This behaviour was the origin of a nasty bug with which the controller
didn't respect the "MaxUnavailable" parameter.
The controller was considering to many Pods as available, for example,
Pods in ImagePullBackoff were considered as available.

clamoriniere force-pushed the clamoriniere/fixIsPodAvailabled branch from 7135def to e8ceb8c Compare

December 28, 2020 19:18

github-actions bot approved these changes

View reviewed changes

github-actions bot left a comment

This pull request contains a valid label.

codecov-io commented Dec 28, 2020

Codecov Report

Merging #72 (e8ceb8c) into master (9df60a1) will increase coverage by 0.09%.
The diff coverage is 23.28%.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   32.26%   32.36%   +0.09%     
==========================================
  Files          37       41       +4     
  Lines        2833     2917      +84     
==========================================
+ Hits          914      944      +30     
- Misses       1830     1882      +52     
- Partials       89       91       +2

Flag	Coverage Δ
unittests	`32.36% <23.28%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/plugin/common/client.go	`0.00% <ø> (ø)`
pkg/plugin/common/pods.go	`0.00% <0.00%> (ø)`
pkg/plugin/common/table.go	`0.00% <0.00%> (ø)`
pkg/plugin/common/utils.go	`62.50% <62.50%> (ø)`
pkg/controller/utils/pod/pod.go	`39.08% <100.00%> (+17.24%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87217d6...e8ceb8c. Read the comment docs.

xornivore approved these changes

View reviewed changes

clamoriniere merged commit 8e7c119 into master

clamoriniere deleted the clamoriniere/fixIsPodAvailabled branch

December 28, 2020 20:42

clamoriniere added a commit that referenced this pull request


          Fix IsPodAvailable() to wait the PodReady is true (#72)

0bfef44

This behaviour was the origin of a nasty bug with which the controller
didn't respect the "MaxUnavailable" parameter.
The controller was considering to many Pods as available, for example,
Pods in ImagePullBackoff were considered as available.

clamoriniere added a commit that referenced this pull request


          Fix IsPodAvailable() to wait the PodReady is true (#72) (#73)

5d52e25

This behaviour was the origin of a nasty bug with which the controller
didn't respect the "MaxUnavailable" parameter.
The controller was considering to many Pods as available, for example,
Pods in ImagePullBackoff were considered as available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment