#2387 Allow deleting of release branches that are accidentally created.
Closed: Accepted 3 years ago by churchyard. Opened 3 years ago by mohanboddu.

RE: RelEng ticket, I have seen a few requests come by and I thought we should come up with a way to resolve this.

This request is an extension to another fesco ticket in which its allowed to remove stream branches.

Currently deleting of release branches is disallowed since there is no way to figure out if a build got submitted from a certain branch and also the solution that is mentioned in above ticket is not usable in this case.

The only way that I can think of doing this is, parsing through all the git hashes in that branch and matching it with the hashes in koji db from which a build got submitted. (@kevin thinks koji saves this data, but not 100% sure). If there is no match then the branch can be deleted, if there is a match, then it should not be deleted. (Although there can be multiple branches with same hashes and I think we should leave them as an unsolvable case)

I am not sure if there is any easier way, but I am interested in options.


This request is an extension to another fesco ticket in which its allowed to remove stream branches

That ticket talks about branches. Not stream branches.

This policy can be applied here. If the commit is in any other reachable branch or tag, the branch can be dropped.

This request is an extension to another fesco ticket in which its allowed to remove stream branches

That ticket talks about branches. Not stream branches.

Sorry, you are right.

This policy can be applied here. If the commit is in any other reachable branch or tag, the branch can be dropped.

So, if all the hashes from the requested delete branch are in other branches, then delete the branch?

I'd say ...

Case 1: Branch contains no commits that were built → deletable

Case 2: Branch contains commits that were built ... are those commits reachable from another branch?
Case 2a: All such commits are reachable from other branches → deletable
Case 2b: Some or all such commits are not reachable from other branches → not deletable

Did I miss a case?

So, if all the hashes from the requested delete branch are in other branches, then delete the branch?

If the one commit that the branch is pointing to is accessible form any other branch, then we can safely delete the branch. Which of course isn't the case for the epel8 branches, because they have that ugly extra commit.

@decathorpe 2a and 2b don't make snese in git. Either the commit the branch is pointing to is reachable (and hence all are) or it is not (and hence at least that one is not "in there").

Branch contains no commits that were built is almost never true. Every branch that was created at a certain point from master has all the commits that were built in master.

So there are basically three cases:

  1. the tip of the to-be-deleted-branch is accessible from any other branch/tag -> safe to delete (current policy)
  2. the tip of the to-be-deleted-branch is not accessible from any other branch/tag, but none of the nonassignable commits on top of that branch were ever used to build anything (nontrivial to detect, but possible) -> safe to delete (but not allowed yet)
  3. the tip of the to-be-deleted-branch is not accessible from any other branch/tag and some of the nonassignable commits on top of that branch were used to build anything -> not safe to delete (but a tag can be created)

Branch contains no commits that were built is almost never true. Every branch that was created at a certain point from master has all the commits that were built in master.

I think the intent here is that the branch from the point it was branched from master contains no commits that were built.

I think the intent here is that the branch from the point it was branched from master contains no commits that were built.

I understand that, but that is very hard to define, especially if the history is not linear.

That's the reason why the rule in https://pagure.io/fesco/issue/2340 is so super simple and does not mention builds.

(The perfect rule would be "all commits ever used to build non-scratch builds in Koji must remain accessible" -- but nobody want's to verify that.)

(The perfect rule would be "all commits ever used to build non-scratch builds in Koji must remain accessible" -- but nobody want's to verify that.)

That's what I wanted to express with my 2.5 cases, but apparently I'm not good with git terminology :)

the tip of the to-be-deleted-branch is accessible from any other branch/tag -> safe to delete (current policy)

Yes, but its a policy for non release branches. We have to make it for release branches as well.

the tip of the to-be-deleted-branch is not accessible from any other branch/tag, but none of the nonassignable commits on top of that branch were ever used to build anything (nontrivial to detect, but possible) -> safe to delete (but not allowed yet)

As long koji stored them as the hashes rather than just as HEAD

the tip of the to-be-deleted-branch is not accessible from any other branch/tag and some of the nonassignable commits on top of that branch were used to build anything -> not safe to delete (but a tag can be created)

I like these suggestions.

Yes, but its a policy for non release branches. We have to make it for release branches as well.

I like to think about it as policy for accidentally created branches.

A.k.a it is not OK to remove the f32 branch if the package was shipped in Fedora 32 (even if the f32 tip is still accessible from master), but it is ok to delete the epel8 branch, when the package was never shipped in epel8 (if the tip is still accessible from any other branch or tag).

The resolution was "Branches reachable from any other branch that are not used for building anything can be removed" which supports this.

I'd like to reiterate what I had in the other decision: releng is happy to follow whatever policy here, but there should be a easy way (say a script) to test things. releng is stretched pretty thin, and I think trying to figure out if a useless branch can be deleted is a poor use of their time. If there's a script and we can say 'the script passes, sure, deleting' or 'no, sorry, can't do it' thats fine.

Sure. The current policy can be covered by a script. I still haven't got the rights to test it out.

Right, someone needs to write the script. My proposal would be to wait for that, and discuss any clarifications if needed than, and close this ticket for now.

(Basically, if the script is smart enough to reliably detect if any commits on a given branch not reachable from other branches were used to build anything in koji, than we'll have more freedom in what we allow to delete. If we can't script that, than we'll have to be more conservative, but we can still delete many things safely. But trying to decide the fine details of the policy before the technical possibilities are clear is premature.)

(If we cannot figure out reliably which commits were used to build things, we could always do a fake merge with git checkout master && git merge -s ours branch-to-delete. That'd be a slight ugliness in history, but we could get rid of the branch without fear of losing any commits. But I hope we can do reliable build detection and this will not be needed.)

(Or tag things with was-epel8-branch-now-deleted.)

(Or tag things with was-epel8-branch-now-deleted.)

The problem with that is that the tag would get pulled on clone, and would be rather visible to users.

IMO, it is not really different from "stream" branches. As long as commits which were used to build packages stay, it is fine.

+1 to close ticket and wait for the script which checks for mandatory things regardless of "stream" or non-stream branch.

Can you discuss this in next meeting?

I don't understand what's here to discuss, sorry.

@churchyard Well, to make it official and write up a policy.

Official policy in https://pagure.io/fesco/issue/2340

I'm waiting for rights in https://pagure.io/releng/issue/7265#comment-643371 (I've missed some comments there, will follow up).

One thing that I'd like to see clarified:

Is it OK to drop a release branch with accessible commits when we acetally built from that branch for a released Fedora/EPEL (I'd think this should not be possible).

Example: Based on the rules, we can remove the f32 branch of rpms/git because it is accessible from master. However, that doesn't actually make any sense.

Example: Based on the rules, we can remove the f32 branch of rpms/git because it is accessible from master. However, that doesn't actually make any sense.

Hmm, does koji actually care about the branch name at all? It seems to me it memorized the git hash, but I don't see any branch information anywhere.

Maybe we should just disallow removing any branches that match a pattern (master | f\d{1,2} | epel\d)?

@zbyszek that is too restrictive, and would defeat the purpose of deleting accidentally created (fedora stable / epel) branches.

  • If a fxx branch was created because the package was already around for the fedora xx branch point, then the branch should be kept IMO.
  • If fxx branch is newly created when xx+{1,2} is rawhide, and the package has never been part of fedora xx, then it can of course be removed.

Example: Based on the rules, we can remove the f32 branch of rpms/git because it is accessible from master. However, that doesn't actually make any sense.

I think if we ever start having one single branch for packages which do not have any differences in spec files and build everything from it for all targets, I think this can be quite useful to clean up branches like this :)

Also sometimes people create branch like epel7, merge master into it and then build fails.. And they never fix it because EL7 is just too old or something like that.

If a fxx branch was created because the package was already around for the fedora xx branch point, then the branch should be kept IMO.

Hmm, so that seems unnecessarily restrictive too. If releng creates a branch, but the maintainer doesn't want to package ever built from that branch, it should be OK to delete it. A more useful cutoff point would be if the package rebuilds in a mass rebuild or anything like that. And that is covered by existing checks.

Also note that git doesn't much care about branch creation time. E.g. I could create an f12 branch in any repo in dist-git right now. Apart from any git reflog entries, the branch would be identical to any branch created 8 years ago. So I don't think we need to care about branches themselves, just the commits in those branches.

Yes, deleting the f32 branch would make no difference. Except the branch would be missing. And that would be confusing. So my point is, if we allow to delete epel and fc branches, I think there should be another criterion, not just git commit based. Something like:

Branches reachable from any other branch and branches where all otherwise unreachable commits have never been built in koji (except for scratch builds) can be deleted. Additional restrictions apply to EPEL (elX, epelX) and Fedora release (fcXX) branches: If the package has ever been built for the respective EPEL/Fedora version, the branch cannot be removed. The master branch, once existing, cannot ever be removed.

OK, as long as we express that as a programatic check.

Would the following suffice: convert branch name to a list of tags, check if any of those tags contains builds for that package, and if yes, refuse removal?
For example: when looking at branch 'f32', we'd have a list of tags 'f32', 'f32-updates-testing', 'f32-updates-pending', and refuse to remove 'f32' is any of those tags contain builds for the package.
(How to go from a branch name to all tags that are interesting for that branch?)

@zbyszek bodhi exposes this as part of its API:

import pprint
import requests

pprint.pprint(requests.get("https://bodhi.fedoraproject.org/releases/?rows_per_page=100").json()["releases"])

[
...

{'branch': 'f29',
 'candidate_tag': 'f29-updates-candidate',
 'composed_by_bodhi': True,
 'composes': [],
 'create_automatic_updates': False,
 'dist_tag': 'f29',
 'id_prefix': 'FEDORA',
 'long_name': 'Fedora 29',
 'mail_template': 'fedora_errata_template',
 'name': 'F29',
 'override_tag': 'f29-override',
 'package_manager': 'dnf',
 'pending_signing_tag': 'f29-signing-pending',
 'pending_stable_tag': 'f29-updates-pending',
 'pending_testing_tag': 'f29-updates-testing-pending',
 'stable_tag': 'f29-updates',
 'state': 'archived',
 'testing_repository': 'updates-testing',
 'testing_tag': 'f29-updates-testing',
 'version': '29'},

...
]

There's probably also other ways to get this info ... but I trust that bodhi knows where to tag builds (at least, most of the time)

Thanks, that is useful.

Unfortunately it only goes as far as 'f21':
['el5', 'el6', 'eln', 'epel7', 'epel8', 'epel8m', 'f21', 'f22', 'f23', 'f24', 'f25', 'f26', 'f27', 'f27m', 'f28', 'f28', 'f28m', 'f29', 'f29', 'f29', 'f29m', 'f30', 'f30', 'f30', 'f30m', 'f31', 'f31', 'f31', 'f31m', 'f32', 'f32', 'f32', 'f32m', 'f33', 'f33']
But I think it would OK to do the following:
- look up the branch in bodhi, if found, do the check using any tags declared by bodhi
- if bodhi doesn't know the branch, and the branch matches the pattern of f\d{1,2}|el\d|epel\d, refuse
- otherwise allow

(We would refuse the removal of very old branches that look like release branches, but people wouldn't want that too often anyway, so I think that's OK.)

EDIT: I removed m from the pattern, because modules didn't exists before f21, so we can rely on bodhi for anything that ends with m.

This is still waiting for any response from releng on the PR. Nothing to discuss here.

Well, not sure what you are waiting for there. Releng is happy to use that script if thats the policy... we need to know thats the policy tho.

The script to implement a check has been merged in https://pagure.io/releng/pull-request/9454, but the policy it implements is more complicated than what we approved in #2340. As requested by @kevin, to formalize this, I propose the following policy for approval:

Allow removal of a branch unless one the following:

  • the branch name is a bodhi release name and koji lists builds from that branch¹
  • branch name is master
  • the branch appears to be a Fedora or EPEL release branch, but bodhi doesn't know about it²
  • for any commits which are on that branch and are not reachable from any other branch:
    • a build is found in koji
    • the name in the spec file is different than the package name³
    • the spec file cannot be parsed or the effective name cannot be figured out³

¹ Koji only cares about git hash, but bodhi uses the branch name to appropriately tag the package. So for example, for branch f27 bodhi lists tags "f27", "f27-updates", "f27-updates-testing", "f27-updates-candidate", "f27-signing-pending", "f27-updates-testing-pending", "f27-updates-pending", "f27-override", and we would check if koji lists any builds for the package name in those branches.
² This rule is because bodhi only knows branches as far as f21. The scripts right now implements the check as: does the branch matche the pattern f\d{1,2}|el\d|epel\d|epel1\d.
³ Those last two points could be relaxed by improving the script to handle more corner cases, they are limitations of implementation.

The script has some graphs to make this easier, see https://pagure.io/releng/pull-request/9454#request_diff.

+1, although I'd add one more point for SPEC files:

  • the branch is referenced by a HEAD commit modulemd of any existing modules/ branch

And for modules we need to check whether any commits in that branch were ever built by MBS.

Technically, this is approved, but we should probably clear up @psabata's points before announcing.

the branch is referenced by a HEAD commit modulemd of any existing modules/ branch

And for modules we need to check whether any commits in that branch were ever built by MBS.

I don't grok modularity enough to implement this. Is there a volunteer from the modularity side to implement the required checks? And if not, please describe in detail what needs to be done, and I'll try to implement it.

So I think we need to check every branch of every module in the modules/ namespace and query the modulemd files that were used to build modules.

In those modulemd files we need to look at the components section: https://github.com/fedora-modularity/libmodulemd/blob/master/yaml_specs/modulemd_stream_v2.yaml#L431

Get all package names (components.rpms.*.name or components.rpms.* if the name is not provided) and their ref, which could be any git ref.

To query built modules, go to https://mbs.fedoraproject.org/module-build-service/1/module-builds/ and see the scmurl field for each successful build (state equal to 5, or not equal to 4 might be better).

@zbyszek Do you think this is doable, or do we abandon the effort?

I haven't had time to work on this, but based on the description provided by @psabata, I don't think I'd be able to make it work anyway because of the complexity. I think somebody from the Modularity side will need to take a stab at implementing this.

Can we at least do this for branches of packages that have never been built for any module? I hope that would be a lot easier to check and should cover a lot of cases already, until a comprehensive check for module stuff can be implemented.

packages that have never been built for any module?

We would still be able to reliable detect this, right?

packages that have never been built for any module?

Sounds like the way to go.

We would still be able to reliably detect this, right?

The question can be stated as: were any module builds done for this package? I think koji can answer this reliably by itself, no?

The question can be stated as: were any module builds done for this package? I think koji can answer this reliably by itself, no?

I would hope so. Wouldn't grepping a package's builds for NVRs containing module_ be enough?

Wouldn't the current script give the koji builds for modular builds as well?

So, releng is already processing the requests for branch removals. It would be nice if somebody could put the current practice in a policy, but as long as it works, I'm not motivated to do that. If we have a volunteer, please take this ticket, otherwise I suggest we close it.

Metadata Update from @churchyard:
- Issue close_status updated to: Accepted
- Issue status updated to: Closed (was: Open)

3 years ago

I don't think the issue was resolved yet. I filed https://pagure.io/releng/pull-request/9837 to implement @decathorpe's simpler approach of refusing removal when we have any modular builds. I think it is OK to keep this closed, since the policy part is clear and it's just the implementation that is somewhat lacking. Any further discussion can be done in the PR or new tickets are appropriate.

Login to comment on this ticket.

Metadata