Image shared informers by soltysh · Pull Request #13835 · openshift/origin

soltysh · 2017-04-20T13:45:32Z

This builds on top of the rebase PR, so only last commit matters here, but I wanted to start the discussion.

@smarterclayton mind taking a look if this is going in the right direction? While working on it I was wondering why we want to have two separate controllers one for scheduled and the other for regular image stream operations, isn't having a single sufficient?

soltysh · 2017-04-20T13:46:04Z

(8) Use shared informer in image controllers

soltysh · 2017-04-20T13:46:19Z

@mfojtik fyi

mfojtik · 2017-04-21T13:43:32Z

pkg/cmd/server/origin/run_components.go

+		!c.Options.ImagePolicyConfig.DisableScheduledImport,
+	)
+	if c.Options.ImagePolicyConfig.DisableScheduledImport {
+		glog.V(2).Infof("Scheduled image import is disabled - the 'scheduled' flag on image streams will be ignored")


wonder if there is a way to communicate this to users (if I create scheduled import as a user, I have no idea it is disabled, right?)

The image stream controller could know that and could add an annotation with that info. I'll defer to @smarterclayton for opinion, since he has mixed feelings, but I agree it would be beneficial for users to know this.

soltysh · 2017-05-04T11:01:30Z

[test]

@smarterclayton this is ready for final review

soltysh · 2017-05-04T13:27:42Z

Flake #14044

[test]

mfojtik · 2017-05-16T12:21:39Z

pkg/cmd/server/origin/run_components.go


 // RunImageImportController starts the image import trigger controller process.
 func (c *MasterConfig) RunImageImportController() {
+	isInformer := c.Informers.ImageStreams()


nuke this, you used this just once

mfojtik · 2017-05-16T12:21:51Z

pkg/cmd/server/origin/run_components.go

-	controller, scheduledController := factory.Create()
-	controller.Run()
+	ctrl, sched := imagecontroller.NewImageStreamControllers(
+		isInformer,


c.Informers.ImageStreams()

mfojtik · 2017-05-16T12:22:21Z

pkg/cmd/server/origin/run_components.go

-	}
-	controller, scheduledController := factory.Create()
-	controller.Run()
+	ctrl, sched := imagecontroller.NewImageStreamControllers(


s/ctrl/controller/

mfojtik · 2017-05-16T12:23:02Z

pkg/cmd/server/origin/run_components.go

 		glog.V(2).Infof("Scheduled image import is disabled - the 'scheduled' flag on image streams will be ignored")
 	} else {
-		scheduledController.RunUntil(utilwait.NeverStop)
+		sched.Run(utilwait.NeverStop)


scheduledController (hate cryptic short forms)

mfojtik · 2017-05-16T12:23:22Z

pkg/image/controller/factory.go

-	q := cache.NewResyncableFIFO(cache.MetaNamespaceKeyFunc)
-	cache.NewReflector(lw, &api.ImageStream{}, q, f.ResyncInterval).Run()
+	// instantiate informer based image stream controller
+	ctrl := newImageStreamController(isInformer, isNamespacer)


soltysh · 2017-05-18T15:00:16Z

Flake #13827

[test]

mfojtik · 2017-05-18T19:03:47Z

@soltysh we might do this e486a4f plumbing for the image controller as well (we are going to migrate all controllers eventually...) can you have a look? if not I can do that on top of this PR :-)

smarterclayton · 2017-05-18T19:55:57Z

pkg/image/controller/imagestream_controller.go

+
+	stream, err := c.getByKey(key.(string))
+	if err == nil && stream != nil {
+		glog.V(3).Infof("Queued import of stream %s/%s...", stream.Namespace, stream.Name)


Was this 3 before?

It's slightly different than what we've had before. But this is this and it was 3 back then.

soltysh · 2017-05-19T05:24:10Z

@mfojtik roger that, I'll try to update that tomorrow.

smarterclayton · 2017-05-19T20:02:13Z

pkg/image/controller/imagestream_controller.go

+	}
+	stream, err := c.lister.ImageStreams(namespace).Get(name, metav1.GetOptions{})
+	if apierrs.IsNotFound(err) {
+		glog.V(4).Infof("Image stream has been deleted: %v", key)


Doesn't need a log line

smarterclayton · 2017-05-19T20:05:12Z

pkg/image/controller/scheduled_image_controller.go

+	if !needsScheduling(sharedStream) {
+		return ErrNotImportable
+	}
+	stream := *sharedStream


Not sufficient, this is mutating a shared cache

smarterclayton · 2017-05-19T20:05:58Z

Logic looks ok except for a few things, want to see this rebased and green tests.

soltysh · 2017-05-20T21:37:25Z

I've rebased and squashed all the changes to a single commit. @mfojtik where's that commit coming from, I can't find that code anywhere in origin :/ I've left there TODO. Let's merge this in on green tests and I'll work out a followup with the init changes, wdyt?

openshift-bot · 2017-05-20T21:37:27Z

Evaluated for origin test up to 54306a3

openshift-bot · 2017-05-20T23:15:19Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1592/) (Base Commit: 37d5498)

soltysh · 2017-05-21T16:10:30Z

@mfojtik @smarterclayton any objections on merging this in?

mfojtik · 2017-05-22T08:44:56Z

@soltysh i'm fine merging this but you should follow up with the controller initialization :-)

mfojtik · 2017-05-24T13:30:21Z

[merge]

mfojtik · 2017-05-25T10:06:44Z

[merge][severity: blocker] memory use is still p0 for 3.6

mfojtik · 2017-05-25T14:38:34Z

[merge][severity: blocker]

flake: #14349

openshift-bot · 2017-05-25T14:45:51Z

Evaluated for origin merge up to 54306a3

openshift-bot · 2017-05-25T17:56:54Z

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/794/) (Base Commit: 4b3473a) (Extended Tests: blocker)

smarterclayton · 2017-05-25T19:00:31Z

Blocked by hate, force merging

mfojtik · 2017-05-25T19:01:56Z

soltysh assigned smarterclayton Apr 20, 2017

mfojtik reviewed Apr 21, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2017

soltysh force-pushed the image_shared_informers branch from e1b4c43 to a135f1c Compare May 2, 2017 11:04

openshift-bot 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 May 2, 2017

soltysh force-pushed the image_shared_informers branch from a135f1c to 47b6759 Compare May 4, 2017 10:59

soltysh changed the title ~~[WIP] Image shared informers~~ Image shared informers May 4, 2017

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2017

mfojtik reviewed May 16, 2017

View reviewed changes

soltysh mentioned this pull request May 17, 2017

More refactoring to image import controller soltysh/origin#3

Merged

soltysh force-pushed the image_shared_informers branch from 6091735 to 15f9c8e Compare May 17, 2017 16:15

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2017

smarterclayton reviewed May 18, 2017

View reviewed changes

smarterclayton reviewed May 19, 2017

View reviewed changes

Use shared informers in image controllers

54306a3

soltysh force-pushed the image_shared_informers branch from bb96e00 to 54306a3 Compare May 20, 2017 21:34

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2017

mfojtik added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2017

mfojtik mentioned this pull request May 25, 2017

Duplicate caches in OpenShift master #8229

Closed

mfojtik merged commit 016b65a into openshift:master May 25, 2017

soltysh deleted the image_shared_informers branch May 29, 2017 08:25

Conversation

soltysh commented Apr 20, 2017

Uh oh!

soltysh commented Apr 20, 2017

Uh oh!

soltysh commented Apr 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh commented May 4, 2017

Uh oh!

soltysh commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh commented May 18, 2017

Uh oh!

mfojtik commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh commented May 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented May 19, 2017

Uh oh!

soltysh commented May 20, 2017

Uh oh!

openshift-bot commented May 20, 2017

Uh oh!

openshift-bot commented May 20, 2017

Uh oh!

soltysh commented May 21, 2017

Uh oh!

mfojtik commented May 22, 2017

Uh oh!

mfojtik commented May 24, 2017

Uh oh!

mfojtik commented May 25, 2017

Uh oh!

mfojtik commented May 25, 2017

Uh oh!

openshift-bot commented May 25, 2017

Uh oh!

openshift-bot commented May 25, 2017

Uh oh!

smarterclayton commented May 25, 2017

Uh oh!

mfojtik commented May 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants