diff --git a/cmd/pj-rehearse/main.go b/cmd/pj-rehearse/main.go index 2129b6c7f02..03ef0be411f 100644 --- a/cmd/pj-rehearse/main.go +++ b/cmd/pj-rehearse/main.go @@ -281,21 +281,24 @@ func rehearseMain() int { metrics.RecordOpportunity(toRehearseClusterProfiles, "cluster-profile-change") toRehearse.AddAll(toRehearseClusterProfiles) - rehearsals := rehearse.ConfigureRehearsalJobs(toRehearse, prConfig.CiOperator, prNumber, loggers, o.allowVolumes, changedTemplates, changedClusterProfiles) - metrics.RecordActual(rehearsals) - if len(rehearsals) == 0 { + jobConfigurer := rehearse.NewJobConfigurer(prConfig.CiOperator, prNumber, loggers, o.allowVolumes, changedTemplates, changedClusterProfiles, jobSpec.Refs) + + presubmitsToRehearse := jobConfigurer.ConfigurePresubmitRehearsals(toRehearse) + metrics.RecordActual(presubmitsToRehearse) + + if len(presubmitsToRehearse) == 0 { logger.Info("no jobs to rehearse have been found") return 0 - } else if len(rehearsals) > o.rehearsalLimit { + } else if len(presubmitsToRehearse) > o.rehearsalLimit { jobCountFields := logrus.Fields{ "rehearsal-threshold": o.rehearsalLimit, - "rehearsal-jobs": len(rehearsals), + "rehearsal-jobs": len(presubmitsToRehearse), } logger.WithFields(jobCountFields).Info("Would rehearse too many jobs, will not proceed") return 0 } - executor := rehearse.NewExecutor(rehearsals, prNumber, o.releaseRepoPath, jobSpec.Refs, o.dryRun, loggers, pjclient) + executor := rehearse.NewExecutor(presubmitsToRehearse, prNumber, o.releaseRepoPath, jobSpec.Refs, o.dryRun, loggers, pjclient) success, err := executor.ExecuteJobs() metrics.Execution = executor.Metrics if err != nil { diff --git a/pkg/rehearse/jobs.go b/pkg/rehearse/jobs.go index 4ab98cf9073..9ced96ebe43 100644 --- a/pkg/rehearse/jobs.go +++ b/pkg/rehearse/jobs.go @@ -110,24 +110,34 @@ func makeRehearsalPresubmit(source *prowconfig.Presubmit, repo string, prNumber return &rehearsal, nil } -func filterJobs(changedPresubmits map[string][]prowconfig.Presubmit, allowVolumes bool, logger logrus.FieldLogger) config.Presubmits { - ret := config.Presubmits{} +func filterPresubmits(changedPresubmits map[string][]prowconfig.Presubmit, allowVolumes bool, logger logrus.FieldLogger) config.Presubmits { + presubmits := config.Presubmits{} for repo, jobs := range changedPresubmits { for _, job := range jobs { jobLogger := logger.WithFields(logrus.Fields{"repo": repo, "job": job.Name}) - if err := filterJob(&job, allowVolumes); err != nil { + if len(job.Branches) == 0 { + jobLogger.Warn("cannot rehearse jobs with no branches") + continue + } + + if len(job.Branches) != 1 { + jobLogger.Warn("cannot rehearse jobs that run over multiple branches") + continue + } + + if err := filterJobSpec(job.Spec, allowVolumes); err != nil { jobLogger.WithError(err).Warn("could not rehearse job") continue } - ret.Add(repo, job) + presubmits.Add(repo, job) } } - return ret + return presubmits } -func filterJob(source *prowconfig.Presubmit, allowVolumes bool) error { +func filterJobSpec(spec *v1.PodSpec, allowVolumes bool) error { // there will always be exactly one container. - container := source.Spec.Containers[0] + container := spec.Containers[0] if len(container.Command) != 1 || container.Command[0] != "ci-operator" { return fmt.Errorf("cannot rehearse jobs that have Command different from simple 'ci-operator'") @@ -137,18 +147,14 @@ func filterJob(source *prowconfig.Presubmit, allowVolumes bool) error { if strings.HasPrefix(arg, "--git-ref") || strings.HasPrefix(arg, "-git-ref") { return fmt.Errorf("cannot rehearse jobs that call ci-operator with '--git-ref' arg") } + if arg == "--promote" { + return fmt.Errorf("cannot rehearse jobs that call ci-operator with '--promote' arg") + } } - if len(source.Spec.Volumes) > 0 && !allowVolumes { + if len(spec.Volumes) > 0 && !allowVolumes { return fmt.Errorf("jobs that need additional volumes mounted are not allowed") } - if len(source.Branches) == 0 { - return fmt.Errorf("cannot rehearse jobs with no branches") - } - - if len(source.Branches) != 1 { - return fmt.Errorf("cannot rehearse jobs that run over multiple branches") - } return nil } @@ -158,85 +164,108 @@ func filterJob(source *prowconfig.Presubmit, allowVolumes bool) error { // of the needed config file passed to the job as a direct value. This needs // to happen because the rehearsed Prow jobs may depend on these config files // being also changed by the tested PR. -func inlineCiOpConfig(job *prowconfig.Presubmit, targetRepo string, ciopConfigs config.CompoundCiopConfig, loggers Loggers) (*prowconfig.Presubmit, error) { - var rehearsal prowconfig.Presubmit - deepcopy.Copy(&rehearsal, job) - for _, container := range rehearsal.Spec.Containers { - for index := range container.Env { - env := &(container.Env[index]) - if env.ValueFrom == nil { - continue +func inlineCiOpConfig(container v1.Container, ciopConfigs config.CompoundCiopConfig, loggers Loggers) error { + for index := range container.Env { + env := &(container.Env[index]) + if env.ValueFrom == nil { + continue + } + if env.ValueFrom.ConfigMapKeyRef == nil { + continue + } + if config.IsCiopConfigCM(env.ValueFrom.ConfigMapKeyRef.Name) { + filename := env.ValueFrom.ConfigMapKeyRef.Key + + loggers.Debug.WithField(logCiopConfigFile, filename).Debug("Rehearsal job uses ci-operator config ConfigMap, needed content will be inlined") + ciopConfig, ok := ciopConfigs[filename] + if !ok { + return fmt.Errorf("ci-operator config file %s was not found", filename) } - if env.ValueFrom.ConfigMapKeyRef == nil { - continue + + ciOpConfigContent, err := yaml.Marshal(ciopConfig) + if err != nil { + loggers.Job.WithError(err).Error("Failed to marshal ci-operator config file") + return err } - if config.IsCiopConfigCM(env.ValueFrom.ConfigMapKeyRef.Name) { - filename := env.ValueFrom.ConfigMapKeyRef.Key - logFields := logrus.Fields{logCiopConfigFile: filename, logCiopConfigRepo: targetRepo, logRehearsalJob: job.Name} - loggers.Debug.WithFields(logFields).Debug("Rehearsal job uses ci-operator config ConfigMap, needed content will be inlined") + env.Value = string(ciOpConfigContent) + env.ValueFrom = nil + } + } + return nil +} - ciopConfig, ok := ciopConfigs[filename] - if !ok { - return nil, fmt.Errorf("ci-operator config file %s was not found", filename) - } +// JobConfigurer holds all the information that is needed for the configuration of the jobs. +type JobConfigurer struct { + ciopConfigs config.CompoundCiopConfig + profiles []config.ConfigMapSource - ciOpConfigContent, err := yaml.Marshal(ciopConfig) - if err != nil { - loggers.Job.WithError(err).Error("Failed to marshal ci-operator config file") - return nil, err - } + prNumber int + refs *pjapi.Refs + loggers Loggers + allowVolumes bool + templateMap map[string]string +} - env.Value = string(ciOpConfigContent) - env.ValueFrom = nil - } - } +// NewJobConfigurer filters the jobs and returns a new JobConfigurer. +func NewJobConfigurer(ciopConfigs config.CompoundCiopConfig, prNumber int, loggers Loggers, allowVolumes bool, templates []config.ConfigMapSource, profiles []config.ConfigMapSource, refs *pjapi.Refs) *JobConfigurer { + return &JobConfigurer{ + ciopConfigs: ciopConfigs, + profiles: profiles, + prNumber: prNumber, + refs: refs, + loggers: loggers, + allowVolumes: allowVolumes, + templateMap: fillTemplateMap(templates), } - - return &rehearsal, nil } -// 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 config.Presubmits, ciopConfigs config.CompoundCiopConfig, prNumber int, loggers Loggers, allowVolumes bool, templates []config.ConfigMapSource, profiles []config.ConfigMapSource) []*prowconfig.Presubmit { - var templateMap map[string]string - if allowVolumes { - templateMap = make(map[string]string, len(templates)) - for _, t := range templates { - templateMap[filepath.Base(t.Filename)] = t.TempCMName("template") - } +func fillTemplateMap(templates []config.ConfigMapSource) map[string]string { + templateMap := make(map[string]string, len(templates)) + for _, t := range templates { + templateMap[filepath.Base(t.Filename)] = t.TempCMName("template") } - rehearsals := []*prowconfig.Presubmit{} + return templateMap +} - rehearsalsFiltered := filterJobs(toBeRehearsed, allowVolumes, loggers.Job) - for repo, jobs := range rehearsalsFiltered { +// ConfigurePresubmitRehearsals adds the required configuration for the presubmits to be rehearsed. +func (jc *JobConfigurer) ConfigurePresubmitRehearsals(presubmits config.Presubmits) []*prowconfig.Presubmit { + var rehearsals []*prowconfig.Presubmit + + presubmitsFiltered := filterPresubmits(presubmits, jc.allowVolumes, jc.loggers.Job) + for repo, jobs := range presubmitsFiltered { for _, job := range jobs { - jobLogger := loggers.Job.WithFields(logrus.Fields{"target-repo": repo, "target-job": job.Name}) - rehearsal, err := makeRehearsalPresubmit(&job, repo, prNumber) + jobLogger := jc.loggers.Job.WithFields(logrus.Fields{"target-repo": repo, "target-job": job.Name}) + rehearsal, err := makeRehearsalPresubmit(&job, repo, jc.prNumber) if err != nil { jobLogger.WithError(err).Warn("Failed to make a rehearsal presubmit") continue } - rehearsal, err = inlineCiOpConfig(rehearsal, repo, ciopConfigs, loggers) - if err != nil { - jobLogger.WithError(err).Warn("Failed to inline ci-operator-config into rehearsal job") + if err := jc.configureJobSpec(rehearsal.Spec, jc.loggers.Debug.WithField("name", job.Name)); err != nil { + jobLogger.WithError(err).Warn("Failed to inline ci-operator-config into rehearsal presubmit job") continue } - if allowVolumes { - replaceCMTemplateName(rehearsal.Spec.Containers[0].VolumeMounts, rehearsal.Spec.Volumes, templateMap) - replaceClusterProfiles(rehearsal.Spec.Volumes, profiles, loggers.Debug.WithField("name", job.Name)) - } - jobLogger.WithField(logRehearsalJob, rehearsal.Name).Info("Created a rehearsal job to be submitted") rehearsals = append(rehearsals, rehearsal) } } - return rehearsals } +func (jc *JobConfigurer) configureJobSpec(spec *v1.PodSpec, logger *logrus.Entry) error { + if err := inlineCiOpConfig(spec.Containers[0], jc.ciopConfigs, jc.loggers); err != nil { + return err + } + + if jc.allowVolumes { + replaceCMTemplateName(spec.Containers[0].VolumeMounts, spec.Volumes, jc.templateMap) + replaceClusterProfiles(spec.Volumes, jc.profiles, logger) + } + return nil +} + // AddRandomJobsForChangedTemplates finds jobs from the PR config that are using a specific template with a specific cluster type. // The job selection is done by iterating in an unspecified order, which avoids picking the same job // So if a template will be changed, find the jobs that are using a template in combination with the `aws`,`openstack`,`gcs` and `libvirt` cluster types. diff --git a/pkg/rehearse/jobs_test.go b/pkg/rehearse/jobs_test.go index b7b547fb591..3663278968f 100644 --- a/pkg/rehearse/jobs_test.go +++ b/pkg/rehearse/jobs_test.go @@ -32,7 +32,7 @@ import ( "github.com/openshift/ci-tools/pkg/config" ) -func TestConfigureRehearsalJobs(t *testing.T) { +func TestConfigurePresubmitRehearsals(t *testing.T) { makeVoume := func(name string) v1.Volume { return v1.Volume{ Name: "cluster-profile", @@ -86,7 +86,9 @@ func TestConfigureRehearsalJobs(t *testing.T) { SHA: "85c627078710b8beee65d06d0cf157094fc46b03", Filename: filepath.Join(config.ClusterProfilesPath, "changed-profile1"), }} - ret := ConfigureRehearsalJobs(jobs, config.CompoundCiopConfig{}, 1234, Loggers{logrus.New(), logrus.New()}, true, nil, profiles) + + jc := NewJobConfigurer(config.CompoundCiopConfig{}, 1234, Loggers{logrus.New(), logrus.New()}, true, nil, profiles, nil) + ret := jc.ConfigurePresubmitRehearsals(jobs) var names []string for _, j := range ret { if vs := j.Spec.Volumes; len(vs) == 0 { @@ -131,7 +133,6 @@ func makeCMReference(cmName, key string) *v1.EnvVarSource { } func TestInlineCiopConfig(t *testing.T) { - testTargetRepo := "org/repo" testCiopConfigInfo := config.Info{ Org: "org", Repo: "repo", @@ -186,7 +187,7 @@ func TestInlineCiopConfig(t *testing.T) { job := makeTestingPresubmitForEnv(tc.sourceEnv) expectedJob := makeTestingPresubmitForEnv(tc.expectedEnv) - newJob, err := inlineCiOpConfig(job, testTargetRepo, tc.configs, testLoggers) + err := inlineCiOpConfig(job.Spec.Containers[0], tc.configs, testLoggers) if tc.expectedError && err == nil { t.Errorf("Expected inlineCiopConfig() to return an error, none returned") @@ -199,8 +200,8 @@ func TestInlineCiopConfig(t *testing.T) { return } - if !equality.Semantic.DeepEqual(expectedJob, newJob) { - t.Errorf("Returned job differs from expected:\n%s", diff.ObjectReflectDiff(expectedJob, newJob)) + if !equality.Semantic.DeepEqual(expectedJob, job) { + t.Errorf("Returned job differs from expected:\n%s", diff.ObjectReflectDiff(expectedJob, job)) } } }) @@ -378,7 +379,8 @@ func TestExecuteJobsErrors(t *testing.T) { return false, nil, nil }) - rehearsals := ConfigureRehearsalJobs(tc.jobs, testCiopConfigs, testPrNumber, testLoggers, true, nil, nil) + jc := NewJobConfigurer(testCiopConfigs, testPrNumber, testLoggers, true, nil, nil, nil) + rehearsals := jc.ConfigurePresubmitRehearsals(tc.jobs) executor := NewExecutor(rehearsals, testPrNumber, testRepoPath, testRefs, true, testLoggers, fakeclient) _, err = executor.ExecuteJobs() @@ -447,7 +449,8 @@ func TestExecuteJobsUnsuccessful(t *testing.T) { return true, ret, nil }) - rehearsals := ConfigureRehearsalJobs(tc.jobs, testCiopConfigs, testPrNumber, testLoggers, true, nil, nil) + jc := NewJobConfigurer(testCiopConfigs, testPrNumber, testLoggers, true, nil, nil, nil) + rehearsals := jc.ConfigurePresubmitRehearsals(tc.jobs) executor := NewExecutor(rehearsals, testPrNumber, testRepoPath, testRefs, false, testLoggers, fakeclient) success, _ := executor.ExecuteJobs() @@ -552,7 +555,8 @@ func TestExecuteJobsPositive(t *testing.T) { } fakecs.Fake.PrependWatchReactor("prowjobs", makeSuccessfulFinishReactor(watcher, tc.jobs)) - rehearsals := ConfigureRehearsalJobs(tc.jobs, testCiopConfigs, testPrNumber, testLoggers, true, nil, nil) + jc := NewJobConfigurer(testCiopConfigs, testPrNumber, testLoggers, true, nil, nil, nil) + rehearsals := jc.ConfigurePresubmitRehearsals(tc.jobs) executor := NewExecutor(rehearsals, testPrNumber, testRepoPath, testRefs, true, testLoggers, fakeclient) success, err := executor.ExecuteJobs() @@ -753,48 +757,54 @@ func TestWaitForJobsLog(t *testing.T) { check(dbgHook, "failure", logrus.DebugLevel, nil) } -func TestFilterJob(t *testing.T) { +func TestFilterPresubmits(t *testing.T) { testCases := []struct { description string volumesAllowed bool - valid bool - crippleFunc func(*prowconfig.Presubmit) *prowconfig.Presubmit + crippleFunc func(*prowconfig.Presubmit) map[string][]prowconfig.Presubmit + expected func(*prowconfig.Presubmit) config.Presubmits }{ { description: "job where command is not `ci-operator`", - crippleFunc: func(j *prowconfig.Presubmit) *prowconfig.Presubmit { + crippleFunc: func(j *prowconfig.Presubmit) map[string][]prowconfig.Presubmit { j.Spec.Containers[0].Command[0] = "not-ci-operator" - return j + return map[string][]prowconfig.Presubmit{"org/repo": {*j}} + }, + expected: func(j *prowconfig.Presubmit) config.Presubmits { + return config.Presubmits{} }, }, { description: "ci-operator job already using --git-ref", - crippleFunc: func(j *prowconfig.Presubmit) *prowconfig.Presubmit { + crippleFunc: func(j *prowconfig.Presubmit) map[string][]prowconfig.Presubmit { j.Spec.Containers[0].Args = append(j.Spec.Containers[0].Args, "--git-ref=organization/repo@master") - return j + return map[string][]prowconfig.Presubmit{"org/repo": {*j}} + }, - }, - { - description: "jobs running over multiple branches", - crippleFunc: func(j *prowconfig.Presubmit) *prowconfig.Presubmit { - j.Brancher.Branches = append(j.Brancher.Branches, "^feature-branch$") - return j + expected: func(j *prowconfig.Presubmit) config.Presubmits { + return config.Presubmits{} }, }, { description: "jobs that need additional volumes mounted, not allowed", - crippleFunc: func(j *prowconfig.Presubmit) *prowconfig.Presubmit { + crippleFunc: func(j *prowconfig.Presubmit) map[string][]prowconfig.Presubmit { j.Spec.Volumes = []v1.Volume{{Name: "volume"}} - return j + return map[string][]prowconfig.Presubmit{"org/repo": {*j}} + }, + expected: func(j *prowconfig.Presubmit) config.Presubmits { + return config.Presubmits{} }, }, { description: "jobs that need additional volumes mounted, allowed", volumesAllowed: true, - valid: true, - crippleFunc: func(j *prowconfig.Presubmit) *prowconfig.Presubmit { + crippleFunc: func(j *prowconfig.Presubmit) map[string][]prowconfig.Presubmit { + j.Spec.Volumes = []v1.Volume{{Name: "volume"}} + return map[string][]prowconfig.Presubmit{"org/repo": {*j}} + }, + expected: func(j *prowconfig.Presubmit) config.Presubmits { j.Spec.Volumes = []v1.Volume{{Name: "volume"}} - return j + return config.Presubmits{"org/repo": {*j}} }, }, } @@ -802,9 +812,11 @@ func TestFilterJob(t *testing.T) { t.Run(tc.description, func(t *testing.T) { basePresubmit := makeBasePresubmit() tc.crippleFunc(basePresubmit) - err := filterJob(basePresubmit, tc.volumesAllowed) - if err == nil && !tc.valid { - t.Errorf("Expected filterJob() to return error") + p := filterPresubmits(map[string][]prowconfig.Presubmit{"org/repo": {*basePresubmit}}, tc.volumesAllowed, logrus.New()) + + expected := tc.expected(basePresubmit) + if !equality.Semantic.DeepEqual(expected, p) { + t.Fatalf("Found: %#v\nExpected: %#v", p, expected) } })