[CRIU] [PATCH v3 4/6] p.haul: add option to enable/disable pre-dumps

[Pavel Emelyanov]

On 10/09/2015 11:39 PM, Adrian Reber wrote:
> On Fri, Oct 09, 2015 at 08:24:42PM +0300, Pavel Emelyanov wrote:
>> On 10/09/2015 07:53 PM, Adrian Reber wrote:
>>> On Fri, Oct 09, 2015 at 06:40:47PM +0300, Pavel Emelyanov wrote:
>>>> On 10/08/2015 05:06 PM, Adrian Reber wrote:
>>>>> From: Adrian Reber <areber at redhat.com>
>>>>>
>>>>> Although p.haul can now auto-detect if the used criu supports
>>>>> memory tracking this adds a command-line option to force
>>>>> pre-dumping on or off:
>>>>>
>>>>>   --no-pre-dump        Force disable pre-dumps
>>>>>   --pre-dump           Force enable pre-dumps
>>>>>
>>>>> Signed-off-by: Adrian Reber <areber at redhat.com>
>>>>> ---
>>>>>  p.haul                | 12 ++++++++++++
>>>>>  phaul/p_haul_iters.py | 18 ++++++++++++++----
>>>>>  2 files changed, 26 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/p.haul b/p.haul
>>>>> index e807c8f..6239709 100755
>>>>> --- a/p.haul
>>>>> +++ b/p.haul
>>>>> @@ -36,6 +36,18 @@ parser.add_argument("--log-file", help="Write logging messages to specified file
>>>>>  parser.add_argument("-j", "--shell-job",help ="Allow migration of shell jobs",
>>>>>  		    default=False, action='store_true')
>>>>>  
>>>>> +
>>>>> +parser.add_argument('--no-pre-dump', dest='pre_dump', action='store_const',
>>>>> +		    const=1, help='Force disable pre-dumps')
>>>>> +parser.add_argument('--pre-dump', dest='pre_dump', action='store_const',
>>>>> +		    const=2, help='Force enable pre-dumps')
>>>>> +
>>>>> +# If pre_dump is 0 then pre-dump autodetection should be used.
>>>>> +# If it is 1 or 2 it should be either enabled or disabled no
>>>>> +# matter if the kernel supports dirty page tracking or not.
>>>>> +
>>>>> +parser.set_defaults(pre_dump = 0)
>>>>> +
>>>>>  args = parser.parse_args()
>>>>>  
>>>>>  # Configure logging
>>>>> diff --git a/phaul/p_haul_iters.py b/phaul/p_haul_iters.py
>>>>> index ba96de3..96458b8 100644
>>>>> --- a/phaul/p_haul_iters.py
>>>>> +++ b/phaul/p_haul_iters.py
>>>>> @@ -42,6 +42,9 @@ class phaul_iter_worker:
>>>>>  		self.pid = self.htype.root_task_pid()
>>>>>  		self.fs.set_target_host(host[0])
>>>>>  
>>>>> +		# pre_dump 0 means auto-detection
>>>>> +		self.pre_dump = 0
>>>>> +
>>>>>  		logging.info("Setting up remote")
>>>>>  		self.target_host.setup(p_type)
>>>>>  
>>>>> @@ -55,6 +58,7 @@ class phaul_iter_worker:
>>>>>  		self.img.set_options(opts)
>>>>>  		self.htype.set_options(opts)
>>>>>  		self.__force = opts["force"]
>>>>> +		self.pre_dump = opts["pre_dump"]
>>>>>  
>>>>>  	def validate_cpu(self):
>>>>>  		logging.info("Checking CPU compatibility")
>>>>> @@ -87,11 +91,17 @@ class phaul_iter_worker:
>>>>>  		self.fs.set_work_dir(self.img.work_dir())
>>>>>  		self.fs.start_migration()
>>>>>  
>>>>> -		logging.info("Checking for Dirty Tracking")
>>>>> -		req = criu_req.make_dirty_tracking_req(self.htype, self.img)
>>>>> -		resp = self.criu_connection.send_req(req)
>>>>> +		if self.pre_dump == 0:
>>>>> +			# pre-dump auto-detection
>>>>> +			logging.info("Checking for Dirty Tracking")
>>>>> +			req = criu_req.make_dirty_tracking_req(self.htype, self.img)
>>>>> +			resp = self.criu_connection.send_req(req)
>>>>> +			pre_dump = resp.success
>>>>> +		elif self.pre_dump == 1:
>>>>> +			pre_dump = False
>>>>> +		else:
>>>>> +			pre_dump = True
>>>>>  
>>>>> -		pre_dump = False
>>>>>  		if resp.success:
>>>>
>>>> The resp will be "uninitialized" in case self.pre_dump != 0 and you
>>>> set up the pre_dump variable to True/False by hands.
>>>
>>> The follow-up patches I posted should fix this. Having rebased this code
>>> a few times I am not 100% sure the intermediate patches provide an error
>>> free implementation. At the end I have only tested the whole patchset.
>>
>> Ah, I see.
>>
>>> Should I respin starting from this patch or is it good enough if the
>>> code is correct after all my patches have been applied.
>>
>> Can you please re-shuffle the patches, so that there are two patches -- one
>> with auto-detection and the other one adding "forced" option that ignores
>> the automatic detection?
> 
> The patches are already split up like this. Auto-detection was
> implemented in the first patch of the series. This patch (4/6) adds the
> command-line option to force pre-dumps on or off.
> 
> 5/6 adds the possibility to use an older version of criu which does not
> have the feature-check interface (like 1.7 and older) and patch 6/6
> moves the auto-detection to its own function as it has become large
> enough to be a separate function.

OK. Sill after reading the combined patch I have comments :)

First, can you introduce named constant for p_haul_iters.pre_dump values
as Nikita suggested. And, the second, not introduce the local for
start_migration() pre_dump variable, instead doing smth like

   if self.pre_dump == AUTO_DETECT:
                 /* do autodetection code that sets self.pre_dump */

   while self.pre_dump:
                 /* iterations code */

-- Pavel