Trigger deployment as success when new replicaSet has reached min you need #2143
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
I signed it! |
k8s-ci-robot
added
cncf-cla: yes
pkg/kube/client.go
Outdated
| for _, r := range rs.Items { | ||
| list, err := getPods(client, value.Namespace, r.Spec.Selector.MatchLabels) | ||
| currentReplicaSet, err := client.ReplicaSets(value.Namespace).Get(r.Name) |
There was a problem hiding this comment.
Does this Get have to happen? They should be up to date from the Replica sets query above. You should just be able to just use rs.Items
pkg/kube/client.go
Outdated
| if err != nil { | ||
| return false, err | ||
| } | ||
| newDeployment := struct { |
There was a problem hiding this comment.
Because you use this struct in 3 places, it might just be cleaner/easier to create it as a type
There was a problem hiding this comment.
Yep good point. Created as type in same file as I wasn't 100% sure where types used just in this file should go. Let me know if it needs to go to another place.
|
Not entirely sure why circleci failed with the error on memory limit after my latest commit. @thomastaylor312 is there anything extra I can do from my side? |
|
@kiich I am guessing that isn't a problem with your code. We have been hitting the limit with some CI things. I'll test this afternoon or over the weekend. Thanks for the quick response! |
pkg/kube/client.go
Outdated
| @@ -59,6 +61,12 @@ type Client struct { | |||
| SchemaCacheDir string | |||
| } | |||
|
|
|||
| // Deployments hold associated replicaSets for deployments | |||
| type Deployments struct { | |||
There was a problem hiding this comment.
This should probably be unexported because other packages won't need to use it
|
@kiich I am done testing and things look good! I had one comment about this. It looks like it works as intended, but it does change the current behavior in one way. Right now as the Could you please update the documentation here to reflect the change in behavior in 2.3 and also the |
|
@thomastaylor312 Great to hear that! Good point on the Do you want me to wait updating the docs until we figure out the above? |
|
@thomastaylor312 Although thinking about it a bit more, by setting "minAvailable" to 1 in Let me know your thoughts. |
|
Also @thomastaylor312 when using |
|
@kiich Sorry about that. I had written that and then tested again and forgot to remove it. I think we'll just go with the documentation route on this one. Once that is pushed up, I'll read through it and then merge if it looks good |
|
@kiich Just circling back on this one. We want to get this in to 2.3 and so are just waiting on that documentation before merging. |
|
Hey @thomastaylor312 Sorry for the delay in this - was at KubeCon Europe
last week so hadn't had a chance to finish it off.
Will make it a priority!
Thanks
On Sat, 1 Apr 2017 at 20:26, Taylor Thomas <notifications@github.com> wrote:
@kiich <https://github.com/kiich> Just circling back on this one. We want
to get this in to 2.3 and so are just waiting on that documentation before
merging.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2143 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AENjRHwvDJXI_Y-0aQUxwLMoToZAhwAwks5rrqTfgaJpZM4MivlP>
.
|
|
@thomastaylor312 Apologies for the delay - Docs updated and hopefully what you were looking for. I had one question in that I wasn't sure how much detail to go into regards to the scenario where |
|
Yes, that is correct. I would at least mention it in the md doc though. Also, I don't think you need to worry about updating all the man pages right now. I think we do that right before the release. Thank you for thinking of that though! Otherwise, I think mentioning about the |
|
Ah got it - further update to the md doc done. Let me know if that is enough/need further clarification. Sorry about the extra changes in the man pages and cobra generated md docs - pretty new to this so was too keen to make sure everywhere was updated! |
|
@kiich That documentation change looks great. I can give this a merge as soon as you rebase and if you can pop off the cobra and man doc changes. Thanks for all of the hard work! |
|
Oh, and I'll figure out tests. It looks like the memory issue/spurious failure we have been running into lately |
… you need which is number of replicas minus maxUnavailable
kiich
force-pushed
the
deploymentsReady-when-newRS-has-minimumReplicas
branch
from
427d018
to
e3655bb
Compare
|
That's great! Commits squashed and doc changes removed. Let me know if anything needs doing! |
|
@kiich This still needs a rebase against master to solve the merge conflict. |
|
Ah apologies - I didn't get around in working on the conflict. I've used the github feature to resolve it as I was interested in trying that out - hope this is still ok. |
Current check for deployment as success is to get related replicasets and then wait till all pods are ready.
This PR is to fine tune this logic where we get the new replica set associated to the deployment, wait until this new replicaset has reached minimum we require which is calculated from number of replicas needed in deployment minus value of maxUnavailable.
Once this value is reached, we treat the deployment is ready.