From 798d892d7e740d4fb91cc4443bb687d35e774a5f Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 30 Apr 2019 23:23:38 -0400 Subject: [PATCH 1/2] Move standard helpers into the api package so tests can use them --- cmd/ci-operator/main.go | 87 ++-------------------------------------- pkg/api/helpers.go | 88 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 84 deletions(-) create mode 100644 pkg/api/helpers.go diff --git a/cmd/ci-operator/main.go b/cmd/ci-operator/main.go index 22ccf230..f8798403 100644 --- a/cmd/ci-operator/main.go +++ b/cmd/ci-operator/main.go @@ -6,10 +6,8 @@ import ( "encoding/base32" "encoding/json" "encoding/xml" - "errors" "flag" "fmt" - "io" "io/ioutil" "log" "os" @@ -426,7 +424,7 @@ func (o *options) Run() error { } if o.print { - if err := printDigraph(os.Stdout, buildSteps); err != nil { + if err := api.PrintDigraph(os.Stdout, buildSteps); err != nil { return fmt.Errorf("could not print graph: %v", err) } return nil @@ -977,91 +975,12 @@ func jobSpecFromGitRef(ref string) (*api.JobSpec, error) { return &api.JobSpec{Type: api.PeriodicJob, Job: "dev", Refs: &api.Refs{Org: prefix[0], Repo: prefix[1], BaseRef: parts[1], BaseSHA: sha}}, nil } -func nodeNames(nodes []*api.StepNode) []string { - var names []string - for _, node := range nodes { - name := node.Step.Name() - if len(name) == 0 { - name = fmt.Sprintf("<%T>", node.Step) - } - names = append(names, name) - } - return names -} - -func linkNames(links []api.StepLink) []string { - var names []string - for _, link := range links { - name := fmt.Sprintf("<%#v>", link) - names = append(names, name) - } - return names -} - -func topologicalSort(nodes []*api.StepNode) ([]*api.StepNode, error) { - var sortedNodes []*api.StepNode - var satisfied []api.StepLink - seen := make(map[api.Step]struct{}) - for len(nodes) > 0 { - var changed bool - var waiting []*api.StepNode - for _, node := range nodes { - for _, child := range node.Children { - if _, ok := seen[child.Step]; !ok { - waiting = append(waiting, child) - } - } - if _, ok := seen[node.Step]; ok { - continue - } - if !api.HasAllLinks(node.Step.Requires(), satisfied) { - waiting = append(waiting, node) - continue - } - satisfied = append(satisfied, node.Step.Creates()...) - sortedNodes = append(sortedNodes, node) - seen[node.Step] = struct{}{} - changed = true - } - if !changed && len(waiting) > 0 { - for _, node := range waiting { - var missing []api.StepLink - for _, link := range node.Step.Requires() { - if !api.HasAllLinks([]api.StepLink{link}, satisfied) { - missing = append(missing, link) - } - log.Printf("step <%T> is missing dependencies: %s", node.Step, strings.Join(linkNames(missing), ", ")) - } - } - return nil, errors.New("steps are missing dependencies") - } - nodes = waiting - } - return sortedNodes, nil -} - -func printDigraph(w io.Writer, steps []api.Step) error { - for _, step := range steps { - for _, other := range steps { - if step == other { - continue - } - if api.HasAnyLinks(step.Requires(), other.Creates()) { - if _, err := fmt.Fprintf(w, "%s %s\n", step.Name(), other.Name()); err != nil { - return err - } - } - } - } - return nil -} - func printExecutionOrder(nodes []*api.StepNode) error { - ordered, err := topologicalSort(nodes) + ordered, err := api.TopologicalSort(nodes) if err != nil { return fmt.Errorf("could not sort nodes: %v", err) } - log.Printf("Running %s", strings.Join(nodeNames(ordered), ", ")) + log.Printf("Running %s", strings.Join(api.NodeNames(ordered), ", ")) return nil } diff --git a/pkg/api/helpers.go b/pkg/api/helpers.go new file mode 100644 index 00000000..68db3486 --- /dev/null +++ b/pkg/api/helpers.go @@ -0,0 +1,88 @@ +package api + +import ( + "errors" + "fmt" + "io" + "log" + "strings" +) + +func NodeNames(nodes []*StepNode) []string { + var names []string + for _, node := range nodes { + name := node.Step.Name() + if len(name) == 0 { + name = fmt.Sprintf("<%T>", node.Step) + } + names = append(names, name) + } + return names +} + +func LinkNames(links []StepLink) []string { + var names []string + for _, link := range links { + name := fmt.Sprintf("<%#v>", link) + names = append(names, name) + } + return names +} + +func TopologicalSort(nodes []*StepNode) ([]*StepNode, error) { + var sortedNodes []*StepNode + var satisfied []StepLink + seen := make(map[Step]struct{}) + for len(nodes) > 0 { + var changed bool + var waiting []*StepNode + for _, node := range nodes { + for _, child := range node.Children { + if _, ok := seen[child.Step]; !ok { + waiting = append(waiting, child) + } + } + if _, ok := seen[node.Step]; ok { + continue + } + if !HasAllLinks(node.Step.Requires(), satisfied) { + waiting = append(waiting, node) + continue + } + satisfied = append(satisfied, node.Step.Creates()...) + sortedNodes = append(sortedNodes, node) + seen[node.Step] = struct{}{} + changed = true + } + if !changed && len(waiting) > 0 { + for _, node := range waiting { + var missing []StepLink + for _, link := range node.Step.Requires() { + if !HasAllLinks([]StepLink{link}, satisfied) { + missing = append(missing, link) + } + log.Printf("step <%T> is missing dependencies: %s", node.Step, strings.Join(LinkNames(missing), ", ")) + } + } + return nil, errors.New("steps are missing dependencies") + } + nodes = waiting + } + return sortedNodes, nil +} + +func PrintDigraph(w io.Writer, steps []Step) error { + for _, step := range steps { + for _, other := range steps { + if step == other { + continue + } + if HasAnyLinks(step.Requires(), other.Creates()) { + if _, err := fmt.Fprintf(w, "%s %s\n", step.Name(), other.Name()); err != nil { + return err + } + } + } + } + return nil +} From 07146a960c9fbab2cfbca7650a3eca9ee5f18230 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 30 Apr 2019 23:23:56 -0400 Subject: [PATCH 2/2] When --template is specified, ignore whatever test with that name is defined As we move templates into the ci-operator, we need a way to stage the changes for sets of jobs and move gradually. When a user provides --template on the command line, ignore any job with the same name so that we can begin moving those to internal definitions. --- pkg/defaults/defaults.go | 20 +++- pkg/defaults/defaults_test.go | 166 +++++++++++++++++++++++++++++++++- 2 files changed, 183 insertions(+), 3 deletions(-) diff --git a/pkg/defaults/defaults.go b/pkg/defaults/defaults.go index f5d59518..535b6b57 100644 --- a/pkg/defaults/defaults.go +++ b/pkg/defaults/defaults.go @@ -7,6 +7,8 @@ import ( "net/url" "strings" + "k8s.io/apimachinery/pkg/util/sets" + "github.com/openshift/ci-operator/pkg/steps/clusterinstall" appsclientset "k8s.io/client-go/kubernetes/typed/apps/v1" @@ -41,9 +43,17 @@ func FromConfig( var buildSteps []api.Step var postSteps []api.Step - requiredNames := make(map[string]struct{}) + requiredNames := sets.NewString() for _, target := range requiredTargets { - requiredNames[target] = struct{}{} + requiredNames.Insert(target) + } + + // if a user specifies a template, overwrite what is defined in the config to allow + // jobs to gradually transition from external to ci-operator to being provided as + // steps + excludedTests := sets.NewString() + for _, target := range templates { + excludedTests.Insert(target.Name) } var buildClient steps.BuildClient @@ -155,6 +165,9 @@ func FromConfig( buildSteps = append(buildSteps, initialReleaseStep) } else if rawStep.TestStepConfiguration != nil && rawStep.TestStepConfiguration.OpenshiftInstallerClusterTestConfiguration != nil && rawStep.TestStepConfiguration.OpenshiftInstallerClusterTestConfiguration.Upgrade { + if excludedTests.Has(rawStep.TestStepConfiguration.As) { + continue + } var err error step, err = clusterinstall.E2ETestStep(*rawStep.TestStepConfiguration.OpenshiftInstallerClusterTestConfiguration, *rawStep.TestStepConfiguration, params, podClient, templateClient, secretGetter, artifactDir, jobSpec) if err != nil { @@ -162,6 +175,9 @@ func FromConfig( } } else if rawStep.TestStepConfiguration != nil { + if excludedTests.Has(rawStep.TestStepConfiguration.As) { + continue + } step = steps.TestStep(*rawStep.TestStepConfiguration, config.Resources, podClient, artifactDir, jobSpec) } diff --git a/pkg/defaults/defaults_test.go b/pkg/defaults/defaults_test.go index 38999ead..0d2b8617 100644 --- a/pkg/defaults/defaults_test.go +++ b/pkg/defaults/defaults_test.go @@ -6,8 +6,13 @@ import ( "reflect" "testing" - "github.com/openshift/ci-operator/pkg/api" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/diff" + "k8s.io/client-go/rest" + + templateapi "github.com/openshift/api/template/v1" + + "github.com/openshift/ci-operator/pkg/api" ) func addCloneRefs(cfg *api.SourceStepConfiguration) *api.SourceStepConfiguration { @@ -470,3 +475,162 @@ func formatStep(step api.StepConfiguration) string { func formatReference(ref api.ImageStreamTagReference) string { return fmt.Sprintf("%s/%s:%s (as:%s)", ref.Namespace, ref.Name, ref.Tag, ref.As) } + +func TestFromConfig(t *testing.T) { + tests := []struct { + name string + + config *api.ReleaseBuildConfiguration + jobSpec *api.JobSpec + templates []*templateapi.Template + paramFile string + artifactDir string + promote bool + clusterConfig *rest.Config + requiredTargets []string + + wantGraph bool + want []string + wantPost []string + wantErr bool + }{ + { + config: &api.ReleaseBuildConfiguration{ + InputConfiguration: api.InputConfiguration{ + BuildRootImage: &api.BuildRootImageConfiguration{ + ImageStreamTagReference: &api.ImageStreamTagReference{Tag: "manual"}, + }, + BaseRPMImages: map[string]api.ImageStreamTagReference{ + "name": { + Namespace: "namespace", + Name: "name", + Tag: "tag", + }, + }, + }, + }, + jobSpec: &api.JobSpec{ + Refs: &api.Refs{ + Org: "org", + Repo: "repo", + }, + BaseNamespace: "base-1", + }, + want: []string{"[input:root]", "src", "[input:name-without-rpms]", "name", "[output-images]", "[images]"}, + }, + + { + name: "a test referencing an image and a root defines a valid graph", + + config: &api.ReleaseBuildConfiguration{ + InputConfiguration: api.InputConfiguration{ + BuildRootImage: &api.BuildRootImageConfiguration{ + ImageStreamTagReference: &api.ImageStreamTagReference{Tag: "manual"}, + }, + }, + Images: []api.ProjectDirectoryImageBuildStepConfiguration{ + { + From: api.PipelineImageStreamTagReference("root"), + To: api.PipelineImageStreamTagReference("name"), + }, + }, + Tests: []api.TestStepConfiguration{ + { + As: "e2e-aws", + ContainerTestConfiguration: &api.ContainerTestConfiguration{ + From: "name", + }, + }, + }, + }, + jobSpec: &api.JobSpec{ + Refs: &api.Refs{ + Org: "org", + Repo: "repo", + }, + BaseNamespace: "base-1", + }, + + wantGraph: true, + want: []string{"[input:root]", "[output-images]", "src", "name", "[output:stable:name]", "e2e-aws", "[images]"}, + }, + + { + name: "specifying a template overrides the step from the config", + + config: &api.ReleaseBuildConfiguration{ + InputConfiguration: api.InputConfiguration{ + BuildRootImage: &api.BuildRootImageConfiguration{ + ImageStreamTagReference: &api.ImageStreamTagReference{Tag: "manual"}, + }, + }, + Images: []api.ProjectDirectoryImageBuildStepConfiguration{ + { + From: api.PipelineImageStreamTagReference("root"), + To: api.PipelineImageStreamTagReference("name"), + }, + }, + Tests: []api.TestStepConfiguration{ + { + As: "e2e-aws", + ContainerTestConfiguration: &api.ContainerTestConfiguration{ + From: "name", + }, + }, + }, + }, + jobSpec: &api.JobSpec{ + Refs: &api.Refs{ + Org: "org", + Repo: "repo", + }, + BaseNamespace: "base-1", + }, + templates: []*templateapi.Template{ + { + ObjectMeta: meta.ObjectMeta{Name: "e2e-aws"}, + }, + }, + + wantGraph: true, + want: []string{"[input:root]", "e2e-aws", "[output-images]", "src", "name", "[output:stable:name]", "[images]"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, got1, err := FromConfig(tt.config, tt.jobSpec, tt.templates, tt.paramFile, tt.artifactDir, tt.promote, tt.clusterConfig, tt.requiredTargets) + if (err != nil) != tt.wantErr { + t.Errorf("FromConfig() error = %v, wantErr %v", err, tt.wantErr) + return + } + + var names []string + if tt.wantGraph { + // verify we can build a graph from the result + graph := api.BuildGraph(got) + sorted, err := api.TopologicalSort(graph) + if err != nil { + t.Fatalf("unexpected error sorting steps: %v", err) + } + for _, node := range sorted { + names = append(names, node.Step.Name()) + } + } else { + for _, step := range got { + names = append(names, step.Name()) + } + } + if !reflect.DeepEqual(names, tt.want) { + t.Errorf("\n%v\n%v", names, tt.want) + } + + names = nil + for _, step := range got1 { + names = append(names, step.Name()) + } + if !reflect.DeepEqual(names, tt.wantPost) { + t.Errorf("\n%v\n%v", names, tt.wantPost) + } + }) + } +}