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 deployment helper - no assumptions on only one new ReplicaSet #41851

Merged
merged 2 commits into from
Feb 23, 2017
Merged

Fix deployment helper - no assumptions on only one new ReplicaSet #41851

merged 2 commits into from
Feb 23, 2017

Conversation

janetkuo
Copy link
Member

@janetkuo janetkuo commented Feb 22, 2017 •
edited

#40415

Release note:

NONE

@kubernetes/sig-apps-bugs

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 22, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: janetkuo

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Feb 22, 2017
@janetkuo janetkuo assigned 0xmichalis and unassigned mfojtik Feb 22, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 22, 2017
@janetkuo janetkuo mentioned this pull request Feb 22, 2017
Closed
@0xmichalis
Copy link
Contributor

/lgtm

We should cherry-pick this to 1.5

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2017
@0xmichalis 0xmichalis added this to the v1.6 milestone Feb 22, 2017
@0xmichalis 0xmichalis added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. cherrypick-candidate labels Feb 22, 2017
@janetkuo
Copy link
Member Author

@k8s-bot non-cri e2e test this
@k8s-bot gci gce e2e test this
@k8s-bot gce etcd3 e2e test this

@janetkuo
Copy link
Member Author

cc @mwielgus for cherrypick approval

@janetkuo janetkuo modified the milestones: v1.5, v1.6 Feb 22, 2017
@0xmichalis
Copy link
Contributor

@k8s-bot gce etcd3 e2e test this

0xmichalis
0xmichalis reviewed Feb 22, 2017
View reviewed changes
pkg/controller/deployment/util/deployment_util.go
@@ -612,9 +612,10 @@ func EqualIgnoreHash(template1, template2 v1.PodTemplateSpec) bool {
// FindNewReplicaSet returns the new RS this given deployment targets (the one with the same pod template).
func FindNewReplicaSet(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet) (*extensions.ReplicaSet, error) {
newRSTemplate := GetNewReplicaSetTemplate(deployment)
sort.Sort(controller.ReplicaSetsByCreationTimestamp(rsList))
Copy link
Contributor

Choose a reason for hiding this comment

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

@smarterclayton @mfojtik we are going to choose the oldest replica set out of two identical replica sets for now - seems like the sanest thing to do when we end up in such a state (seems possible after cluster upgrades: #40415).

Copy link
Contributor

Choose a reason for hiding this comment

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

@janetkuo can you add a comment explaining this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

0xmichalis
0xmichalis reviewed Feb 22, 2017
View reviewed changes
pkg/controller/deployment/util/deployment_util.go Outdated
// Filter out replica set that has the same pod template spec as the deployment - that is the new replica set.
if EqualIgnoreHash(rs.Spec.Template, newRSTemplate) {
// Filter out new replica set
if rs == nil || (newRS != nil && rs.UID == newRS.UID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rs cannot be nil here.

@0xmichalis 0xmichalis removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2017
0xmichalis
0xmichalis reviewed Feb 22, 2017
View reviewed changes
pkg/controller/deployment/util/deployment_util.go
newRS, err := FindNewReplicaSet(deployment, rsList)
if err != nil {
return requiredRSs, allRSs, err
}
for _, pod := range podList.Items {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have PR that reworks FindOldReplicaSets - the podList is not needed at all. We should simply look at rs.spec.replicas for filtering RSs with and without pods.

@0xmichalis
Copy link
Contributor

@janetkuo just two comments, reapply lgtm once they are fixed.

@janetkuo janetkuo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2017
@k8s-github-robot k8s-github-robot removed approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 22, 2017
@janetkuo
Copy link
Member Author

Test failed because #41650 added staging godeps check. Running hack/update-staging-godeps.sh to fix it.

@janetkuo janetkuo added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 22, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 38957, 41819, 41851, 40667, 41373)

@k8s-github-robot k8s-github-robot merged commit 7cca0d1 into kubernetes:master Feb 23, 2017
@janetkuo janetkuo removed this from the v1.5 milestone Feb 23, 2017
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@janetkuo janetkuo added this to the v1.5 milestone Feb 23, 2017
@janetkuo janetkuo mentioned this pull request Feb 23, 2017
Merged
@smarterclayton
Copy link
Contributor

I just saw https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/41729/pull-kubernetes-bazel/6787/ flake - is this related to the changes made here?

@janetkuo janetkuo mentioned this pull request Feb 23, 2017
Merged
@0xmichalis
Copy link
Contributor

I just saw https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/41729/pull-kubernetes-bazel/6787/ flake - is this related to the changes made here?

Yes, @janetkuo already opened a fix #41961

@sjenning sjenning mentioned this pull request Feb 23, 2017
Closed
k8s-github-robot pushed a commit that referenced this pull request Feb 27, 2017
Merge pull request #41958 from janetkuo/automated-cherry-pick-of-#418…
a94de60 
…51-upstream-release-1.5

Automatic merge from submit-queue

Automated cherry pick of #41851 and #41961 upstream release 1.5

Automated cherry pick of #41851 and #41961 upstream release 1.5
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.5" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

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

@0xmichalis 0xmichalis 0xmichalis left review comments

Assignees

@0xmichalis 0xmichalis

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@janetkuo @k8s-reviewable @k8s-github-robot @0xmichalis @k8s-cherrypick-bot @smarterclayton @mfojtik @k8s-ci-robot