Add code_freeze_maintainers_team option to milestone prow plugin

[saschagrunert]

The new option allows to set a specific team (like the release-team-leads) to be milestone maintainer when code freeze is in place. For that we extend the milestone plugin to be able to validate codefreeze based on the tide configuration of the repository.

Ref:

• Milestone maintenance enhancements  sig-release#2386

PRs for code freeze updates:

• Enforce 1.29 code freeze #31164
• [release-1.27] Lift Kubernetes 1.27 code freeze #29259

cc @kubernetes/release-managers

Still in WIP, I'm not sure about 1-2 approaches here (see comments below).

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: saschagrunert
Once this PR has been reviewed and has the lgtm label, please assign cjwagner for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• prow/plugins/OWNERS

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

[liggitt]

Hmm, I think it's important to still allow sig leads and the normal milestone maintainers to indicate an issue or fix is required for the milestone.

I'm not sure I would switch the alias used to control milestone access.

If the goal is to ensure release managers coordinate merges, would it make more sense to change merge requirements to also require the cherrypick-approved label after test freeze once the release branch is created? A automated cherry-pick to the release branch is essentially what happens automatically during that time, and that label is already in the release manager workflow.

[saschagrunert]

If the goal is to ensure release managers coordinate merges, would it make more sense to change merge requirements to also require the cherrypick-approved label after test freeze once the release branch is created? A automated cherry-pick to the release branch is essentially what happens automatically during that time, and that label is already in the release manager workflow.

Yeah, my goal was to cover the gap between code freeze and test freeze as well:

WIth the milestone maintainer team toggle at code freeze (only for the particular milestone), we would technically require comms between folks who want to get a PR merged and the release team, which is good I guess?

I think we could also require a cherry-pick label for those PRs, but is this even more confusing? Have to think a bit more about this.

/hold

[liggitt]

If people in the normal milestone maintainers group are intentionally putting risky or nonessential things into the milestone after code freeze, we have much bigger problems. The members of the normal milestone group are supposed to be the people responsible for the health of the various subsystems in the milestone. Ensuring that group is up to date, and members of that group having a clear understanding of the bar and accountability for exercise of that power is good. Removing the ability of that group to indicate a particular issue or change is essential for the milestone is… less good.

I'd focus any changes here around coordination of merges with the release team at points where uncoordinated merges would be problematic. I thought that was primarily during releases and after the release branch is created. I'm not attached to the cherrypick-approved suggestion if that is more confusing than helpful.

[saschagrunert]

Ensuring that group is up to date, and members of that group having a clear understanding of the bar and accountability for exercise of that power is good.

That's a good point. At least to me the amount of people on the list feels huge: https://github.com/kubernetes/org/blob/8bcd95a09bcd961f7af9f7a5ccf87235e79e5a35/config/kubernetes/sig-release/teams.yaml#L16-L144

Would it be better to re-iterate the list periodically for folks who actually want and use the role?

cc @kubernetes/sig-release-leads

I think the docs needs more wording as well: https://github.com/kubernetes/sig-release/blob/d08accce30dc28b6976cc9dc45e48d5a67a949d9/release-team/README.md#milestone-maintainers

Having the information available on k/website could help, too.

[liggitt]

{sig leads, release team, release branch managers} seems like the ~right way to construct that list to me (with the expectation that sig leads milestone things for their areas)

[xmudrii]

If the goal is to ensure release managers coordinate merges, would it make more sense to change merge requirements to also require the cherrypick-approved label after test freeze once the release branch is created?

I think this might be a very solid workaround here. We could require a label to be set. I think the number of folks who can lift labels is fairly minimal and we didn't have non-releng folks lifting the cherry-pick-unapproved labels before (at least as far as I know).

I think we could also require a cherry-pick label for those PRs, but is this even more confusing? Have to think a bit more about this.

It doesn't have to be the cherry-pick label, we can introduce do-not-merge/code-freeze. We don't need a new Prow plugin for this to work, we could use require_matching_label, e.g.:

require_matching_label:
  - missing_label: do-not-merge/code-freeze
    org: kubernetes
    repo: kubernetes
    prs: true
    branch: master
    regexp: ^code-freeze-approved$

(we use this approach at my $day_job and it worked pretty well)

[saschagrunert]

If the goal is to ensure release managers coordinate merges, would it make more sense to change merge requirements to also require the cherrypick-approved label after test freeze once the release branch is created?

I think this might be a very solid workaround here. We could require a label to be set. I think the number of folks who can lift labels is fairly minimal and we didn't have non-releng folks lifting the cherry-pick-unapproved labels before (at least as far as I know).

The end effect would be the same as flipping the milestone maintainers, we just add another barrier to merge a PR.

[liggitt]

The end effect would be the same as flipping the milestone maintainers ...

not quite, having things in the milestone is a super clear way to indicate "this is for release 1.x" ... removing the ability to signal that and have issues and PRs show up in queries makes it too easy to lose track of critical things

... we just add another barrier to merge a PR.

If I understand correctly, wasn't the intent of this to explicitly add another barrier?

[xmudrii]

The end effect would be the same as flipping the milestone maintainers, we just add another barrier to merge a PR.

Not really, this is closer to what we actually want. The original Slack conversation started around kubernetes/kubernetes#121861. The milestone was applied and the PR was merged without an ACK from the release team.

The idea that we discussed there was to prevent applying the current milestone to PRs for everyone but the release team leads and the release managers. We should agree if we want such a barrier in place, and if yes, should it be based on milestone or label.

I find a label-based barrier to be useful:

• milestone maintainers can mark PRs they think that should go into release with /milestone
• it's up to the release team leads to decide if that PR should actually go into the release by leaving or lifting the label

This is the actual process that we have in fact, where SIG leads can propose something for the release, but it's up to the release team leads to decide if they're going to follow on that advice from SIG leads.

[saschagrunert]

The end effect would be the same as flipping the milestone maintainers ...

not quite, having things in the milestone is a super clear way to indicate "this is for release 1.x" ... removing the ability to signal that and have issues and PRs show up in queries makes it too easy to lose track of critical things

... we just add another barrier to merge a PR.

If I understand correctly, wasn't the intent of this to explicitly add another barrier?

Yes, but without making it more complex, that's why I was going for the toggle. Using the milestone as indicator for the release is quite an argument, that's probably also the reason why the list is that large.

Going to close this one now since it will not land. OTOH I don't think that making the process more complex will help us here. But I'm open to alternatives 🙃

[neolit123]

yes, sig leads must continue to apply milestone during code freeze. that's how critical bug fixes can get merged.
but if we evaluate that the volume is relatively low perhaps we can gate all that on release team ack as well.

or if we want a period when only the release team can merge (while milestone maintainers retail their privs), my vote is to add an explicit label mechanic...it's just clearer ALA needs-release-team-ack.

i think i kicked the bee hive one time that the k8s release process could have been done differently.
but my arguments are rather pointless given we have a good deal of maintainers convinced that what we have today is close to optimal.

[saschagrunert]

i think i kicked the bee hive one time that the k8s release process could have been done differently.
but my arguments are rather pointless given we have a good deal of maintainers convinced that what we have today is close to optimal.

@neolit123 I'm personally happy to hear any alternatives in the same way as SIG Release is open for suggestions into any direction. The process is fairly complex as of today, reducing that complexity by making it better and simpler is a tough challenge for us.