diff --git a/cmd/pj-rehearse/main.go b/cmd/pj-rehearse/main.go index 21fd660639e..66a402d164e 100644 --- a/cmd/pj-rehearse/main.go +++ b/cmd/pj-rehearse/main.go @@ -183,7 +183,7 @@ func rehearseMain() error { affectedJobs = jobs } - var changedRegistrySteps registry.NodeByName + var changedRegistrySteps []registry.Node var refs registry.ReferenceByName var chains registry.ChainByName var workflows registry.WorkflowByName @@ -214,8 +214,8 @@ func rehearseMain() error { } if len(changedRegistrySteps) != 0 { var names []string - for step := range changedRegistrySteps { - names = append(names, step) + for _, step := range changedRegistrySteps { + names = append(names, step.Name()) } logger.Infof("found %d changed registry steps: %s", len(changedRegistrySteps), strings.Join(names, ", ")) } diff --git a/pkg/config/release.go b/pkg/config/release.go index 1503a3e07d4..a6ce68dc936 100644 --- a/pkg/config/release.go +++ b/pkg/config/release.go @@ -166,43 +166,41 @@ func GetChangedTemplates(path, baseRev string) ([]ConfigMapSource, error) { return ret, nil } -func loadRegistryStep(filename string, graph, changes registry.NodeByName) error { +func loadRegistryStep(filename string, graph registry.NodeByName) (registry.Node, error) { // if a commands script changed, mark reference as changed filename = strings.ReplaceAll(filename, "-commands.sh", "-ref.yaml") - name := "" - if strings.HasSuffix(filename, "-ref.yaml") { - name = strings.TrimSuffix(filename, "-ref.yaml") + var node registry.Node + var ok bool + switch { + case strings.HasSuffix(filename, "-ref.yaml"): + node, ok = graph.References[strings.TrimSuffix(filename, "-ref.yaml")] + case strings.HasSuffix(filename, "-chain.yaml"): + node, ok = graph.Chains[strings.TrimSuffix(filename, "-chain.yaml")] + case strings.HasSuffix(filename, "-workflow.yaml"): + node, ok = graph.Workflows[strings.TrimSuffix(filename, "-workflow.yaml")] + default: + return nil, fmt.Errorf("invalid step filename: %s", filename) } - if strings.HasSuffix(filename, "-chain.yaml") { - name = strings.TrimSuffix(filename, "-chain.yaml") - } - if strings.HasSuffix(filename, "-workflow.yaml") { - name = strings.TrimSuffix(filename, "-workflow.yaml") - } - if name == "" { - return fmt.Errorf("invalid step filename: %s", filename) - } - node, ok := graph[name] if !ok { - return fmt.Errorf("could not find registry component in registry graph: %s", name) + return nil, fmt.Errorf("could not find registry component in registry graph: %s", filename) } - changes[name] = node - return nil + return node, nil } // GetChangedRegistrySteps identifies all registry components (refs, chains, and workflows) that changed. -func GetChangedRegistrySteps(path, baseRev string, graph registry.NodeByName) (registry.NodeByName, error) { - changes := make(registry.NodeByName) +func GetChangedRegistrySteps(path, baseRev string, graph registry.NodeByName) ([]registry.Node, error) { + var changes []registry.Node revChanges, err := getRevChanges(path, RegistryPath, baseRev, true) if err != nil { return changes, err } for _, c := range revChanges { if filepath.Ext(c.Filename) == ".yaml" || strings.HasSuffix(c.Filename, "-commands.sh") { - err := loadRegistryStep(filepath.Base(c.Filename), graph, changes) + node, err := loadRegistryStep(filepath.Base(c.Filename), graph) if err != nil { return changes, err } + changes = append(changes, node) } } return changes, nil diff --git a/pkg/registry/graph.go b/pkg/registry/graph.go index 3d0eb9444c4..c7f547e4e8b 100644 --- a/pkg/registry/graph.go +++ b/pkg/registry/graph.go @@ -22,17 +22,21 @@ type Node interface { // Type returns the type of the registry element a Node refers to Type() Type // AncestorNames returns a set of strings containing the names of all of the node's ancestors - AncestorNames() sets.String + Ancestors() []Node // DescendantNames returns a set of strings containing the names of all of the node's descendants - DescendantNames() sets.String + Descendants() []Node // ParentNames returns a set of strings containing the names of all the node's parents - ParentNames() sets.String + Parents() []Node // ChildrenNames returns a set of strings containing the names of all the node's children - ChildrenNames() sets.String + Childrens() []Node } // NodeByName provides a mapping from node name to the Node interface -type NodeByName map[string]Node +type NodeByName struct { + References map[string]Node + Chains map[string]Node + Workflows map[string]Node +} type nodeWithName struct { name string @@ -107,51 +111,51 @@ func (*referenceNode) Type() Type { return Reference } -func (n *nodeWithParents) ParentNames() sets.String { - parents := sets.NewString() +func (n *nodeWithParents) Parents() []Node { + var parents []Node for parent := range n.workflowParents { - parents.Insert(parent.Name()) + parents = append(parents, parent) } for parent := range n.chainParents { - parents.Insert(parent.Name()) + parents = append(parents, parent) } return parents } -func (*workflowNode) ParentNames() sets.String { return sets.NewString() } +func (*workflowNode) Parents() []Node { return []Node{} } -func (n *nodeWithChildren) ChildrenNames() sets.String { - children := sets.NewString() +func (n *nodeWithChildren) Childrens() []Node { + var children []Node for child := range n.referenceChildren { - children.Insert(child.Name()) + children = append(children, child) } for child := range n.chainChildren { - children.Insert(child.Name()) + children = append(children, child) } return children } -func (*referenceNode) ChildrenNames() sets.String { return sets.NewString() } +func (*referenceNode) Childrens() []Node { return []Node{} } -func (n *nodeWithParents) AncestorNames() sets.String { - ancestors := n.ParentNames() +func (n *nodeWithParents) Ancestors() []Node { + ancestors := n.Parents() for parent := range n.chainParents { - ancestors.Insert(parent.AncestorNames().List()...) + ancestors = append(ancestors, parent.Ancestors()...) } return ancestors } -func (*workflowNode) AncestorNames() sets.String { return sets.NewString() } +func (*workflowNode) Ancestors() []Node { return []Node{} } -func (n *nodeWithChildren) DescendantNames() sets.String { - descendants := n.ChildrenNames() +func (n *nodeWithChildren) Descendants() []Node { + descendants := n.Childrens() for child := range n.chainChildren { - descendants.Insert(child.DescendantNames().List()...) + descendants = append(descendants, child.Descendants()...) } return descendants } -func (*referenceNode) DescendantNames() sets.String { return sets.NewString() } +func (*referenceNode) Descendants() []Node { return []Node{} } func (n *workflowNode) addChainChild(child *chainNode) { n.chainChildren.insert(child) @@ -216,7 +220,11 @@ func hasCycles(node *chainNode, ancestors sets.String, traversedPath []string) e // NewGraph returns a NodeByType map representing the provided step references, chains, and workflows as a directed graph. func NewGraph(stepsByName ReferenceByName, chainsByName ChainByName, workflowsByName WorkflowByName) (NodeByName, error) { - nodesByName := make(NodeByName) + nodesByName := NodeByName{ + References: make(map[string]Node), + Chains: make(map[string]Node), + Workflows: make(map[string]Node), + } // References can only be children; load them so they can be added as children by workflows and chains referenceNodes := make(referenceNodeByName) for name := range stepsByName { @@ -225,10 +233,10 @@ func NewGraph(stepsByName ReferenceByName, chainsByName ChainByName, workflowsBy nodeWithParents: newNodeWithParents(), } referenceNodes[name] = node - nodesByName[name] = node + nodesByName.References[name] = node } // since we may load the parent chain before a child chain, we need to make the parent->child links after loading all chains - parentChildChain := make(map[*chainNode]string) + parentChildChain := make(map[*chainNode][]string) chainNodes := make(chainNodeByName) for name, chain := range chainsByName { node := &chainNode{ @@ -237,29 +245,31 @@ func NewGraph(stepsByName ReferenceByName, chainsByName ChainByName, workflowsBy nodeWithParents: newNodeWithParents(), } chainNodes[name] = node - nodesByName[name] = node + nodesByName.Chains[name] = node for _, step := range chain { if step.Reference != nil { if _, exists := referenceNodes[*step.Reference]; !exists { - return nil, fmt.Errorf("Chain %s contains non-existent reference %s", name, *step.Reference) + return nodesByName, fmt.Errorf("Chain %s contains non-existent reference %s", name, *step.Reference) } node.addReferenceChild(referenceNodes[*step.Reference]) } if step.Chain != nil { - parentChildChain[node] = *step.Chain + parentChildChain[node] = append(parentChildChain[node], *step.Chain) } } } - for parent, child := range parentChildChain { - if _, exists := chainNodes[child]; !exists { - return nil, fmt.Errorf("Chain %s contains non-existent chain %s", parent.Name(), child) + for parent, children := range parentChildChain { + for _, child := range children { + if _, exists := chainNodes[child]; !exists { + return nodesByName, fmt.Errorf("Chain %s contains non-existent chain %s", parent.Name(), child) + } + parent.addChainChild(chainNodes[child]) } - parent.addChainChild(chainNodes[child]) } // verify that no cycles exist for _, chain := range chainNodes { if err := hasCycles(chain, sets.NewString(), []string{}); err != nil { - return nil, err + return nodesByName, err } } workflowNodes := make(workflowNodeByName) @@ -269,18 +279,18 @@ func NewGraph(stepsByName ReferenceByName, chainsByName ChainByName, workflowsBy nodeWithChildren: newNodeWithChildren(), } workflowNodes[name] = node - nodesByName[name] = node + nodesByName.Workflows[name] = node steps := append(workflow.Pre, append(workflow.Test, workflow.Post...)...) for _, step := range steps { if step.Reference != nil { if _, exists := referenceNodes[*step.Reference]; !exists { - return nil, fmt.Errorf("Workflow %s contains non-existent reference %s", name, *step.Reference) + return nodesByName, fmt.Errorf("Workflow %s contains non-existent reference %s", name, *step.Reference) } node.addReferenceChild(referenceNodes[*step.Reference]) } if step.Chain != nil { if _, exists := chainNodes[*step.Chain]; !exists { - return nil, fmt.Errorf("Workflow %s contains non-existent chain %s", name, *step.Chain) + return nodesByName, fmt.Errorf("Workflow %s contains non-existent chain %s", name, *step.Chain) } node.addChainChild(chainNodes[*step.Chain]) } diff --git a/pkg/registry/graph_test.go b/pkg/registry/graph_test.go index 80240d985cb..54d7610524d 100644 --- a/pkg/registry/graph_test.go +++ b/pkg/registry/graph_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/openshift/ci-tools/pkg/api" - "k8s.io/apimachinery/pkg/util/sets" ) // These maps contain the representation of ../../test/multistage-registry/registry with the @@ -16,14 +15,24 @@ var ipiDeprovisionDeprovision = "ipi-deprovision-deprovision" var ipiInstall = "ipi-install" var ipiDeprovision = "ipi-deprovision" var ipi = "ipi" +var nested = "nested" +var ipiConf = "ipi-conf" +var ipiConfAWS = "ipi-conf-aws" var referenceMap = ReferenceByName{ ipiInstallInstall: {}, ipiInstallRBAC: {}, ipiDeprovisionDeprovision: {}, ipiDeprovisionMustGather: {}, + ipiConf: {}, + ipiConfAWS: {}, } var chainMap = ChainByName{ + ipiConfAWS: {{ + Reference: &ipiConf, + }, { + Reference: &ipiConfAWS, + }}, ipiInstall: {{ Reference: &ipiInstallInstall, }, { @@ -34,6 +43,11 @@ var chainMap = ChainByName{ }, { Reference: &ipiDeprovisionDeprovision, }}, + nested: {{ + Chain: &ipiInstall, + }, { + Chain: &ipiDeprovision, + }}, } var workflowMap = WorkflowByName{ ipi: { @@ -46,23 +60,54 @@ var workflowMap = WorkflowByName{ }, } -func TestAncestorNames(t *testing.T) { +func nodesEqual(x, y []Node) bool { + if len(x) != len(y) { + return false + } + for _, xNode := range x { + found := false + for _, yNode := range y { + if xNode.Name() == yNode.Name() && xNode.Type() == yNode.Type() { + found = true + break + } + } + if !found { + return false + } + } + return true +} + +func TestAncestors(t *testing.T) { testCases := []struct { - name string - set sets.String + name string + nodeType Type + expected []Node }{{ - name: ipi, - set: sets.String{}, + name: ipi, + nodeType: Workflow, + expected: []Node{}, + }, { + name: ipiInstall, + nodeType: Chain, + expected: []Node{ + &workflowNode{nodeWithName: nodeWithName{name: ipi}}, + &chainNode{nodeWithName: nodeWithName{name: nested}}, + }, }, { - name: ipiInstall, - set: sets.String{ - ipi: sets.Empty{}, + name: ipiDeprovisionMustGather, + nodeType: Reference, + expected: []Node{ + &workflowNode{nodeWithName: nodeWithName{name: ipi}}, + &chainNode{nodeWithName: nodeWithName{name: ipiDeprovision}}, + &chainNode{nodeWithName: nodeWithName{name: nested}}, }, }, { - name: ipiDeprovisionMustGather, - set: sets.String{ - ipi: sets.Empty{}, - ipiDeprovision: sets.Empty{}, + name: ipiConfAWS, + nodeType: Reference, + expected: []Node{ + &chainNode{nodeWithName: nodeWithName{name: ipiConfAWS}}, }, }} @@ -71,36 +116,66 @@ func TestAncestorNames(t *testing.T) { t.Fatalf("failed to create graph: %v", err) } for _, testCase := range testCases { - node := graph[testCase.name] - if !testCase.set.Equal(node.AncestorNames()) { + var node Node + switch testCase.nodeType { + case Reference: + node = graph.References[testCase.name] + case Chain: + node = graph.Chains[testCase.name] + case Workflow: + node = graph.Workflows[testCase.name] + } + if !nodesEqual(node.Ancestors(), testCase.expected) { t.Errorf("%s: ancestor sets not equal", testCase.name) } } } -func TestDescendantNames(t *testing.T) { +func TestDescendants(t *testing.T) { testCases := []struct { - name string - set sets.String + name string + nodeType Type + expected []Node }{{ - name: ipi, - set: sets.String{ - ipiInstall: sets.Empty{}, - ipiInstallInstall: sets.Empty{}, - ipiInstallRBAC: sets.Empty{}, - ipiDeprovision: sets.Empty{}, - ipiDeprovisionMustGather: sets.Empty{}, - ipiDeprovisionDeprovision: sets.Empty{}, + name: ipi, + nodeType: Workflow, + expected: []Node{ + &chainNode{nodeWithName: nodeWithName{name: ipiInstall}}, + &referenceNode{nodeWithName: nodeWithName{name: ipiInstallInstall}}, + &referenceNode{nodeWithName: nodeWithName{name: ipiInstallRBAC}}, + &chainNode{nodeWithName: nodeWithName{name: ipiDeprovision}}, + &referenceNode{nodeWithName: nodeWithName{name: ipiDeprovisionMustGather}}, + &referenceNode{nodeWithName: nodeWithName{name: ipiDeprovisionDeprovision}}, }, }, { - name: ipiInstall, - set: sets.String{ - ipiInstallInstall: sets.Empty{}, - ipiInstallRBAC: sets.Empty{}, + name: ipiInstall, + nodeType: Chain, + expected: []Node{ + &referenceNode{nodeWithName: nodeWithName{name: ipiInstallInstall}}, + &referenceNode{nodeWithName: nodeWithName{name: ipiInstallRBAC}}, }, }, { - name: ipiDeprovisionMustGather, - set: sets.String{}, + name: ipiDeprovisionMustGather, + nodeType: Reference, + expected: []Node{}, + }, { + name: nested, + nodeType: Chain, + expected: []Node{ + &chainNode{nodeWithName: nodeWithName{name: ipiInstall}}, + &referenceNode{nodeWithName: nodeWithName{name: ipiInstallInstall}}, + &referenceNode{nodeWithName: nodeWithName{name: ipiInstallRBAC}}, + &chainNode{nodeWithName: nodeWithName{name: ipiDeprovision}}, + &referenceNode{nodeWithName: nodeWithName{name: ipiDeprovisionMustGather}}, + &referenceNode{nodeWithName: nodeWithName{name: ipiDeprovisionDeprovision}}, + }, + }, { + name: ipiConfAWS, + nodeType: Chain, + expected: []Node{ + &referenceNode{nodeWithName: nodeWithName{name: ipiConfAWS}}, + &referenceNode{nodeWithName: nodeWithName{name: ipiConf}}, + }, }} graph, err := NewGraph(referenceMap, chainMap, workflowMap) @@ -108,8 +183,16 @@ func TestDescendantNames(t *testing.T) { t.Fatalf("failed to create graph: %v", err) } for _, testCase := range testCases { - node := graph[testCase.name] - if !testCase.set.Equal(node.DescendantNames()) { + var node Node + switch testCase.nodeType { + case Reference: + node = graph.References[testCase.name] + case Chain: + node = graph.Chains[testCase.name] + case Workflow: + node = graph.Workflows[testCase.name] + } + if !nodesEqual(node.Descendants(), testCase.expected) { t.Errorf("%s: descendant sets not equal", testCase.name) } } diff --git a/pkg/rehearse/jobs.go b/pkg/rehearse/jobs.go index a237d143d0e..17dbdaaf38a 100644 --- a/pkg/rehearse/jobs.go +++ b/pkg/rehearse/jobs.go @@ -576,33 +576,44 @@ func getPresubmitsForRegistryStep(node registry.Node, configs config.DataByFilen return toTest, addedConfigs, nil } -// expandAncestors takes a graph of changed steps and adds all ancestors of -// the existing steps to the changed steps graph -func expandAncestors(changed, graph registry.NodeByName) { +// sorting functions for []registry.Node +type nodeArr []registry.Node + +func (s nodeArr) Len() int { + return len(s) +} +func (s nodeArr) Swap(i, j int) { + s[i], s[j] = s[j], s[i] +} +func (s nodeArr) Less(i, j int) bool { + if s[i].Name() == s[j].Name() { + return s[i].Type() < s[j].Type() + } + return s[i].Name() < s[j].Name() +} + +// getAllAncestors takes a graph of changed steps and finds all ancestors of +// the existing steps in changed +func getAllAncestors(changed []registry.Node) []registry.Node { + var ancestors []registry.Node for _, node := range changed { - for name := range node.AncestorNames() { - changed[name] = graph[name] - } + ancestors = append(ancestors, node.Ancestors()...) } + return ancestors } -func AddRandomJobsForChangedRegistry(regSteps, graph registry.NodeByName, prConfigPresubmits map[string][]prowconfig.Presubmit, configPath string, loggers Loggers) config.Presubmits { +func AddRandomJobsForChangedRegistry(regSteps []registry.Node, graph registry.NodeByName, prConfigPresubmits map[string][]prowconfig.Presubmit, configPath string, loggers Loggers) config.Presubmits { configsByFilename, err := config.LoadDataByFilename(configPath) if err != nil { loggers.Debug.Errorf("Failed to load config by filename in AddRandomJobsForChangedRegistry: %v", err) } - expandAncestors(regSteps, graph) + regSteps = append(regSteps, getAllAncestors(regSteps)...) rehearsals := make(config.Presubmits) - // get sorted list of regSteps keys to make the function deterministic - var keys []string - for k := range regSteps { - keys = append(keys, k) - } - sort.Strings(keys) + // sort steps to make it deterministic + sort.Sort(nodeArr(regSteps)) // make list to store MultiStageTestConfigurations that we've already added to the test list addedConfigs := []*api.MultiStageTestConfiguration{} - for _, key := range keys { - step := regSteps[key] + for _, step := range regSteps { var presubmitsMap map[string][]prowconfig.Presubmit presubmitsMap, addedConfigs, err = getPresubmitsForRegistryStep(step, configsByFilename, prConfigPresubmits, addedConfigs) if err != nil { @@ -614,7 +625,7 @@ func AddRandomJobsForChangedRegistry(regSteps, graph registry.NodeByName, prConf } for repo, presubmits := range presubmitsMap { for _, job := range presubmits { - selectionFields := logrus.Fields{diffs.LogRepo: repo, diffs.LogJobName: job.Name, diffs.LogReasons: fmt.Sprintf("registry step %s changed", key)} + selectionFields := logrus.Fields{diffs.LogRepo: repo, diffs.LogJobName: job.Name, diffs.LogReasons: fmt.Sprintf("registry step %s changed", step.Name())} loggers.Job.WithFields(selectionFields).Info(diffs.ChosenJob) } rehearsals[repo] = append(rehearsals[repo], presubmits...)