Before branching day: step 2

[danilo-gemoli]

This is a tool that tries to automate the second step of phase "Before branching day" .
Check Centralized Release Branching and Config Management V3

/cc @jmguzik @droslean

[droslean]

/hold

I don't feel confident in merging this PR because is HUGE. I would prefer to split this PR into 4 smaller ones.

[jmguzik]

@danilo-gemoli what about this one?

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

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

/lifecycle stale

[danilo-gemoli]

/retitle Before branching day: step 2
/remove-lifecycle stale

[danilo-gemoli]

/unhold

[droslean]

This looks really smelly and adds a lot of complexity for no reason. Are you sure that this is the only way of doing this?

[danilo-gemoli]

When it comes to bump filenames we have to handle some cases like this:
openshift-release-master__stable-4.7-upgrade-from-stable-4.6-from-stable-4.5.yaml
that should be transformed into:
openshift-release-master__stable-4.8-upgrade-from-stable-4.7-from-stable-4.6.yaml.

In order to cope with that properly you should start from the biggest minor way to the smallest and keep bumping each of them (bottom-up simply doesn't work). So you catch all the versions {"4.6","4.7","4.8",} then all the minors out of them {"6","7","8"} and finally in a descending order {"8","7","6"}. The routine uniqueSortedMinors tries to accomplish that. By the way I'm going to use the sets the reduce the lines of code if we stick with this approach.

That's not the only way, we could start from the minor of the version we're targeting, that is the future release, and repeat the processed I've described before. So if we're bumping to 4.13 (future release) the code would look like as follow:

// As an example: 
// line := "openshift-release-master__stable-4.12-upgrade-from-stable-4.11-from-stable-4.10.yaml"
major, minor := 4, 13
for minor := minor-1; minor >= 0; minor-- {
    curVersion := fmt.Sprintf("%d.%d", major, minor)
    nextVersion := fmt.Sprintf("%d.%d", major, minor+1)
    line = strings.ReplaceAll(line, curVersion, nextVersion)
}
// Result: 
// line := "openshift-release-master__stable-4.13-upgrade-from-stable-4.12-from-stable-4.11.yaml"

That is going to work too and it will cycle a little bit more, but to be honest I don't think it doesn't make (and it won't make in the future too unless numbers become huge) any difference in terms of performance here.

[droslean]

Why we can't just increment any 4.x value?

[jmguzik]

@droslean I don't think incrementing any 4.x is a correct behavior if we are doing a branch cut for eg 4.12. Maybe it covers most of the cases, but not sure, this could lead to bumping something that is hard coded for example. That being said, it is true that normally we operate in ranges 4.x-2 to 4.x+2, and bumping anything in that range I would consider safe.

[danilo-gemoli]

We can bump any 4.x value but the order the increment is done matters.

Consider again this line:
openshift-release-master__stable-4.12-upgrade-from-stable-4.11-from-stable-4.10.yaml
that has to be transformed to:
openshift-release-master__stable-4.13-upgrade-from-stable-4.12-from-stable-4.11.yaml

If we bump it in an order different from descending, the following scenario could happen:

1. Bumping from 4.10 to 4.11
openshift-release-master__stable-4.12-upgrade-from-stable-4.11-from-stable-4.11.yaml
2. Bumping from 4.11 to 4.12
openshift-release-master__stable-4.12-upgrade-from-stable-4.12-from-stable-4.12.yaml
3. Bumping from 4.12 to 4.13
openshift-release-master__stable-4.13-upgrade-from-stable-4.13-from-stable-4.13.yaml

That's not what we expect.

[droslean]

Bumping from 4.10 to 4.11:
openshift-release-master__stable-($TO+0.1)-upgrade-from-stable-$TO-from-stable-$TO.yaml

[droslean]

Either way. I am still not convinced that this is the best way. Why do we need to sort them? And what is this 2d string array? Ultimately, we just need to match a regex and increment it based on the given FROM and TO versions. This applies to everything.

[danilo-gemoli]

Bumping from 4.10 to 4.11:
openshift-release-master__stable-($TO+0.1)-upgrade-from-stable-$TO-from-stable-$TO.yaml

If what you intended was TO="4.11" then it will result in:
openshift-release-master__stable-4.12-upgrade-from-stable-4.11-from-stable-4.11.yaml
and I'm not sure it's what we want.

If the starting string is:
openshift-release-master__stable-4.12-upgrade-from-stable-4.11-from-stable-4.10.yaml

the pattern it follows is:
N=12
openshift-release-master__stable-4.$(( N ))-upgrade-from-stable-4.$(( N-1 ))-from-stable-4.$(( N-2 )).yaml

therefore it should be transformed to (every minor version incremented by 1):
openshift-release-master__stable-4.$(( N+1 ))-upgrade-from-stable-4.$(( N ))-from-stable-4.$(( N-1 )).yaml

that is:
openshift-release-master__stable-4.13-upgrade-from-stable-4.12-from-stable-4.11.yaml

Why do we need to sort them?

According to my approach we need to sort them because of this.

And what is this 2d string array?

It is the result of function Regexp.FindAllStringSubmatch from the standard library.

I am still not convinced that this is the best way.

Feel free to suggest any code so we can discuss on it together.

[jmguzik]

The reason I do not like this function is not necessarily the method or the complexity but the input, I feel it could be better than [][]string which is described not in the code but in the comment. I am ok, to extract this discussion and upgrade of this mechanism to another PR or minor Jira issue, just to prevent this whole tool from rotting.

[jmguzik]

@danilo-gemoli please prepare alternative step descriptions for your tools in the branching document. Please do not remove existing ones. Document where your tools still need manual intervention (eg these bumps in comments or commands). You can just simply do it or create a Jira task if needed.

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD f7cc2ec and 2 for PR HEAD 29b6315 in total

[jmguzik]

/retest-required

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD d71773c and 1 for PR HEAD 29b6315 in total

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD f7860cf and 0 for PR HEAD 29b6315 in total

[openshift-ci-robot]

/hold

Revision 29b6315 was retested 3 times: holding

[danilo-gemoli]

/override ci/prow/e2e
we should reduce e2e tests flakiness ASAP

[openshift-ci]

@danilo-gemoli: Overrode contexts on behalf of danilo-gemoli: ci/prow/e2e

In response to this:

/override ci/prow/e2e
we should reduce e2e tests flakiness ASAP

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.

[danilo-gemoli]

/unhold

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danilo-gemoli, jmguzik

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [danilo-gemoli,jmguzik]

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

[openshift-ci]

@danilo-gemoli: all tests passed!

Full PR test history. Your PR dashboard.

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.