Rehearse jobs that use changed ci-operator configs #101

petr-muller · 2019-03-06T22:24:27Z

Post-#96, we are able to detect which ci-operator configs changed in a PR. To start rehearsing the jobs affected by changed ci-operator configs, we needed to implement these two remaining capabilities:

Walk through all presubmits and find which ones reference one of the changed configs
Merge the two discovered sets of rehearsal jobs without creating duplicates.

/cc @stevekuznetsov @bbguimaraes @droslean

stevekuznetsov · 2019-03-06T23:07:20Z

cmd/pj-rehearse/main_test.go

 	}

+	changedCiopConfigs := diffs.GetChangedCiopConfigs(ciopMasterConfig, ciopPrConfig, logger)
+	changedPresubmits.AddAll(diffs.GetPresubmitsForCiopConfigs(prowPRConfig, changedCiopConfigs, logger))


The fact that you needed to do this in the test, too, is smelly

True, that test basically mimics our main().

stevekuznetsov · 2019-03-06T23:08:31Z

pkg/config/jobs_test.go

+			"org/destination-repo": {prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "destination-job-for-org-destination-repo"}}},
+		},
+	}, {
+		description: "merge jobs with same name for a single repo, result has the one originally in destination",


Since we need unique names to work this might be better as an error, but meh

It cannot be an error. In rehearse, when both ci-operator config and a presubmit that uses it change, we'll want to rehearse the same job for two reasons, so it ends up being returned by two different "what jobs do we need to rehearse" methods, and those results need to be merged.

stevekuznetsov · 2019-03-06T23:08:46Z

pkg/config/release.go

 	// TemplatesPath is the path of the templates from release repo
 	TemplatesPath = "ci-operator/templates"
+	// Name of the configmap that stores all ci-operator configs
+	CiOperatorConfigsCMName = "ci-operator-configs"


This is not true -- we are sharded. See the ConfigMapName() method on the config.Info struct

I'm aware of that, I want to tackle that tomorrow. IMHO it's worth a separate PR. I only moved this constant, not created it. Rehearse is a little broken after the sharding anyways.

Addressed in #104

stevekuznetsov · 2019-03-06T23:10:18Z

pkg/rehearse/jobs.go

 // ConfigureRehearsalJobs filters the jobs that should be rehearsed, then return a list of them re-configured with the
 // ci-operator's configuration inlined.
-func ConfigureRehearsalJobs(toBeRehearsed map[string][]prowconfig.Presubmit, ciopConfigs config.CompoundCiopConfig, prNumber int, loggers Loggers, allowVolumes bool) []*prowconfig.Presubmit {
+func ConfigureRehearsalJobs(toBeRehearsed config.Presubmits, ciopConfigs config.CompoundCiopConfig, prNumber int, loggers Loggers, allowVolumes bool) []*prowconfig.Presubmit {


Might have been nicer to do the type changes in a separate PR

Maybe. It's basically just a rename, I did a separate commit, and it was mostly driven by the need for the AddAll method.

droslean

In general, this PR is ok. But still there are some cases missing.

If there is a changed ci-op config and a changed presubmit that its using that config, there is no need to do all of this, and instead we can just rehearse the changed presubmit with the changed ci-op config inlined. I am pretty sure that in most cases this is what will happen since if someone will change his ci-op config, prowgen will generate different presubmit.

So, the only thing left in this PR is to do this procedure only if there is no other changed presubmit that uses the changed ci-op config.

droslean · 2019-03-07T08:37:34Z

pkg/diffs/diffs.go

+			if job.Agent != string(pjapi.KubernetesAgent) {
+				continue
+			}
+			for _, container := range job.Spec.Containers {


there is no need to iterate here. just use job.Spec.Containers[0]
There is a validation already happening in the upstream library.

droslean · 2019-03-07T08:53:10Z

cmd/pj-rehearse/main.go

-	changedPresubmits := diffs.GetChangedPresubmits(masterConfig.Prow, prConfig.Prow, logger)
-	rehearsals := rehearse.ConfigureRehearsalJobs(changedPresubmits, prConfig.CiOperator, prNumber, loggers, o.allowVolumes)
+	toRehearse := diffs.GetChangedPresubmits(masterConfig.Prow, prConfig.Prow, logger)
+	toRehearse.AddAll(diffs.GetPresubmitsForCiopConfigs(prConfig.Prow, changedCiopConfigs, logger))


So, the intention here is to add all the jobs that are using the changed ci-op config?

All the jobs that are using some of the changed ci-op configs, there could be more...

AddAll prevents job being present twice, so if a job was already in toRehearse because it was changed directly, it's not added again.

petr-muller · 2019-03-07T09:50:36Z

If there is a changed ci-op config and a changed presubmit that its using that config, there is no need to do all of this, and instead we can just rehearse the changed presubmit with the changed ci-op config inlined. I am pretty sure that in most cases this is what will happen since if someone will change his ci-op config, prowgen will generate different presubmit.

So, the only thing left in this PR is to do this procedure only if there is no other changed presubmit that uses the changed ci-op config.

Can you elaborate on the changes that you are requesting? We detect which jobs changed directly. Then we detect which ciop configs changed and which jobs are using them, and merge these jobs with those changed directly. The resulting set of jobs will be rehearsed. What do you think should be done differently?

droslean · 2019-03-07T10:08:58Z

Can you elaborate on the changes that you are requesting? We detect which jobs changed directly. Then we detect which ciop configs changed and which jobs are using them, and merge these jobs with those changed directly. The resulting set of jobs will be rehearsed. What do you think should be done differently?

I wanted to be more selective in which jobs we should rehearse instead of running them all when we detect a changed ci-op config. I think what its implemented in this PR is a good start and we can be more selective in the future.

petr-muller · 2019-03-07T13:55:01Z

Addressed the actionable comments.

These methods will be useful when we have multiple causes of rehearse triggers, so we will need to simply merge instances of these maps without creating duplicities.

stevekuznetsov

/lgtm

openshift-ci-robot · 2019-03-07T17:15:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, stevekuznetsov

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [petr-muller,stevekuznetsov]

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 requested review from bbguimaraes, droslean and stevekuznetsov March 6, 2019 22:24

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 6, 2019

petr-muller force-pushed the ciop-change-rehearse branch from 506d9f4 to 85bfaa2 Compare March 6, 2019 22:28

stevekuznetsov reviewed Mar 6, 2019

View reviewed changes

droslean reviewed Mar 7, 2019

View reviewed changes

petr-muller force-pushed the ciop-change-rehearse branch from 85bfaa2 to e750492 Compare March 7, 2019 13:51

petr-muller added 2 commits March 7, 2019 14:57

rehearse: make map[string][]Presubmit a type with methods

4b07194

These methods will be useful when we have multiple causes of rehearse triggers, so we will need to simply merge instances of these maps without creating duplicities.

Rehearse jobs that use changed ci-operator configs

988fc1f

petr-muller force-pushed the ciop-change-rehearse branch from e750492 to 988fc1f Compare March 7, 2019 15:05

petr-muller mentioned this pull request Mar 7, 2019

Handle sharded ci-operator CMs in rehearse #104

Merged

stevekuznetsov approved these changes Mar 7, 2019

View reviewed changes

openshift-ci-robot assigned stevekuznetsov Mar 7, 2019

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

openshift-merge-robot merged commit ffa6e04 into openshift:master Mar 7, 2019

Rehearse jobs that use changed ci-operator configs #101

Rehearse jobs that use changed ci-operator configs #101

Uh oh!

Conversation

petr-muller commented Mar 6, 2019

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!

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!

droslean left a comment

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!

petr-muller commented Mar 7, 2019

Uh oh!

droslean commented Mar 7, 2019

Uh oh!

petr-muller commented Mar 7, 2019

Uh oh!

stevekuznetsov left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Mar 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants