rebuild filesystem map on config change #95

gabemontero · 2019-02-08T02:59:47Z

No description provided.

gabemontero · 2019-02-08T15:03:23Z

the latest e2e-aws-operator run uncovered some more short comings in the e2e test ... it minimally needs to wait for the skip list change to be fully processed by the operator ... in the last run, it became apparent it was not

will be adding fixes to this PR

gabemontero · 2019-02-08T20:26:09Z

image-eco: another incredibly slow dancer-mysql build wrt downloading dancer-mysql dependencies from various github and other internet locations

gabemontero · 2019-02-08T20:26:28Z

and the first clean aws-operator in a bit

gabemontero · 2019-02-08T20:26:49Z

/test e2e-aws-image-ecosystem

gabemontero · 2019-02-08T20:27:15Z

running operator e2e again

/test e2e-aws-operator

gabemontero · 2019-02-08T21:49:22Z

aws-operator passes again

image-eco flakes again with the same dancer slowness ....

@bparees ptal at the changes

I'll initiate more test runs, but if these slowness flakes persist, having to dive into them and possible disabling tests or increasing timeouts of these tests might be needed .... I think the delays are upstream internet issues, but a second or third set of eyes may not hurt if it comes to that

gabemontero · 2019-02-08T21:49:35Z

/test e2e-aws-image-ecosystem

gabemontero · 2019-02-08T21:49:43Z

/test e2e-aws-operator

bparees · 2019-02-08T21:58:39Z

pkg/stub/interfaces.go

-		imagestream = tagInPayload("v4.0", "IMAGE_AGENT_NODEJS", imagestream)
+	if g.h != nil {
+		// if we are not skipping jenkins imagestream, tag in images from payload
+		if _, ok := g.h.imagestreamFile["jenkins"]; ok {


don't need to handle the 2 agent imagestream names separately?

I was inclined to think that customers would not be looking to manage the agent streams since they don't come from known samples content from openshift/library

But sure perhaps that is an over reaching assumption?

if we allow people to skip an imagestream by name i think we have to assume they might use that to skip the jenkins agent imagestreams in addition to the master imagestream. (and conversely, just because they skip jenkins doesn't mean they want to skip the agents)

this will go away in next push

bparees · 2019-02-08T22:02:49Z

pkg/stub/interfaces.go

-	case imagestream.Name == "jenkins-agent-nodejs":
-		imagestream = tagInPayload("v4.0", "IMAGE_AGENT_NODEJS", imagestream)
+	if g.h != nil {
+		// if we are not skipping jenkins imagestream, tag in images from payload


why does this need to be special cased?

file processing can occur regardless of actual imagestream upserting

I'll add a comment

but why not just always do this?

(ie always update the imagestream w/ the payload pullspec, even if the user is "skipping" the imagestream).

If anything it seems safer than returning an un-mutated imagestream that contains content we know is wrong.

I think there will be cases when users want to import their own jenkins images ... at least at the beginning of 4.0 dev @jupierce relayed to me his team would need this

The agent streams yeah maybe it just makes sense to always mutate

though per the other comment thread maybe we do allow manage separately, and allow to skip :-)

What I was trying to get at is that I see calling this function as orthogonal to whether the imagestream is being skipped or not.

This function's job(as i understand it) is to return an imagestream for a given filepath. (and in the case of jenkins, handle mutating the imagestream as needed, though maybe this isn't the right place to do the mutation since presumably the other imagestream mutations, such as to change the registry hostname, are done elsewhere).

It shouldn't care whether a given imagestream is in the skipped list or not, that should be up to the caller to filter out.

So making this function aware of the skiplist, specifically to handle jenkins mutation, seems awkward.

What would happen if this function always mutated the jenkins imagestream before returning it, regardless of the skiplist?

Alternatively, can we put the mutation in the same place other imagestream mutation is done?

Admittedly the path of least resistance was here. I'll revisit isolating the mutation, placing it elsewhere, and we can compare the awkwardness, etc.

yeah it will be cleaner ... part of next push

bparees · 2019-02-08T22:04:34Z

pkg/stub/config.go

 		newStreamMap[si] = true
 	}
 	h.skippedImagestreams = newStreamMap
 	h.skippedTemplates = newTempMap

 }

-func (h *Handler) buildFileMaps(cfg *v1.Config) error {
+func (h *Handler) buildFileMaps(cfg *v1.Config, cfgChange bool) error {


maybe rename this to forceRebuild ? That seems to be its intent.

Yeah I went with the "why" vs. "intent" etc.

But there may be new reasons to force in the future ... I'll change

part of next push

bld skip filters off of Status.Skipped* don't tag payload for jenkins* when jenkins in skip list make sure Status.Skipped* updated in TestSkipped

gabemontero · 2019-02-11T16:41:34Z

updates pushed @bparees ... ptal

bparees · 2019-02-11T18:27:05Z

/lgtm

openshift-ci-robot · 2019-02-11T18:27:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, gabemontero

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bparees,gabemontero]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2019-02-11T18:27:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, gabemontero

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bparees,gabemontero]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gabemontero · 2019-02-11T18:32:37Z

cool thx @bparees

and the "ben 1" commit will live in posterity :-)

bparees · 2019-02-11T18:35:28Z

doh. i swear github was only showing me one commit. oh well.

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2019

openshift-ci-robot requested review from coreydaley, legionus and dmage February 8, 2019 02:59

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 8, 2019

gabemontero force-pushed the clear-map-cfg-change branch from 11fe0c5 to 5a2687c Compare February 8, 2019 19:12

openshift-ci-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 8, 2019

bparees reviewed Feb 8, 2019

View reviewed changes

gabemontero added 2 commits February 11, 2019 10:20

rebuild filesystem map on config change

606cc18

bld skip filters off of Status.Skipped* don't tag payload for jenkins* when jenkins in skip list make sure Status.Skipped* updated in TestSkipped

ben 1

89577c4

gabemontero force-pushed the clear-map-cfg-change branch from 5a2687c to 89577c4 Compare February 11, 2019 16:41

openshift-ci-robot assigned bparees Feb 11, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2019

openshift-merge-robot merged commit 68fdae1 into openshift:master Feb 11, 2019

gabemontero deleted the clear-map-cfg-change branch February 11, 2019 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rebuild filesystem map on config change #95

rebuild filesystem map on config change #95

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

bparees Feb 8, 2019

gabemontero Feb 8, 2019

bparees Feb 8, 2019

gabemontero Feb 11, 2019

bparees Feb 8, 2019

gabemontero Feb 8, 2019

gabemontero Feb 8, 2019

bparees Feb 8, 2019

bparees Feb 8, 2019

gabemontero Feb 10, 2019

gabemontero Feb 10, 2019

bparees Feb 11, 2019

gabemontero Feb 11, 2019

gabemontero Feb 11, 2019

bparees Feb 8, 2019

gabemontero Feb 8, 2019

gabemontero Feb 11, 2019

gabemontero commented Feb 11, 2019

bparees commented Feb 11, 2019

openshift-ci-robot commented Feb 11, 2019

openshift-ci-robot commented Feb 11, 2019

gabemontero commented Feb 11, 2019

bparees commented Feb 11, 2019

rebuild filesystem map on config change #95

rebuild filesystem map on config change #95

Conversation

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

gabemontero commented Feb 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Feb 11, 2019

bparees commented Feb 11, 2019

openshift-ci-robot commented Feb 11, 2019

openshift-ci-robot commented Feb 11, 2019

gabemontero commented Feb 11, 2019

bparees commented Feb 11, 2019