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.

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 negative unavailable replicas #14046

Merged
merged 1 commit into from
May 19, 2017
Merged

Fix negative unavailable replicas #14046

merged 1 commit into from
May 19, 2017

Conversation

0xmichalis
Copy link
Contributor

@mfojtik @tnozicka saw this in one of our jobs

https://ci.openshift.redhat.com/jenkins/view/All/job/ami_build_origin_int_rhel_fork/7/consoleFull#-169259343956c60d7be4b02b88ae8c268b

status:
  availableReplicas: 3
  conditions:
  - lastTransitionTime: 2017-05-03T15:38:41Z
    lastUpdateTime: 2017-05-03T15:38:41Z
    message: Deployment config has minimum availability.
    status: "True"
    type: Available
  - lastTransitionTime: 2017-05-03T15:41:36Z
    lastUpdateTime: 2017-05-03T15:41:38Z
    message: replication controller "history-limit-11" successfully rolled out
    reason: NewReplicationControllerAvailable
    status: "True"
    type: Progressing
  details:
    causes:
    - type: ConfigChange
    message: config change
  latestVersion: 11
  observedGeneration: 22
  readyReplicas: 1
  replicas: 1
  unavailableReplicas: -2
  updatedReplicas: 1

Similar fix to kubernetes/kubernetes#38299

Also fixes #11462

Still need to finish the unit test for calculateStatus but this is ready for reviews.

@0xmichalis
Copy link
Contributor Author

[test]

mfojtik
mfojtik reviewed May 15, 2017
View reviewed changes
pkg/deploy/util/util.go
func MaxUnavailable(config deployapi.DeploymentConfig) int32 {
if !IsRollingConfig(&config) {
func MaxUnavailable(config *deployapi.DeploymentConfig) int32 {
if !IsRollingConfig(config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to pass a pointer there when we don't mutate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what the convention is but since in Go this would be just a shallow copy it can't guarantee you that you won't change the object so passing it by pointer makes it clear that the modifications might have a side effect + the struct is fairly large

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tnozicka exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.google.com/document/d/190lWs3SeU4lCnNtscErlCtFgISNlbvDlu5OeISOLKo0/edit?ts=592c1efe#

@mfojtik
Copy link
Contributor

mfojtik commented May 15, 2017

one nit, LGTM

tnozicka
tnozicka reviewed May 17, 2017
View reviewed changes
pkg/deploy/controller/deploymentconfig/deploymentconfig_controller_test.go Outdated
rcs []*kapi.ReplicationController

expected deployapi.DeploymentConfigStatus
}{}
Copy link
Contributor

Choose a reason for hiding this comment

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

@Kargakis where are the test cases? this seems like an empty array...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will add them today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added unit tests to exercise the changes this PR introduces, ptal

pkg/deploy/util/util.go
func MaxUnavailable(config deployapi.DeploymentConfig) int32 {
if !IsRollingConfig(&config) {
func MaxUnavailable(config *deployapi.DeploymentConfig) int32 {
if !IsRollingConfig(config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what the convention is but since in Go this would be just a shallow copy it can't guarantee you that you won't change the object so passing it by pointer makes it clear that the modifications might have a side effect + the struct is fairly large

tnozicka
tnozicka reviewed May 17, 2017
View reviewed changes
pkg/deploy/controller/deploymentconfig/deploymentconfig_controller_test.go Outdated
{
name: "unavailable deployment",

dc: newDC(2, availableCond),
Copy link
Contributor

Choose a reason for hiding this comment

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

@Kargakis is this intentional or should it be unavailableCond?

pkg/deploy/controller/deploymentconfig/deploymentconfig_controller_test.go Outdated
UpdatedReplicas: int32(0),
UnavailableReplicas: int32(1),
Conditions: []deployapi.DeploymentCondition{
availableCond,
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

tnozicka
tnozicka approved these changes May 17, 2017
View reviewed changes
Copy link
Contributor

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

lgtm

@mfojtik
Copy link
Contributor

mfojtik commented May 17, 2017

LGTM [merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 9364ad8

@0xmichalis 0xmichalis mentioned this pull request May 18, 2017
Merged
@mfojtik
Copy link
Contributor

mfojtik commented May 18, 2017

test flake is #13943

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 9364ad8

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1551/) (Base Commit: fb92178)

@openshift-bot
Copy link
Contributor

openshift-bot commented May 19, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/702/) (Base Commit: 5e6c6f7) (Image: devenv-rhel7_6240)

@openshift-bot openshift-bot merged commit ff5f7c5 into openshift:master May 19, 2017
@0xmichalis 0xmichalis deleted the fix-negative-unavailable-replicas branch May 19, 2017 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfojtik mfojtik mfojtik left review comments

@tnozicka tnozicka tnozicka approved these changes

Assignees

@tnozicka tnozicka

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stop listing pods in the deployment config controller
4 participants
@0xmichalis @mfojtik @openshift-bot @tnozicka