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

MinReadySeconds / AvailableReplicas for ReplicaSets #32771

Merged
merged 3 commits into from
Sep 28, 2016
Merged

MinReadySeconds / AvailableReplicas for ReplicaSets #32771

merged 3 commits into from
Sep 28, 2016

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented Sep 15, 2016

This PR adds minReadySeconds and availableReplicas for replica sets / replication controllers

Partially addresses #28381

cc: @mfojtik

@bgrant0607 for the api changes, @janetkuo for controller changes


This change is Reviewable

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 15, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2016
@k8s-bot
Copy link

k8s-bot commented Sep 19, 2016

GCE e2e build/test passed for commit 61e1a145cc29eb8664fac913c3804fed3a0658d4.

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 19, 2016
@0xmichalis
Copy link
Contributor Author

@k8s-bot gce e2e test this issue: #31776

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit d7dd751.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2016
@bgrant0607
Copy link
Member

API change LGTM

@bgrant0607 bgrant0607 added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Sep 22, 2016
// 1. minReadySeconds == 0, or
// 2. LastTransitionTime (is set) + minReadySeconds < current time
func IsPodAvailable(pod *Pod, minReadySeconds int32, now unversioned.Time) bool {
if !IsPodReady(pod) {
Copy link
Member

@janetkuo janetkuo Sep 23, 2016

Choose a reason for hiding this comment

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

Should we exclude inactive ones, i.e. the ones that succeeded, failed, or are terminating? If we're changing this, add this test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in this level. I wanted to keep IsPodAvailable merely as an incremental addition on top of IsPodReady. Callers should decide if they want to filter succeeded/failed/terminating pods, similar to how we use IsPodReady in the replica set and rc controllers.

@@ -1739,6 +1744,9 @@ type ReplicationControllerStatus struct {
// The number of ready replicas for this replication controller.
ReadyReplicas int32 `json:"readyReplicas,omitempty"`

// The number of available replicas for this replication controller.
Copy link
Member

Choose a reason for hiding this comment

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

explain what "available" means in the comment (how is it different from ready ones, and mention minreadyseconds)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -2050,6 +2055,9 @@ type ReplicationControllerStatus struct {
// The number of ready replicas for this replication controller.
ReadyReplicas int32 `json:"readyReplicas,omitempty" protobuf:"varint,4,opt,name=readyReplicas"`

// The number of available replicas for this replication controller.
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -632,6 +637,9 @@ type ReplicaSetStatus struct {
// The number of ready replicas for this replica set.
ReadyReplicas int32 `json:"readyReplicas,omitempty"`

// The number of available replicas for this replica set.
AvailableReplicas int32 `json:"availableReplicas,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

here also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2016
@k8s-github-robot
Copy link

@k8s-ci-robot
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@0xmichalis
Copy link
Contributor Author

@k8s-bot unit test this issue: #31729

fakePodControl := controller.FakePodControl{}
manager.podControl = &fakePodControl

// The controller checks for available pods at 00:21 - it shoud see only one available pod.
Copy link
Member

Choose a reason for hiding this comment

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

*should

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


// The controller checks for available pods at 00:21 - it shoud see only one available pod.
now = func() unversioned.Time { return unversioned.Date(2016, 11, 13, 0, 0, 21, 0, time.UTC) }
manager.syncReplicaSet(getKey(rs, t))
Copy link
Member

Choose a reason for hiding this comment

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

Is now only used here (in the unit test)? Why not just declare it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's used only for unit testing but we need to declare it in replica_set.go since it's part of it.:)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, now I see what now is for. Could you instead create a private function in manager/ReplicaSetController (e.g. setNow()) that does what now did? It'd be more clear IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@janetkuo
Copy link
Member

Fix the typo and the last suggestion then I have no more comments

// Minimum number of seconds for which a newly created pod should be ready
// without any of its container crashing, for it to be considered available.
// Defaults to 0 (pod will be considered available as soon as it is ready)
MinReadySeconds int32 `json:"minReadySeconds,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Please put this last in the spec, after Template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@bgrant0607
Copy link
Member

We're going to add minReadySeconds to all controllers. We should consider adding it to PodSpec, but version skew (Kubelets not supporting it) would make it hard to rely on, so I'm ok with this for now.

@bgrant0607 bgrant0607 removed their assignment Sep 27, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit af01ebcf917631223d3b9b5d06b7d0ac845acf61. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit af01ebcf917631223d3b9b5d06b7d0ac845acf61. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@janetkuo janetkuo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2016
@janetkuo
Copy link
Member

@k8s-bot gke e2e test this #33361

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2016
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 28, 2016
@0xmichalis
Copy link
Contributor Author

rebased, reapplying lgtm

@0xmichalis 0xmichalis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2016
@0xmichalis
Copy link
Contributor Author

@k8s-bot test this issue: #33388

@0xmichalis
Copy link
Contributor Author

@k8s-bot gke test this issue: #33617

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit d8dc2aa. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5af1b25 into kubernetes:master Sep 28, 2016
@0xmichalis 0xmichalis deleted the minReadySecondsForRS branch September 28, 2016 13:41
k8s-github-robot pushed a commit that referenced this pull request Nov 23, 2016
…ler-ready-available-replicas

Automatic merge from submit-queue

populate ready replicas and aviable replicas to federated replicaset …

populate ready replicas and aviable replicas to federated replicaset status

@nikhiljindal #33312
#29481 #32771

@deepak-vij
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants