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

Implement tide and replace the submit queue #3866

Closed
spxtr opened this Issue Aug 2, 2017 · 46 comments

Comments

Projects
None yet
@spxtr
Copy link
Member

spxtr commented Aug 2, 2017

Design doc here, requires you to be a member of either kubernetes-dev or kubernetes-sig-testing. Please comment there with overall design decisions rather than on this issue.

Steps:

  • Implement tide's core loop for serial jobs. Start it up in dry run mode where it simply prints actions it would take. #4113
  • Implement batches. #4479
  • Turn it on in test-infra.
  • Implement the user interface in cmd/deck.
  • Use it in test-infra for a week or so. Demo with contribex and convince the world that it's a good idea to switch over the rest.
  • Turn down the submit queue everywhere in favor of tide.

🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊

@spxtr spxtr self-assigned this Aug 2, 2017

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Aug 4, 2017

I expect plenty of 🌊🌊🌊 😄

@grodrigues3

This comment has been minimized.

Copy link
Contributor

grodrigues3 commented Aug 30, 2017

@kubernetes/sig-contributor-experience-proposals @kubernetes/sig-testing-proposals

@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Sep 5, 2017

@spxtr seems like we also need to have in place the finer-grained management of PRs that the submit queue is doing today where specific status contexts are required as opposed to looking through all prowjobs, ie. all jobs that ever run for a PR.

Related: #4387

@spxtr

This comment has been minimized.

Copy link
Member Author

spxtr commented Sep 6, 2017

If we need to do that then we can add a config option. That's also necessary if the repo is using travis or something. We will definitely ignore jobs that are set to skip reporting.

@mml

This comment has been minimized.

Copy link
Contributor

mml commented Sep 6, 2017

Can we make sure that this piece of configuration is global? That is, you set it in one place, but the dashboard, tide, etc. all look at the same place?

@spxtr

This comment has been minimized.

Copy link
Member Author

spxtr commented Sep 6, 2017

Yes, it will all live in prow/config.yaml.

@spiffxp spiffxp added this to the v1.9 milestone Oct 19, 2017

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Oct 24, 2017

Want to make sure we support kubernetes/sig-release#26

Other blocks that came up during sig-testing meeting:

  • A front end for tide that shows merge requirements and history (that's the web ui checklist item above, natch)
  • We need to either stop reporting non-required contexts to github or make tide aware of which contexts are required.
@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Oct 24, 2017

We need to either stop reporting non-required contexts to github or make tide aware of which contexts are required.

ref: #4932

@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Oct 24, 2017

The issue with tide not being aware of which contexts are required is when leftover contexts that are not required are broken or when a new blocking job is added and its context is missing from all PRs. I think fixing #4932 and making sure tide knows what jobs are required will get us what we want.

@spxtr

This comment has been minimized.

Copy link
Member Author

spxtr commented Oct 24, 2017

We need to either stop reporting non-required contexts to github or make tide aware of which contexts are required.

I would like to live in a world where all green checkmarks on a PR means it will merge, and any red X means it will not. #4932 moves us there :)

making sure tide knows what jobs are required will get us what we want.

One question is whether we want to build in a separate list of required contexts, or simply use always-run non-skip-reporting jobs from the config only.

@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Oct 24, 2017

One question is whether we want to build in a separate list of required contexts, or simply use always-run non-skip-reporting jobs from the config only.

I like the idea of just using the config, but we have some contexts that are not controlled by Prow, but should still be required. Notably the CLA context.

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Oct 24, 2017

@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Oct 25, 2017

simply use always-run non-skip-reporting jobs from the config only.

Should be enough but needs to be documented.

@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Oct 25, 2017

@kargakis What about contexts like the CLA context? It should still be required, but isn't in the config.

@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Oct 25, 2017

Why does tide need to care about the cla context? I thought that it was checking for the cla label.

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Oct 25, 2017

Why does tide need to care about the cla context? I thought that it was checking for the cla label.

The CLA bot bugs and people manually mark the CLA ok? If tide only pays attention to specific contexts this isn't a problem, if it expects all contexts to be green this becomes more annoying.
I'd prefer all contexts to be green, but things like this might be worth considering.

@munnerz

This comment has been minimized.

Copy link
Member

munnerz commented Nov 11, 2017

I think support additional contexts required to merge is a useful feature. I've used it in the past to require a travis context to pass for example, and it's a super convenient integration point for external tools to have a say in the merge process.

Could we automatically require all that have skip_report: false, as well as support an additional_required_contexts field or something?

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Nov 11, 2017

Could we automatically require all that have skip_report: false, as well as support an additional_required_contexts field or something?

I'm not sure I understand why this is needed or what this would do exactly? Right now tide blocks on all contexts on GitHub being green regardless of source, in the future we might want to have a white or blacklist that can be configured in Tide for which contexts exactly should be green. Even then though the prow jobs shouldn't need to set anything themselves and there can be more than one prow job configuration per context (eg for different branches).

@munnerz

This comment has been minimized.

Copy link
Member

munnerz commented Nov 11, 2017

Ah - that makes sense. I thought tide simply checks the prow config for jobs that have skip_report: false and merged when all of those were green. Checking the GitHub API and expecting all to be green makes a lot more sense!

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Nov 11, 2017

Yeah, you can see where it does that here with a TODO to look at the jobs, which we still seem to be coming to consensus on how to handle :-)

func pickSmallestPassingNumber(prs []PullRequest) (bool, PullRequest) {
smallestNumber := -1
var smallestPR PullRequest
for _, pr := range prs {
if smallestNumber != -1 && int(pr.Number) >= smallestNumber {
continue
}
if len(pr.Commits.Nodes) < 1 {
continue
}
// TODO(spxtr): Check the actual statuses for individual jobs.
if string(pr.Commits.Nodes[0].Commit.Status.State) != "SUCCESS" {
continue
}
smallestNumber = int(pr.Number)
smallestPR = pr
}
return smallestNumber > -1, smallestPR
}

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Nov 15, 2017

We discussed during sig-testing yesterday, and are planning to wait until after the 1.9 release to roll out tide for kubernetes/kubernetes. We will continue to canary it elsewhere, as it's been going fine in kubernetes/test-infra. There was some question over whether we've done adequate load testing, which openshift might be willing to help us with.

@spiffxp spiffxp added this to the 1.11 milestone Apr 11, 2018

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jul 10, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Jul 16, 2018

/remove-lifecycle stale
/milestone 1.12
This didn't happen for 1.11. Are we going to be able to make it happen for 1.12?

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Jul 16, 2018

/milestone 1.12
... now that the milestone plugin is actually enabled for this repo

@k8s-ci-robot k8s-ci-robot modified the milestones: 1.11, 1.12 Jul 16, 2018

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 6, 2018

Discussed this at sig-contribex yesterday, and discussing during community meeting today. Would like to notify kubernetes-dev afterward

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 6, 2018

PR to move k/k (to be revived) #6417

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 10, 2018

PR that made the move: #9303

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 10, 2018

@cblecker is using the migratestatus tool to retire the Submit Queue status with a note to look at the tide status instead, since GitHub doesn't allow us to delete existing statuses on PR's

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 10, 2018

https://submit-queue.k8s.io/ currently timing out, can we redirect this somehow

@ixdy

This comment has been minimized.

Copy link
Member

ixdy commented Sep 10, 2018

@spiffxp we could probably make it redirect to tide, yes

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 10, 2018

The "Submit Queue" context sorted to the top, "tide" does not, maybe we should rename the tide context: #9324

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 10, 2018

@ixdy

This comment has been minimized.

Copy link
Member

ixdy commented Sep 10, 2018

https://submit-queue.k8s.io now redirects to the Tide status page.

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 11, 2018

Current status:

  • kubernetes-misc-mungers is running with mungers: stale-green-ci,comment-deleter
  • kubernetes-cherrypick is running with mungers: cherrypick-must-have-milestone,cherrypick-clear-after-merge,cherrypick-queue (cherrypick.k8s.io is up)

I implied we were going to turn all of those off in the k-dev e-mail, is this something we can get to today? I'm going to survey cherrypick usage in the meantime.

Opened #9336 to track turning down the remaining mungers

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 11, 2018

Noticed tide's UI could stand to display more query info: #9335

@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Sep 13, 2018

This is done!

@cjwagner cjwagner closed this Sep 13, 2018

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Sep 13, 2018

@qhuynh96 is also working on improving the query info :-) PRs are in flight

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 17, 2018

We missed the fact that a submit-queue badge was on k/k kubernetes/kubernetes#68642

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 17, 2018

retest-not-required-docs-only labeling can be turned off since tide doesn't support this concept (turned off in #9452)

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 17, 2018

We needed to update tide to block merges on needs-kind and needs-sig labels for v1.12 #9364

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 17, 2018

We need to redo the dashboard that was driven by MGH stats #9458

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Sep 24, 2018

We missed removing splice as part of removing MGH #9520

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.