Add experimental batch support #84

mrusu91 · 2018-02-20T14:06:41Z

No description provided.

JaimeLennox · 2018-04-02T18:59:41Z

@aschmolck I'm reasonably happy with the state of this now.

There's quite a major refactor involved here, in order to keep as much functionality as possible between the regular job and the batch job. As such, the tests have been shuffled around quite a lot too.

I'm not entirely happy with the state of the batch job tests, as there are some core tests missing here, but to be honest I do find the mock API setup a bit confusing and I think it'd be nicer to get this merged and improve on that later, especially as this functionality is considered experimental for now.

aschmolck · 2018-06-04T15:00:22Z

Generally looks great! Just needs a minor bit of cleanup (history, a typo here and there, that sort of thing). Only real nit is that I'd personally prefer is fewer uses of Mocks (I think the state machine approach of Mocklab is nicer), but that won't stand in the way of merging it.

Signed-off-by: Marian.Rusu <rusumarian91@gmail.com>

Will be needed by batch merge job Signed-off-by: Marian.Rusu <rusumarian91@gmail.com>

Make some things reusable in preparation for batch job Signed-off-by: Marian.Rusu <rusumarian91@gmail.com>

JaimeLennox · 2018-06-04T20:39:32Z

Rebased. What are the typos you want me to fix?

Regarding the history, I actually think the commits are separate enough to warrant the number, unless you want a big super-commit - but seeing as the big refactor is halfway through, I'm not sure this makes sense. If you have any suggestions for commits to squash I'd appreciate it.

I agree that using the state machine would've been the nicest approach, but I had a hard time getting my head around it. This was the easier method, for now, and we can probably work towards improving the tests later.

aschmolck · 2018-06-05T05:57:55Z

Super-commit: I fully agree that it's not desirable to squash the mid-way refactoring nor the refinements such as the job cancellation or the modification to the batching logic away, just intermediate fixups to these (fix tests; rebase fixes etc). I have annotated the individual commits that I think are candidates.

Typos: ---batch, noted above; I thought there was another in a commit message, but since I can't see it anymore, maybe I misremembered. There are a few very minor things in the code I would change (drop the or [] above if it's indeed spurious etc).

Tests: agreed.

aschmolck · 2018-06-04T12:31:33Z

marge/batch_job.py

+            api=self._api,
+            project_id=self._project.id,
+            params=params,
+        ) or []


it looks to me like the or [] is bogus, correct?

Correct. Will fix.

aschmolck · 2018-06-04T13:23:09Z

marge/batch_job.py

+            self._api,
+        )
+        error_message = 'The {} changed whilst merging!'
+        if changed_mr.source_branch != merge_request.source_branch:


I'd be tempted to condense this down to:

for attr in 'source_branch source_project_id target_branch target_project_id sha'.split(): if getattr(changed_mr, attr) != getattr(merge_request, attr): raise CannotMerge(error_message.format(attr.replace('_', ' ')))

aschmolck · 2018-06-04T14:07:37Z

marge/git.py

+    def checkout_branch(self, branch, start_point=''):
+        self.git('checkout', '-B', branch, start_point, '--')
+
+    def push(self, branch, source_repo_url=None, force=False):


I'd change signature to:

def push(self, branch, *, source_repo_url=None, force=False):

(esp. since force is risky)

aschmolck · 2018-06-04T14:07:55Z

marge/git.py

+        force_flag = '--force' if force else ''
+        self.git('push', force_flag, source, '%s:%s' % (branch, branch))
+
+    def push_force(self, branch, source_repo_url=None):


lose this method?

Yep, not sure why it really existed in the first place.

aschmolck · 2018-06-04T14:15:47Z

marge/batch_job.py

@@ -15,7 +15,7 @@
    'WIP': 'Is marked as Work-In-Progress.',


@JaimeLennox squash please

aschmolck · 2018-06-04T14:44:07Z

marge/batch_job.py

@@ -219,6 +219,8 @@ def execute(self):
            batch_mr = self.create_batch_mr(


@JaimeLennox can't comment on previous commit, since no changes other than rename, but I think it should be squashed (the previous rename commit).

I thought it better to leave a rename commit as separate, as not to mess with the history too much (and to make it easier to git blame in the future.

aschmolck · 2018-06-04T14:45:18Z

marge/batch_job.py

@@ -229,6 +229,8 @@ def execute(self):
                continue
            else:


This belongs to your try to continue batching as many as possible, right? If so, I think it should be squashed.

aschmolck · 2018-06-04T14:46:02Z

marge/batch_job.py

@@ -1,3 +1,4 @@
+# pylint: disable=too-many-branches,too-many-statements
 import logging as log


@JaimeLennox Nice! Maybe squash into refactor commit?

aschmolck · 2018-06-04T14:52:09Z

README.md

@@ -105,6 +105,8 @@ optional arguments:
                           [env var: MARGE_BRANCH_REGEXP] (default: .*)
  --debug               Debug logging (includes all HTTP requests etc).
                           [env var: MARGE_DEBUG] (default: False)
+  ---batch  Enable processing MRs in batches.


s/---batch/--batch/, spacing looks off.

aschmolck · 2018-06-04T14:55:32Z

marge/job.py

@@ -5,7 +5,6 @@
 from datetime import datetime, timedelta



@JaimeLennox would be nice to squash this PR

I appear to have lost which commit this refers to.

Actually, if this was a reference to the "rebase fixes" commit, then I've since squashed that.

This is an attempt to make marge-bot faster by batching MRs Signed-off-by: Marian.Rusu <rusumarian91@gmail.com>

MergeJob now has as much common functionality as possible.

With batches we do a lot of work with local branches, and don't always want to use the remote branch.

We'd previously fail the batch job and start a regular job if any merge requests failed on rebasing. Instead, we can skip that one and continue with the rest. This way only the failing job is removed from the batch. If there's not enough merge jobs for the batch then we'll continue with a regular job as usual.

This gives visibility into the process and means people have a better idea if their MR is being handled.

Due to the way we update merge requests in a batch before pushing to master, extra pipelines will be triggered for each of these. We should try to cancel them to avoid extra work.

* Rename the group to make it clearer these are experimental features. * Rename --experimental-batch to --batch. The help message marks it as experimental instead, which is consistent with the --add-merge-strategy option. Note that putting the batch option in this group means the merge strategy can't be used with batching. There doesn't seem to be a good way of using the same argument in different mutually exclusive groups, and this seems acceptable for now given they are both experimental features.

This includes an explanation of the current implementation for batch jobs, along with some limitations.

The pipeline doesn't appear immediately, and so it's likely we'll hit the assertion error. Instead, we can wait for the first pipeline fetched to match our commit sha.

JaimeLennox · 2018-06-05T20:32:36Z

@aschmolck I've made the changes requested, including squashing the intermediate commits. Could you have another look and check everything is ok now?

aschmolck · 2018-06-06T06:58:32Z

@JaimeLennox great -- merged! Thanks, and sorry for the wait!

JaimeLennox force-pushed the batch branch from 428ae71 to 2025949 Compare April 2, 2018 18:50

JaimeLennox requested a review from aschmolck April 2, 2018 18:56

mrusu91 added 3 commits June 4, 2018 21:20

clean-up setup.cfg

169d077

Signed-off-by: Marian.Rusu <rusumarian91@gmail.com>

MergeRequest: Add search, create, delete

767f11e

Will be needed by batch merge job Signed-off-by: Marian.Rusu <rusumarian91@gmail.com>

Repo: small refactor

45001a7

Make some things reusable in preparation for batch job Signed-off-by: Marian.Rusu <rusumarian91@gmail.com>

JaimeLennox force-pushed the batch branch from 2025949 to 6742f84 Compare June 4, 2018 20:36

aschmolck reviewed Jun 5, 2018

View reviewed changes

mrusu91 and others added 16 commits June 5, 2018 21:07

Add batch job

8914c45

This is an attempt to make marge-bot faster by batching MRs Signed-off-by: Marian.Rusu <rusumarian91@gmail.com>

Refactor jobs

2b84970

MergeJob now has as much common functionality as possible.

Fix tests

6f47ec3

Add a local option for fusing

39d346b

With batches we do a lot of work with local branches, and don't always want to use the remote branch.

Rename test_job -> test_single_job

c666506

Comment on merge request when batching

db451ea

This gives visibility into the process and means people have a better idea if their MR is being handled.

Attempt to cancel unnecessary pipelines

9bc7185

Due to the way we update merge requests in a batch before pushing to master, extra pipelines will be triggered for each of these. We should try to cancel them to avoid extra work.

Properly handle errors during batch job

6e9cd36

Test common job functionality in separate file

6489159

Use class for batch job tests

53ab8bf

Fix remaining skipped tests

e63e189

Update README with batch job info

3db59df

This includes an explanation of the current implementation for batch jobs, along with some limitations.

README: spelling/grammar fixes

44ca478

Fix fetching MR CI status

c780532

The pipeline doesn't appear immediately, and so it's likely we'll hit the assertion error. Instead, we can wait for the first pipeline fetched to match our commit sha.

JaimeLennox force-pushed the batch branch from 6742f84 to c780532 Compare June 5, 2018 20:31

aschmolck merged commit 2fd3ef3 into smarkets:master Jun 6, 2018

JaimeLennox mentioned this pull request Jul 18, 2018

Parallel Builds #18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add experimental batch support #84

Add experimental batch support #84

mrusu91 commented Feb 20, 2018

JaimeLennox commented Apr 2, 2018 •

edited

Loading

aschmolck commented Jun 4, 2018

JaimeLennox commented Jun 4, 2018

aschmolck commented Jun 5, 2018 •

edited

Loading

aschmolck Jun 4, 2018

JaimeLennox Jun 5, 2018

aschmolck Jun 4, 2018

JaimeLennox Jun 5, 2018

aschmolck Jun 4, 2018

JaimeLennox Jun 5, 2018

aschmolck Jun 4, 2018

JaimeLennox Jun 5, 2018

aschmolck Jun 4, 2018

JaimeLennox Jun 5, 2018

aschmolck Jun 4, 2018

JaimeLennox Jun 5, 2018

aschmolck Jun 4, 2018

JaimeLennox Jun 5, 2018

aschmolck Jun 4, 2018

JaimeLennox Jun 5, 2018

aschmolck Jun 4, 2018

JaimeLennox Jun 5, 2018

aschmolck Jun 4, 2018

JaimeLennox Jun 5, 2018

JaimeLennox Jun 5, 2018

JaimeLennox commented Jun 5, 2018

aschmolck commented Jun 6, 2018

		@@ -219,6 +219,8 @@ def execute(self):
		batch_mr = self.create_batch_mr(

		@@ -1,3 +1,4 @@
		# pylint: disable=too-many-branches,too-many-statements
		import logging as log

Add experimental batch support #84

Add experimental batch support #84

Conversation

mrusu91 commented Feb 20, 2018

JaimeLennox commented Apr 2, 2018 • edited Loading

aschmolck commented Jun 4, 2018

JaimeLennox commented Jun 4, 2018

aschmolck commented Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaimeLennox commented Jun 5, 2018

aschmolck commented Jun 6, 2018

JaimeLennox commented Apr 2, 2018 •

edited

Loading

aschmolck commented Jun 5, 2018 •

edited

Loading