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

fix: avoid empty reason when auto-pausing canary #133

Merged
merged 1 commit into from
Nov 30, 2021

Conversation

ahmed-mez
Copy link
Contributor

What does this PR do?

  • Make sure we don't return empty reasons in HighestRestartCount and MostRecentRestart
  • Add new errors to the cannotStartReasons set:
    • ImageInspectError
    • ErrImageNeverPull
    • RegistryUnavailable
    • InvalidImageName
    • PreCreateHookError

Motivation

Make the canary auto-pause more reliable

Describe your test plan

InvalidImageName is the easiest option to reproduce:

  • Deploy a valid eds with canary auto-pause enabled.
  • Update the workload image name to an invalid value (include non alpha-numerical characters).
  • Make sure the new canary is paused with the correct reason printed
  NAMESPACE  NAME              DESIRED  CURRENT  READY  UP-TO-DATE  AVAILABLE  IGNORED UNRESPONSIVE NODES  STATUS         REASON            ACTIVE RS               CANARY RS               AGE
  default    pause-containers  4        4        2      2           2          0                           Canary Paused  InvalidImageName  pause-containers-sz5ft  pause-containers-2qdx6  16 minutes

@ahmed-mez ahmed-mez added bug Something isn't working component/controller labels Nov 29, 2021
@ahmed-mez ahmed-mez added this to the v0.8 milestone Nov 29, 2021
@ahmed-mez ahmed-mez requested a review from a team as a code owner November 29, 2021 15:38
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

@codecov-commenter
Copy link

Codecov Report

Merging #133 (02379c6) into main (97329e9) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #133      +/-   ##
==========================================
+ Coverage   62.28%   62.31%   +0.03%     
==========================================
  Files          41       41              
  Lines        2166     2168       +2     
==========================================
+ Hits         1349     1351       +2     
  Misses        711      711              
  Partials      106      106              
Flag Coverage Δ
unittests 62.31% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
api/v1alpha1/extendeddaemonset_types.go 100.00% <ø> (ø)
pkg/controller/utils/pod/pod.go 91.26% <100.00%> (+0.17%) ⬆️

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 97329e9...02379c6. Read the comment docs.

@@ -142,7 +142,9 @@ func HighestRestartCount(pod *v1.Pod) (int, datadoghqv1alpha1.ExtendedDaemonSetS
restartCount = s.RestartCount
reason = datadoghqv1alpha1.ExtendedDaemonSetStatusReasonUnknown
if s.LastTerminationState != (v1.ContainerState{}) && *s.LastTerminationState.Terminated != (v1.ContainerStateTerminated{}) {
reason = datadoghqv1alpha1.ExtendedDaemonSetStatusReason(s.LastTerminationState.Terminated.Reason)
if s.LastTerminationState.Terminated.Reason != "" { // The Reason field is optional and can be empty
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ahmed-mez ahmed-mez merged commit e08ae24 into main Nov 30, 2021
@ahmed-mez ahmed-mez deleted the ahmed/avoid-empty-reason branch November 30, 2021 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component/controller
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants