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

Switch statefulset controller to shared informers #41482

Merged
merged 1 commit into from
Feb 23, 2017
Merged

Switch statefulset controller to shared informers #41482

merged 1 commit into from
Feb 23, 2017

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented Feb 15, 2017

Originally part of #40097

I think the controller currently makes a deep copy of a StatefulSet before it mutates it, but I'm not 100% sure. For those who are most familiar with this code, could you please confirm?

@Beeps @smarterclayton @ingvagabund @sttts @liggitt @deads2k @kubernetes/sig-apps-pr-reviews @kubernetes/sig-scalability-pr-reviews @timothysc @gmarek @wojtek-t

@ncdc ncdc added release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. labels Feb 15, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 15, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 15, 2017
0xmichalis
0xmichalis reviewed Feb 15, 2017
View reviewed changes
pkg/controller/statefulset/stateful_set.go
@@ -114,28 +110,33 @@ func NewStatefulSetController(podInformer cache.SharedIndexInformer, kubeClient
},
DeleteFunc: ssc.enqueueStatefulSet,
},
statefulSetResyncPeriod,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to switch to the default resync period but probably not before adding a pvc cache ... Add a TODO

Copy link
Member Author

Choose a reason for hiding this comment

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

@Kargakis do you want me to plumb in using a PVC informer now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary I think but an issue to extend the resync period and also add the PVC informer would be nice.

@ncdc
Copy link
Member Author

ncdc commented Feb 15, 2017

@k8s-bot kubemark e2e test this, infra flake

W0215 08:25:47.304] ERROR: (gcloud.components.install) The component [alpha] failed to download.
W0215 08:25:47.304] 
W0215 08:25:47.304] ('The read operation timed out',)

@smarterclayton
Copy link
Contributor

@foxish @kow3ns

@kow3ns
Copy link
Member

kow3ns commented Feb 15, 2017

@ncdc The code only mutates the StatefulSet.Status.Replicas field. The relevant code is here. The mutation is performed on a deep copy. The supplied *apps.StatefulSet parameter is treated as an inout parameter, and it is mutated, but it is only mutated when it successfully commits its current status API server. The value of the deep copy is re-assigned to the input parameter.

@kow3ns
Copy link
Member

kow3ns commented Feb 15, 2017

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

pkg/controller/statefulset/stateful_set.go, line 34 at r1 (raw file):

	"k8s.io/client-go/tools/record"
	"k8s.io/client-go/util/workqueue"
	"k8s.io/kubernetes/pkg/api"

space between client-go and kubernetes?

Comments from Reviewable

@smarterclayton
Copy link
Contributor

We're still combining everything under k8s.io into one import block.

@ncdc
Copy link
Member Author

ncdc commented Feb 15, 2017

The code only mutates the StatefulSet.Status.Replicas field. The relevant code is here. The mutation is performed on a deep copy. The supplied *apps.StatefulSet parameter is treated as an inout parameter, and it is mutated, but it is only mutated when it successfully commits its current status API server. The value of the deep copy is re-assigned to the input parameter.

@kow3ns ok, thanks. So I'll need to fix this in my PR, since the reassignment of the parameter results in changing the data in the shared cache.

@ncdc ncdc added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 15, 2017
@0xmichalis 0xmichalis mentioned this pull request Feb 16, 2017
Closed
@spiffxp
Copy link
Member

spiffxp commented Feb 16, 2017

FWIW I'm probably not the right person to be assigned this PR, I'm not in any of the OWNERS files. cc'ing the approvers listed at cmd/kube-controller-manager/OWNERS

/cc @deads2k @lavalamp @mikedanese

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 17, 2017
@ncdc
Copy link
Member Author

ncdc commented Feb 17, 2017

I've added a new commit that stops mutating the stateful sets and pods retrieved from the shared informer caches. PTAL.

@ncdc
Copy link
Member Author

ncdc commented Feb 17, 2017

@k8s-bot cvm gke e2e test this

smarterclayton
smarterclayton reviewed Feb 17, 2017
View reviewed changes
pkg/controller/statefulset/stateful_set_control_test.go Outdated
@@ -726,6 +915,11 @@ func scaleDownStatefulSetControl(set *apps.StatefulSet, ssc StatefulSetControlIn
spc.podsIndexer.Delete(pods[ordinal])
}
if err := ssc.UpdateStatefulSet(set, pods); err != nil {
fmt.Printf("ANDY3: %v\n", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

debug output

@ncdc ncdc force-pushed the shared-informers-11-statefulset branch from 6669ab9 to f6dbfbd Compare February 17, 2017 14:47
@ncdc
Copy link
Member Author

ncdc commented Feb 17, 2017

Rebased, squashed, debugging removed

@kow3ns
Copy link
Member

kow3ns commented Feb 17, 2017

Reviewed 2 of 4 files at r2, 3 of 4 files at r3.
Review status: 10 of 13 files reviewed at latest revision, 5 unresolved discussions.

pkg/controller/statefulset/stateful_pod_control.go, line 38 at r3 (raw file):

// implementation provides for PVC creation, ordered Pod creation, ordered Pod termination, and Pod identity enforcement.
// Like controller.PodControlInterface, it is implemented as an interface to provide for testing fakes.
type StatefulPodControlInterface interface {

Update comments to address changes made to the interface contract with respect to the mutation of the parameter.

pkg/controller/statefulset/stateful_pod_control.go, line 91 at r3 (raw file):

	podCopy := obj.(*v1.Pod)
	attemptedUpdate := false
	err = retry.RetryOnConflict(retry.DefaultBackoff, func() error {

Does this ever stop retrying. We will now have a backoff in the workqueue and here, what are the implications?

pkg/controller/statefulset/stateful_set_control.go, line 103 at r3 (raw file):

	// if the current number of replicas has changed update the statefulSets replicas
	if set.Status.Replicas != int32(ready) {
		obj, err := api.Scheme.Copy(set)

Move the StatefulSet deep copy into UpdateStatefulSetReplicas or move the pod deep copy out of UpdateStatefulPod for consistency.

Comments from Reviewable

@ncdc ncdc force-pushed the shared-informers-11-statefulset branch from ced85b3 to 20f1a71 Compare February 21, 2017 17:04
@ncdc
Copy link
Member Author

ncdc commented Feb 21, 2017

Updated to look up the stateful set from the cache in the event of a failed update of the set's replica count.

@0xmichalis
Copy link
Contributor

Updated to look up the stateful set from the cache in the event of a failed update of the set's replica count.

You'll want to deep-copy that or you'll make @deads2k sad. Also there is a pod read call in the other retry func that can be turned into a cache lookup. I just don't know if we want to go all-in with our caches... :)

@ncdc
Copy link
Member Author

ncdc commented Feb 21, 2017

You'll want to deep-copy that or you'll make @deads2k sad

And me too :-(. Will fix.

Also there is a pod read call in the other retry func that can be turned into a cache lookup. I just don't know if we want to go all-in with our caches... :)

I'll update that too. Thanks.

@ncdc ncdc force-pushed the shared-informers-11-statefulset branch from 20f1a71 to 74afebf Compare February 21, 2017 18:23
@ncdc
Copy link
Member Author

ncdc commented Feb 21, 2017

@Kargakis updated

@ncdc ncdc mentioned this pull request Feb 21, 2017
Closed
@ncdc
Copy link
Member Author

ncdc commented Feb 21, 2017

@kow3ns please review the latest update when you get a chance. Thanks!

@ncdc
Copy link
Member Author

ncdc commented Feb 21, 2017

@k8s-bot cvm gce e2e test this

@kow3ns
Copy link
Member

kow3ns commented Feb 21, 2017

Reviewed 1 of 1 files at r7, 3 of 3 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

@smarterclayton
Copy link
Contributor

@k8s-bot cvm gce e2e test this
@k8s-bot non-cri e2e test this

@ncdc
Copy link
Member Author

ncdc commented Feb 22, 2017

@k8s-bot cvm gce e2e test this

@ncdc ncdc force-pushed the shared-informers-11-statefulset branch from 74afebf to f6a186b Compare February 22, 2017 14:06
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: ncdc, smarterclayton

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

@ncdc
Copy link
Member Author

ncdc commented Feb 22, 2017

Anyone have any other comments?

@kow3ns
Copy link
Member

kow3ns commented Feb 22, 2017

@k8s-bot non-cri e2e test this

@ncdc
Copy link
Member Author

ncdc commented Feb 22, 2017

The non-cri e2e failure looks like the recently-created #41889. I don't think it's unique to this PR.

@ncdc ncdc mentioned this pull request Feb 22, 2017
Merged
@deads2k
Copy link
Contributor

deads2k commented Feb 22, 2017

lgtm

@ncdc
Copy link
Member Author

ncdc commented Feb 22, 2017

Green tests 😄

@0xmichalis 0xmichalis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2017
@kow3ns
Copy link
Member

kow3ns commented Feb 22, 2017

/lgtm

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 41146, 41486, 41482, 41538, 41784)

@k8s-github-robot k8s-github-robot merged commit 4396f19 into kubernetes:master Feb 23, 2017
k8s-github-robot pushed a commit that referenced this pull request Feb 25, 2017
Merge pull request #41918 from ncdc/shared-informers-14-scheduler
8e6af48 
Automatic merge from submit-queue (batch tested with PRs 41714, 41510, 42052, 41918, 31515)

Switch scheduler to use generated listers/informers

Where possible, switch the scheduler to use generated listers and
informers. There are still some places where it probably makes more
sense to use one-off reflectors/informers (listing/watching just a
single node, listing/watching scheduled & unscheduled pods using a field
selector).

I think this can wait until master is open for 1.7 pulls, given that we're close to the 1.6 freeze.

After this and #41482 go in, the only code left that references legacylisters will be federation, and 1 bit in a stateful set unit test (which I'll clean up in a follow-up).

@resouer I imagine this will conflict with your equivalence class work, so one of us will be doing some rebasing 😄 

cc @wojtek-t @gmarek  @timothysc @jayunit100 @smarterclayton @deads2k @liggitt @sttts @derekwaynecarr @kubernetes/sig-scheduling-pr-reviews @kubernetes/sig-scalability-pr-reviews
@wongma7 wongma7 mentioned this pull request Mar 22, 2017
Closed
@0xmichalis 0xmichalis mentioned this pull request Apr 7, 2017
Closed
@ncdc ncdc deleted the shared-informers-11-statefulset branch April 18, 2017 18:02
@0xmichalis 0xmichalis removed their assignment Aug 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarterclayton smarterclayton smarterclayton left review comments

@0xmichalis 0xmichalis 0xmichalis left review comments

Assignees

@foxish foxish

@smarterclayton smarterclayton

@kow3ns kow3ns

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. release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@ncdc @k8s-reviewable @smarterclayton @kow3ns @spiffxp @0xmichalis @k8s-github-robot @deads2k @foxish @k8s-ci-robot