Skip to content
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.

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

Sketch out structure and signatures for inrepoconfig #13342

Closed

Conversation

alvaroaleman
Copy link
Member

This pr demonstrates the structure and function signature I have in mind to use for inrepoconfig. Its purpose at this stage is demonstration only/basis for further discussion as agreed on the meeting on june 27th. Hence, several parts are not actually implemented and the whole thing wont build.

Included are:

  • Required refactorings in prow/config to make the presubmit defaulting and validation usable after startup
  • Getter funcs in prow/config, replacing the existing public Presubmit field which was made private
  • A sample of a simple plugin using the new getter (skip)
  • A sample of a component that can not use the dynamic Presubmits, as its not github-based (prow/gerrit/adapter)
  • Adjustments for the trigger plugin
  • Adjustments for tide
  • Adjustments for the status-reconciler and branchprotector which have no/limited functionality when used in conjunction with inrepoconfig
  • Sample adjustments in Deck

/assign @cjwagner @stevekuznetsov

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 7, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/prow Issues or PRs related to prow area/prow/branchprotector Issues or PRs related to prow's branchprotector component area/prow/deck Issues or PRs related to prow's deck component area/prow/gerrit Issues or PRs related to prow's gerrit component area/prow/tide Issues or PRs related to prow's tide component sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 7, 2019
prow/config/inrepoconfig.go Show resolved Hide resolved
prow/config/inrepoconfig.go Outdated Show resolved Hide resolved
prow/config/inrepoconfig.go Show resolved Hide resolved
return nil, err
}

return append(c.presubmits[org+"/"+repo], inrepoconfig.Presubmits...), nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have logic for merging config from multiple files together. We could adapt that for reuse here instead of doing a simple append to get some additional validation.

prow/config/config.go Outdated Show resolved Hide resolved
@@ -717,6 +718,23 @@ func setPeriodicDecorationDefaults(c *Config, ps *Periodic) {
}
}

func finalizePresubmits(c *Config, presubmits []Presubmit) error {
setPresubmitDecorationDefaults(c, presubmits)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only do this if c.decorationRequested() and we should only check that once since it is somewhat expensive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c.decorationRequested iterates over all jobs and returns true if any of them has Decorate: true.
If it returns true, we check the validity of the DecorationConfig, then iterate over all jobs again and set the decorationConfig on all of them that have decorate: True.

In finalizePresubmits we have to iterate over all presubmits anyways. The additional cost of checking if decorate: true and then setting the decoration config is IMHO neglible.

The main faulty thing here is that setPresubmitDecorationDefaults takes a single presubmit and not a list of them and that finalizePresubmits its currently gated inside c.decorationRequested() which doesn't make any sense, I'll fix that up.


func (a *Agent) presubmits(pr *github.PullRequest) (string, []config.Presubmit, error) {
org, repo := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name
baseSHA, err := c.GitHubClient.GetRef(org, repo, "heads/"+pr.Base.Ref)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be looking up the base SHA here. I think in most or all cases the caller will already have a specific base SHA value to use (this is at least true for the trigger and tide cases) so lets just pass it in.

edit: I see now that you are having this function be responsible for determining the baseSHA to use and then reusing that value. I don't think this really adds any value over config.GetPresubmits() except for the GH comment generation and pruning, but I also don't think we want a generic error comment for all uses of this.
I don't feel super strongly about this, but this seems out of place in the plugin agent and I'm not sure it adds anything besides overly generalized error commenting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have strong feeling about this either, the reason for adding this is that without this, all plugins need to look up the baseSHA and have logic for error reporting if something fails.

With this, all plugins just need a simple

presubmits, err := pluginAgent.Presubmits(pr)
if err != nil {
  return err
}

Regarding the out of place well, we could it into the config package, but it will then need a lot more arguments. Maybe still the more logical choice, thought.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all plugins need to look up the baseSHA and have logic for error reporting if something fails.

Only plugins that actually need the in repo config functionality. Either way I don't think thats worth avoiding, its only a few line of common go boilerplate and I think the flow makes more sense that way.
Moving this to the config package (or just using config.GetPresubmits()) would make a lot more sense IMO. Things like tide shouldn't have to import the plugins package to use this logic. Having to pass some extra args is totally fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only plugins that actually need the in repo config functionality.

Everything that accesses the Presubmits or not? I thought we settled on integrating this everywhere so we don't get weird issues because different components have a different view on the current state of config.

Moving this to the config package (or just using config.GetPresubmits()) would make a lot more sense IMO. Things like tide shouldn't have to import the plugins package to use this logic. Having to pass some extra args is totally fine.

Well, tide works just fine with what is in the config package. But I am fine moving it 👍

inRepoConfigEnabled := c.Config.InRepoConfigConfiguration(org, repo).Enabled
// We do the status context handling here instead of in the PluginAgent to avoid
// updating it once to Pending and then Success/Failed per plugin that accesses the
// Presubmit config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on this? AFAIK we never changed status contexts from the PluginAgent and loading the presubmit config should not have anything to do with posting status contexts. Additionally trigger never sets pending, success, or failure status contexts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we need to set this context but we want to do it only once so we dont use at least two tokens per plugin that accesses the Presubmit config. Hence it can not be in a wrapper because a wrapper will be used by all plugins.

Alternatively, it could also be implemented by a distinct "plugin" that just tries to access the presubmit config and updates the status accordingly.

@@ -260,3 +258,33 @@ func skipRequested(c Client, pr *github.PullRequest, skippedJobs []config.Presub
}
return errorutil.NewAggregate(errors...)
}

func getPresubmits(c Client, pr *github.PullRequest) (string, []Presubmit, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would this function be used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it should be used instead of *PluginAgent.Presubmits.

This is badly designed right now, it should actually be the Presubmits func of triggers Client. I'll keep it for the moment to not hide your comments.


if inrepoconfigEnabled {
// Managed by trigger
required = append(required, "prow-config-parsing")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the presubmit-that's-not-a-presubmit where code in trigger posts a status to your PR if the in-repo config you are suggesting is not valid

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will always get posted to ensure PRs don't get merged if hook is down/missed an event.

We can not really block on actual Presubmits anymore as they can be different for every PR, but this status context is something we can always require.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I'd expected us to just keep using something like the checkconfig presubmit for this. I understand that a user could delete the checkconfig presubmit in their PR to avoid running it, but that would be pretty obvious and intentional.

This might make more sense, but at minimum we shouldn't be doing it for every webhook like Steve said. I think we should discuss this approach and see if we can think of something better before we settle on this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmhm, checkconfig is actually a very nice idea. The presubmit will need to clone both the code repo and the infra repo but that shouldn't be an issue.

Copy link
Contributor

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be clear about the impact to handling every PR event from this feature

prow/cmd/deck/main.go Show resolved Hide resolved
@@ -818,7 +818,7 @@ func verifyOwnersPlugin(cfg *plugins.Configuration) error {

func validateTriggers(cfg *config.Config, pcfg *plugins.Configuration) error {
configuredRepos := sets.NewString()
for orgRepo := range cfg.JobConfig.Presubmits {
for orgRepo := range cfg.GetStaticPresubmitsForAllRepos() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not StaticPresubmits()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean just remove the word Get from the funcs name to be more idiomatic?

prow/cmd/deck/pr_history.go Show resolved Hide resolved

if inrepoconfigEnabled {
// Managed by trigger
required = append(required, "prow-config-parsing")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the presubmit-that's-not-a-presubmit where code in trigger posts a status to your PR if the in-repo config you are suggesting is not valid

prow/config/config.go Outdated Show resolved Hide resolved
baseSHA, presubmits, err := a.presubmits(pr)
if err != nil {
// log error
// Render error comment and create it on Github if it doesn't exist already
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Aren't we posting a status to the PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the status can only tell you that something went wrong but not why

@@ -98,6 +98,10 @@ func handle(gc githubClient, log *logrus.Entry, e *github.GenericCommentEvent, p
}
statuses := combinedStatus.Statuses

presubmits, err := (&pc).Presubmits(pr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to take the address here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that for some unknown reason, a pluginAgent literal gets passed down into the plugins, however all funcs on the pluginAgent have a pointer receiver. Having a mix of pointer receiver and value receiver funcs on the PluginAgent is worse IMHO.

The "real" solution for this is obviously to pass the pluginAgent around as pointer or to make its funcs have value receivers. But since that inconsistency is not introduced here, I wanted something that will work with as little change as possible.

if err != nil {
return err
}
func runRequested(c Client, pr *github.PullRequest, baseSHA string, requestedJobs []config.Presubmit, eventGUID string) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could just update the pr.Base.SHA field and use that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could, but changing the pr object to not reflect anymore what is in the api/ was at time of retrieval feels rather wrong to me.

// * Ignore "tide" and "prow-config-parsing" contexts
// * Ignore contexts whose `target_url` does not start with c.Config.GetJobUrlPrefix()
// * check if found context is in presubmits if not, retire it
// TODO: Can we use this approach everywhere and use it to replace the status-reconciler?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boooooooo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing it explicitly when we change config seems much saner than by default on every PR interaction. What's the impact of listing statuses on ... every webhook?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One additional token per pull request event. If there is a third party updating statuses, also up to an additional token per comment that triggers trigger.

prow/tide/tide.go Outdated Show resolved Hide resolved
Copy link
Member Author

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback @cjwagner @stevekuznetsov , I've addressed all the comments.

The big question from my POV is still if you consider the complexity of the whole thing okay. Is this PR sufficent to be able to answer that question? Should we make another call maybe?

@@ -818,7 +818,7 @@ func verifyOwnersPlugin(cfg *plugins.Configuration) error {

func validateTriggers(cfg *config.Config, pcfg *plugins.Configuration) error {
configuredRepos := sets.NewString()
for orgRepo := range cfg.JobConfig.Presubmits {
for orgRepo := range cfg.GetStaticPresubmitsForAllRepos() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean just remove the word Get from the funcs name to be more idiomatic?

prow/cmd/deck/main.go Show resolved Hide resolved
prow/cmd/deck/pr_history.go Show resolved Hide resolved

if inrepoconfigEnabled {
// Managed by trigger
required = append(required, "prow-config-parsing")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will always get posted to ensure PRs don't get merged if hook is down/missed an event.

We can not really block on actual Presubmits anymore as they can be different for every PR, but this status context is something we can always require.

@@ -717,6 +718,23 @@ func setPeriodicDecorationDefaults(c *Config, ps *Periodic) {
}
}

func finalizePresubmits(c *Config, presubmits []Presubmit) error {
setPresubmitDecorationDefaults(c, presubmits)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c.decorationRequested iterates over all jobs and returns true if any of them has Decorate: true.
If it returns true, we check the validity of the DecorationConfig, then iterate over all jobs again and set the decorationConfig on all of them that have decorate: True.

In finalizePresubmits we have to iterate over all presubmits anyways. The additional cost of checking if decorate: true and then setting the decoration config is IMHO neglible.

The main faulty thing here is that setPresubmitDecorationDefaults takes a single presubmit and not a list of them and that finalizePresubmits its currently gated inside c.decorationRequested() which doesn't make any sense, I'll fix that up.

@@ -98,6 +98,10 @@ func handle(gc githubClient, log *logrus.Entry, e *github.GenericCommentEvent, p
}
statuses := combinedStatus.Statuses

presubmits, err := (&pc).Presubmits(pr)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that for some unknown reason, a pluginAgent literal gets passed down into the plugins, however all funcs on the pluginAgent have a pointer receiver. Having a mix of pointer receiver and value receiver funcs on the PluginAgent is worse IMHO.

The "real" solution for this is obviously to pass the pluginAgent around as pointer or to make its funcs have value receivers. But since that inconsistency is not introduced here, I wanted something that will work with as little change as possible.

if err != nil {
return err
}
func runRequested(c Client, pr *github.PullRequest, baseSHA string, requestedJobs []config.Presubmit, eventGUID string) error {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could, but changing the pr object to not reflect anymore what is in the api/ was at time of retrieval feels rather wrong to me.

@@ -260,3 +258,33 @@ func skipRequested(c Client, pr *github.PullRequest, skippedJobs []config.Presub
}
return errorutil.NewAggregate(errors...)
}

func getPresubmits(c Client, pr *github.PullRequest) (string, []Presubmit, error) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it should be used instead of *PluginAgent.Presubmits.

This is badly designed right now, it should actually be the Presubmits func of triggers Client. I'll keep it for the moment to not hide your comments.

inRepoConfigEnabled := c.Config.InRepoConfigConfiguration(org, repo).Enabled
// We do the status context handling here instead of in the PluginAgent to avoid
// updating it once to Pending and then Success/Failed per plugin that accesses the
// Presubmit config
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we need to set this context but we want to do it only once so we dont use at least two tokens per plugin that accesses the Presubmit config. Hence it can not be in a wrapper because a wrapper will be used by all plugins.

Alternatively, it could also be implemented by a distinct "plugin" that just tries to access the presubmit config and updates the status accordingly.

// * Ignore "tide" and "prow-config-parsing" contexts
// * Ignore contexts whose `target_url` does not start with c.Config.GetJobUrlPrefix()
// * check if found context is in presubmits if not, retire it
// TODO: Can we use this approach everywhere and use it to replace the status-reconciler?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One additional token per pull request event. If there is a third party updating statuses, also up to an additional token per comment that triggers trigger.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alvaroaleman
To complete the pull request process, please assign cjwagner
You can assign the PR to them by writing /assign @cjwagner in a comment when ready.

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

@alvaroaleman: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-test-infra-bazel 13cfb29 link /test pull-test-infra-bazel

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

// * Iterating over all existing contexts that do not have a "Success" status
// * Ignore "tide" and "prow-config-parsing" contexts
// * Ignore contexts whose `target_url` does not start with c.Config.GetJobUrlPrefix()
// * check if found context is in presubmits if not, retire it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to retire optional presubmits, presubmits that are not always_run, or statuses posted from 3rd party services so this isn't sufficient. We also need to be able to retire status contexts that are expected, but don't exist.

I think we should reuse the unsuccessfulContexts logic in Tide using the in-cluster config (not including PR changes) to identify contexts that are blocking merge, including missing contexts. We can then cross reference that with the presubmits we found from the in repo config and retire anything that isn't in the presubmits.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should reuse the unsuccessfulContexts logic in Tide using the in-cluster config (not including PR changes) to identify contexts that are blocking merge, including missing contexts. We can then cross reference that with the presubmits we found from the in repo config and retire anything that isn't in the presubmits.

Oh that is actually great, as it also prevents tide from merging in case the context creation doesn't happen for whatever reason 👍

@cjwagner
Copy link
Member

Something else I hadn't thought of before was that Tide previously assumed that the presubmits required for a batch of PRs is the union of their individual requirements. This is not necessarily the case now. (Consider each PR changing the name of a different presubmit.)

I think this can be addressed by updating accumulateBatch and trigger to actually merge the PRs and use the merged in-repo-config when accumulating and triggering batches, but it is something else to keep in mind.

@alvaroaleman
Copy link
Member Author

Something else I hadn't thought of before was that Tide previously assumed that the presubmits required for a batch of PRs is the union of their individual requirements. This is not necessarily the case now. (Consider each PR changing the name of a different presubmit.)
I think this can be addressed by updating accumulateBatch and trigger to actually merge the PRs and use the merged in-repo-config when accumulating and triggering batches, but it is something else to keep in mind.

Oh, good that you point that out, I only smoke tested a simple "Single PR needs retest" case when I did the prototype. I will have to dig deeper into the tide code base to figure out what needs changing, but generally, batch testing is the reason Presubmits takes a headRefs []string as argument, so that should be doable

@cblecker cblecker removed their request for review September 17, 2019 21:07
@alvaroaleman
Copy link
Member Author

Not needed anymore, this got implemented in the meantime, see https://github.com/kubernetes/test-infra/blob/d8a8d3fed70ff7fc35726304a6d50e4027c9bc89/prow/inrepconfig.md for docs.

@alvaroaleman alvaroaleman deleted the inrepoconfig-sketch branch December 7, 2019 21:39
@k8s-ci-robot
Copy link
Contributor

@alvaroaleman: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prow/branchprotector Issues or PRs related to prow's branchprotector component area/prow/deck Issues or PRs related to prow's deck component area/prow/gerrit Issues or PRs related to prow's gerrit component area/prow/tide Issues or PRs related to prow's tide component area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants