Skip to content

pj-rehearse: support handcrafted jobs with UNRESOLVED_CONFIG #1430

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

Merged
openshift-merge-robot merged 1 commit into from
Dec 1, 2020
Merged

pj-rehearse: support handcrafted jobs with UNRESOLVED_CONFIG #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 75 additions & 34 deletions pkg/rehearse/jobs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,13 +221,9 @@ func hasRehearsableLabel(labels map[string]string) bool {
}

// getResolverConfigForTest returns a resolved ci-operator based on the provided filename and only includes the specified test in the
// `tests` section of the config.
// `tests` section of the config. If `testname` is empty, the resolved config will contain all items from the original `tests`.
// The ImageStreamTagMap contains all imagestreamtags used within this config and is used to ensure they exist on all target clusters.
func getResolvedConfigForTest(ciopConfigs config.DataByFilename, resolver registry.Resolver, filename, testname string) (string, apihelper.ImageStreamTagMap, error) {
ciopConfig, ok := ciopConfigs[filename]
if !ok {
return "", nil, fmt.Errorf("ci-operator config file %s was not found", filename)
}
func getResolvedConfigForTest(ciopConfig config.DataWithInfo, resolver registry.Resolver, testname string) (string, apihelper.ImageStreamTagMap, error) {
// make copy so we don't change in-memory config
ciopCopy := config.DataWithInfo{
Configuration: ciopConfig.Configuration,
Expand All @@ -236,11 +232,11 @@ func getResolvedConfigForTest(ciopConfigs config.DataByFilename, resolver regist
// only include the test we need to reduce env var size
ciopCopy.Configuration.Tests = []api.TestStepConfiguration{}
for _, test := range ciopConfig.Configuration.Tests {
if test.As == testname {
if testname == "" || test.As == testname {
ciopCopy.Configuration.Tests = append(ciopCopy.Configuration.Tests, test)
break
}
}

ciopConfigResolved, err := registry.ResolveConfig(resolver, ciopCopy.Configuration)
if err != nil {
return "", nil, fmt.Errorf("failed resolve ReleaseBuildConfiguration: %w", err)
Expand All @@ -258,45 +254,90 @@ func getResolvedConfigForTest(ciopConfigs config.DataByFilename, resolver regist
return string(ciOpConfigContent), imageStreamTags, nil
}

// inlineCiOpConfig detects whether a job needs a ci-operator config file
// provided by a `ci-operator-configs` ConfigMap and if yes, returns a copy
// of the job where a reference to this ConfigMap is replaced by the content
// 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.
// inlineCiOpConfig detects whether a Container in a rehearsed job uses
// a ci-operator config file and if yes, it modifies the Container so that its
// environment has a CONFIG_SPEC variable containing a resolved configuration
// coming from the content of the release repository.
// This needs to happen because the config files or step registry content they
// refer to may change in the PR that triggered a rehearsal, and the rehearsals
// must use all content changed in this way.
//
// Also returns an ImageStreamTagMap with that contains all imagestreamtags used
// within the inlined config (this is needed to later ensure they exist on all
// target clusters where the rehearsals will execute).
func inlineCiOpConfig(container *v1.Container, ciopConfigs config.DataByFilename, resolver registry.Resolver, metadata api.Metadata, testname string, loggers Loggers) (apihelper.ImageStreamTagMap, error) {
allImageStreamTags := apihelper.ImageStreamTagMap{}
// replace all ConfigMapKeyRef mounts with inline config maps
for index := range container.Env {
env := &(container.Env[index])
if env.Name == "CONFIG_SPEC" {
// if CONFIG_SPEC has already been set, do not add new CONFIG_SPEC section
if container.Command == nil || container.Command[0] != "ci-operator" {
return allImageStreamTags, nil
}

var hasConfigEnv bool
var ciopConfig config.DataWithInfo
var envs []v1.EnvVar
for idx, env := range container.Env {
switch {
case env.Name == "CONFIG_SPEC" && env.ValueFrom != nil:
// job attempts to get CONFIG_SPEC from cluster resource, which is weird,
// unexpected and we cannot support rehearsals for that
return nil, fmt.Errorf("CONFIG_SPEC is set from a cluster resource, cannot rehearse such job")
case env.Name == "UNRESOLVED_CONFIG" && env.ValueFrom != nil:
// job attempts to get UNRESOLVED_CONFIG from cluster resource, which is weird,
// unexpected and we cannot support rehearsals for that
return nil, fmt.Errorf("UNRESOLVED_CONFIG is set from a cluster resource, cannot rehearse such job")
case env.Name == "CONFIG_SPEC" && env.Value != "":
// job already has inline CONFIG_SPEC: we should not modify it
return allImageStreamTags, nil
case env.Name == "UNRESOLVED_CONFIG" && env.Value != "":
if err := yaml.Unmarshal([]byte(env.Value), &ciopConfig.Configuration); err != nil {
return nil, fmt.Errorf("failed to unmarshal UNRESOLVED_CONFIG: %w", err)
}
// Annoying hack: UNRESOLVED_CONFIG means this is a handcrafted job, which means
// `testname` cannot be relied on (it is derived from job name, which is arbitrary
// in handcrafted jobs). We need the test name to know which `tests` field to
// resolve, so we try to detect it from `--target` arg, if present.
//
// The worst case is that we do not find the matching name. In such case,
Comment thread
petr-muller marked this conversation as resolved.
// the inlined config will contain all items from `tests` stanza.
testname = ""
for idx, arg := range container.Args {
if strings.HasPrefix(arg, "--target=") {
testname = strings.TrimPrefix(arg, "--target=")
break
}
if arg == "--target" {
if len(container.Args) == (idx + 1) {
return nil, errors.New("plain '--target' is a last arg, expected to be followed with a value")
}
testname = container.Args[idx+1]
break
}
}
hasConfigEnv = true
default:
// Another envvar, we just need to keep it
envs = append(envs, container.Env[idx])
}
}

// inline CONFIG_SPEC for all ci-operator jobs
if container.Command != nil && container.Command[0] == "ci-operator" {
if !hasConfigEnv {
if err := metadata.IsComplete(); err != nil {
return nil, fmt.Errorf("could not infer which ci-operator config this job uses: %w", err)
}
filename := metadata.Basename()
loggers.Debug.WithField(logCiopConfigFile, filename).Debug("Rehearsal job uses ci-operator config ConfigMap, needed content will be inlined")
ciOpConfigContent, imageStreamTags, err := getResolvedConfigForTest(ciopConfigs, resolver, filename, testname)
if err != nil {
loggers.Job.WithError(err).Error("Failed to get resolved config for test")
return nil, err
if _, ok := ciopConfigs[filename]; !ok {
return nil, fmt.Errorf("ci-operator config file %s was not found", filename)
}
apihelper.MergeImageStreamTagMaps(allImageStreamTags, imageStreamTags)
ciopConfig = ciopConfigs[filename]
loggers.Debug.WithField(logCiopConfigFile, filename).Debug("Rehearsal job would use ci-operator config from registry, its content will be inlined")
}

envs := container.Env
env := v1.EnvVar{
Name: "CONFIG_SPEC",
Value: ciOpConfigContent,
}
envs = append(envs, env)
container.Env = envs
ciOpConfigContent, imageStreamTags, err := getResolvedConfigForTest(ciopConfig, resolver, testname)
if err != nil {
loggers.Job.WithError(err).Error("Failed to get resolved config for test")
return nil, err
}
apihelper.MergeImageStreamTagMaps(allImageStreamTags, imageStreamTags)
container.Env = append(envs, v1.EnvVar{Name: "CONFIG_SPEC", Value: ciOpConfigContent})
return allImageStreamTags, nil
Comment thread
petr-muller marked this conversation as resolved.
}

Expand Down
186 changes: 128 additions & 58 deletions pkg/rehearse/jobs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,40 +115,40 @@ func generateTestConfigFiles() config.DataByFilename {

var ignoreUnexported = cmpopts.IgnoreUnexported(prowconfig.Presubmit{}, prowconfig.Brancher{}, prowconfig.RegexpChangeMatcher{})

func makeTestingPresubmitForEnv(env []v1.EnvVar) *prowconfig.Presubmit {
return &prowconfig.Presubmit{
JobBase: prowconfig.JobBase{
Agent: "kubernetes",
Name: "test-job-name",
Labels: map[string]string{"pj-rehearse.openshift.io/can-be-rehearsed": "true"},
Spec: &v1.PodSpec{
Containers: []v1.Container{
{Env: env},
func TestInlineCiopConfig(t *testing.T) {
unresolvedConfig := api.ReleaseBuildConfiguration{
Tests: []api.TestStepConfiguration{
{
As: "test1",
MultiStageTestConfiguration: &api.MultiStageTestConfiguration{
Pre: []api.TestStep{{LiteralTestStep: &api.LiteralTestStep{As: "test1-from-unresolved"}}},
},
},
{
As: "test2",
},
},
}
}

func makeCMReference(cmName, key string) *v1.EnvVarSource {
return &v1.EnvVarSource{
ConfigMapKeyRef: &v1.ConfigMapKeySelector{
LocalObjectReference: v1.LocalObjectReference{
Name: cmName,
unresolvedConfigContent, err := yaml.Marshal(&unresolvedConfig)
if err != nil {
t.Fatal("Failed to marshal ci-operator config")
}
test1ConfigFromUnresolved := api.ReleaseBuildConfiguration{
Tests: []api.TestStepConfiguration{
{
As: "test1",
MultiStageTestConfigurationLiteral: &api.MultiStageTestConfigurationLiteral{
Pre: []api.LiteralTestStep{{As: "test1-from-unresolved"}},
},
},
Key: key,
},
}
}

var testCiopConfigInfo = api.Metadata{
Org: "targetOrg",
Repo: "targetRepo",
Branch: "master",
}
test1ConfigContentFromUnresolved, err := yaml.Marshal(&test1ConfigFromUnresolved)
if err != nil {
t.Fatal("Failed to marshal ci-operator config")
}

func TestInlineCiopConfig(t *testing.T) {
testCiopConfig := api.ReleaseBuildConfiguration{
resolvedConfig := api.ReleaseBuildConfiguration{
Tests: []api.TestStepConfiguration{{
As: "test1",
MultiStageTestConfigurationLiteral: &api.MultiStageTestConfigurationLiteral{
Expand All @@ -159,41 +159,104 @@ func TestInlineCiopConfig(t *testing.T) {
}},
}

testCiopConfigTest1 := api.ReleaseBuildConfiguration{Tests: []api.TestStepConfiguration{resolvedConfig.Tests[0]}}
testCiopConfigContentTest1, err := yaml.Marshal(&testCiopConfigTest1)
if err != nil {
t.Fatal("Failed to marshal ci-operator config")
}

testCiopConfigTest2 := api.ReleaseBuildConfiguration{Tests: []api.TestStepConfiguration{resolvedConfig.Tests[1]}}
testCiopConfigContentTest2, err := yaml.Marshal(&testCiopConfigTest2)
if err != nil {
t.Fatal("Failed to marshal ci-operator config")
}

standardMetadata := api.Metadata{Org: "targetOrg", Repo: "targetRepo", Branch: "master"}
incompleteMetadata := api.Metadata{Org: "openshift", Repo: "release"}

makePresubmit := func(command string, env []v1.EnvVar, args []string) *prowconfig.Presubmit {
return &prowconfig.Presubmit{
JobBase: prowconfig.JobBase{
Agent: "kubernetes",
Name: "test-job-name",
Labels: map[string]string{"pj-rehearse.openshift.io/can-be-rehearsed": "true"},
Spec: &v1.PodSpec{
Containers: []v1.Container{
{
Args: args,
Command: []string{command},
Env: env,
},
},
},
},
}
}

configs := config.DataByFilename{
standardMetadata.Basename(): {
Info: config.Info{
Metadata: standardMetadata,
},
Configuration: resolvedConfig,
},
}

testCases := []struct {
description string
testname string
sourceEnv []v1.EnvVar
configs config.DataByFilename
description string

testname string
command string
sourceEnv []v1.EnvVar
metadata api.Metadata

expectedEnv []v1.EnvVar
expectedError bool
expectedImageStreamTagMap apihelper.ImageStreamTagMap
}{{
description: "empty env -> no changes",
configs: config.DataByFilename{},
}, {
description: "no Env.ValueFrom -> no changes",
sourceEnv: []v1.EnvVar{{Name: "T", Value: "V"}},
configs: config.DataByFilename{},
expectedEnv: []v1.EnvVar{{Name: "T", Value: "V"}},
}, {
description: "no Env.ValueFrom.ConfigMapKeyRef -> no changes",
sourceEnv: []v1.EnvVar{{Name: "T", ValueFrom: &v1.EnvVarSource{ResourceFieldRef: &v1.ResourceFieldSelector{}}}},
configs: config.DataByFilename{},
expectedEnv: []v1.EnvVar{{Name: "T", ValueFrom: &v1.EnvVarSource{ResourceFieldRef: &v1.ResourceFieldSelector{}}}},
}, {
description: "CM reference but not ci-operator-configs -> no changes",
sourceEnv: []v1.EnvVar{{Name: "T", ValueFrom: makeCMReference("test-cm", "key")}},
configs: config.DataByFilename{},
expectedEnv: []v1.EnvVar{{Name: "T", ValueFrom: makeCMReference("test-cm", "key")}},
},
}{
{
description: "not a ci-operator job -> no changes",
command: "not-ci-operator",
metadata: standardMetadata,
},
{
description: "ci-operator job with CONFIG_SPEC -> no changes",
sourceEnv: []v1.EnvVar{{Name: "CONFIG_SPEC", Value: "this is kept"}},
metadata: standardMetadata,
expectedEnv: []v1.EnvVar{{Name: "CONFIG_SPEC", Value: "this is kept"}},
},
{
description: "ci-operator job -> adds CONFIG_SPEC with resolved config for the given test (test1)",
testname: "test1",
metadata: standardMetadata,
expectedEnv: []v1.EnvVar{{Name: "CONFIG_SPEC", Value: string(testCiopConfigContentTest1)}},
expectedImageStreamTagMap: apihelper.ImageStreamTagMap{"fancy/willem:first": types.NamespacedName{Namespace: "fancy", Name: "willem:first"}},
},
{
// After DPTP-1685: jobs are not expected to refer to ci-operator CMs directly anymore, so
// rehearsals do not need to support that cases anymore
description: "CM reference to ci-operator-configs -> no changes; test1",
description: "ci-operator job -> adds CONFIG_SPEC with resolved config for the given test (test2)",
testname: "test2",
metadata: standardMetadata,
expectedEnv: []v1.EnvVar{{Name: "CONFIG_SPEC", Value: string(testCiopConfigContentTest2)}},
},
{
description: "ci-operator job with UNRESOLVED_CONFIG -> adds CONFIG_SPEC with resolved config for the given test (test1)",
testname: "test1",
sourceEnv: []v1.EnvVar{{Name: "T", ValueFrom: makeCMReference(testCiopConfigInfo.ConfigMapName(), "filename")}},
configs: config.DataByFilename{"filename": {Info: config.Info{Metadata: testCiopConfigInfo}, Configuration: testCiopConfig}},
expectedEnv: []v1.EnvVar{{Name: "T", ValueFrom: makeCMReference(testCiopConfigInfo.ConfigMapName(), "filename")}},
metadata: standardMetadata,
sourceEnv: []v1.EnvVar{{Name: "UNRESOLVED_CONFIG", Value: string(unresolvedConfigContent)}},
expectedEnv: []v1.EnvVar{{Name: "CONFIG_SPEC", Value: string(test1ConfigContentFromUnresolved)}},
},
{
description: "Incomplete metadata -> error",
testname: "test1",
metadata: incompleteMetadata,
expectedError: true,
},
{
description: "A non-ci-operator jobs with UNRESOLVED_CONFIG should be left untouched",
command: "not-ci-operator",
metadata: standardMetadata,
sourceEnv: []v1.EnvVar{{Name: "UNRESOLVED_CONFIG", Value: "should not change"}},
expectedEnv: []v1.EnvVar{{Name: "UNRESOLVED_CONFIG", Value: "should not change"}},
},
}

Expand All @@ -205,10 +268,17 @@ func TestInlineCiopConfig(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
testLoggers := Loggers{logrus.New(), logrus.New()}
job := makeTestingPresubmitForEnv(tc.sourceEnv)
expectedJob := makeTestingPresubmitForEnv(tc.expectedEnv)
if tc.command == "" {
tc.command = "ci-operator"
}
var args []string
if tc.testname != "" {
args = append(args, fmt.Sprintf("--target=%s", tc.testname))
}
job := makePresubmit(tc.command, tc.sourceEnv, args)
expectedJob := makePresubmit(tc.command, tc.expectedEnv, args)

imageStreamTags, err := inlineCiOpConfig(&job.Spec.Containers[0], tc.configs, resolver, testCiopConfigInfo, tc.testname, testLoggers)
imageStreamTags, err := inlineCiOpConfig(&job.Spec.Containers[0], configs, resolver, tc.metadata, tc.testname, testLoggers)

if tc.expectedError && err == nil {
t.Fatalf("Expected inlineCiopConfig() to return an error, none returned")
Expand Down
Loading