Ticket2624 file tracking #139

ThomasLohnert · 2017-10-10T12:44:33Z

Description of work

Simplified the system responsible for keeping user scripts and configurations under version control:
VC is now managed by a thread running in the blockserver, which simply adds, commits and pushes all files in the repositories every 5 minutes. This change greatly reduces the complexity of the system, removes the need for any filewatchers, and with it previously problematic interactions between the filewatcher and the version control threads.

To test

ISISComputingGroup/IBEX#2624

Acceptance criteria

Any changes on the file system are committed to git in regular intervals (currently 5 minutes - you can tweak this interval in the PUSH_BASE_INTERVAL variable in git_version_control.py if it makes testing easier
The commit messages provide an appropriate summary of changes
Graceful handling of problematic file system / git interactions: Trying to save a file while it is being added; trying to add a file while it is being saved. You can use the following powershell script to manually lock/unlock a file for testing:
Interactions with configurations from the GUI are unchanged

#Specify the file name
$fileName = "[file path]"

#Open the file in read only mode, without sharing (I.e., locked as requested)
$file = [System.io.File]::Open($fileName, 'Open', 'Read', 'None')

#Wait in the above (file locked) state until the user presses a key
Write-Host "Press any key to continue ..."
$null = $host.UI.RawUI.ReadKey("NoEcho,IncludeKeyDown")

#Close the file (This releases the current handle and unlocks the file)
$file.Close()

Code Review

Is the code of an acceptable quality?
Has the author taken into account the multi-threaded nature of the code?
Have the changes been documented in the release notes. If so, do they describe the changes appropriately?
Has the manual system tests spreadsheet been updated?

Functional Tests

Do changes function as described? Add comments below that describe the tests performed.

Final steps

Reviewer has updated the submodule in the main EPICS repo? See Reviewing work for the subModules of EPICS in the Git workflow page for details.
Reviewer has moved the release notes entry for this ticket in the "Changes merged into master" section

…ert)

…file_tracking

…e is in use)

Tom-Willemsen

Getting the following exception, using latest build of genie_python installed from \\isis\inst$\Kits$\CompGroup\ICP\genie_python. Do you perhaps have a different version of Python installed on your system (or, more precisely, GitPython version)? The git python version on the latest build on the build server is 0.3.6

...
INFO: Stopping IOC INHIBITR_01
Alarm server will update in 20 seconds from now

INFO: Stopping IOC CHIPIR_XYZ
Alarm server will update in 20 seconds from now

INFO: Stopping IOC GALIL_02
Exception in thread Thread-3:
Traceback (most recent call last):
  File "C:\Instrument\Apps\Python\lib\threading.py", line 810, in __bootstrap_inner
    self.run()
  File "C:\Instrument\Apps\Python\lib\threading.py", line 763, in run
    self.__target(*self.__args, **self.__kwargs)
  File "C:\Instrument\Apps\EPICS\ISIS\inst_servers\master\ConfigVersionControl\git_version_control.py", line 152, in _commit_and_push
    self._commit()
  File "C:\Instrument\Apps\EPICS\ISIS\inst_servers\master\ConfigVersionControl\git_version_control.py", line 141, in _commit
    raise CommitToVersionControlException("Couldn't commit to version control")
CommitToVersionControlException: Couldn't commit to version control

Alarm server will update in 20 seconds from now

INFO: Stopping IOC SDTEST_01
Alarm server will update in 20 seconds from now

INFO: Stopping IOC TEST_01
Alarm server will update in 20 seconds from now
...

Code looks good, some small stylistic changes suggested in the review comments, but haven't been able to run this to test it properly.

Tom-Willemsen · 2017-10-11T07:56:13Z

BlockServer/devices/devices_file_io.py

+                time.sleep(RETRY_INTERVAL)
+
+        raise Exception(
+            "Could not save to device screens file at %s. Please check the file is not in use by another process." % file_name)


Our standard (from the python style gui chat) is now to use .format() rather than % to format strings

Comment repeats as necessary

Tom-Willemsen · 2017-10-11T07:56:42Z

BlockServer/devices/devices_file_io.py

+                attempts += 1
+                time.sleep(RETRY_INTERVAL)
+
+        raise Exception(


Don't raise Exception, raise a specific exception instead.

Tom-Willemsen · 2017-10-11T08:29:12Z

BlockServer/fileIO/file_manager.py

+                attempts += 1
+                time.sleep(RETRY_INTERVAL)
+
+        raise Exception(


Don't raise Exception

Tom-Willemsen · 2017-10-11T08:54:48Z

BlockServer/fileIO/file_manager.py

+                attempts += 1
+                time.sleep(RETRY_INTERVAL)
+
+        raise Exception(


Don't raise Exception

Tom-Willemsen · 2017-10-11T09:59:30Z

BlockServer/fileIO/file_manager.py

+                attempts += 1
+                time.sleep(RETRY_INTERVAL)
+
+        raise Exception(


See if you can factor out this block - I've seen a very similar block 4 times so far. You might need to pass in a function to deal with the different bits but the basic

while attempts < RETRY_MAX_ATTEMPTS: try: with open(file_path, mode) as f: # do something clever except IOError: attempts += 1 time.sleep(RETRY_INTERVAL) #raise something clever

structure should be encapsulated somewhere

Tom-Willemsen · 2017-10-11T10:16:15Z

block_server.py

-        # Start file watcher
-        self._filewatcher = ConfigFileWatcherManager(SCHEMA_DIR, self._config_list, self._syn, self._devices)
+        # self._other_files = UnclassifiedFileManager(self)  TODO
+        # self.on_the_fly_handlers.append(self._other_files)


Remove commented code or complete TODO item.

Tom-Willemsen · 2017-10-11T10:54:23Z

ConfigVersionControl/git_version_control.py

                return
-            except Exception as err:
+            except Exception:
                sleep(RETRY_INTERVAL)

            attempts += 1

        raise CommitToVersionControlException("Couldn't commit to version control")


It looks like this exception never gets caught.

Tom-Willemsen · 2017-10-11T10:55:29Z

ConfigVersionControl/git_version_control.py

+                    push_interval = PUSH_BASE_INTERVAL
+                    first_failure = True
+
+                except GitCommandError as e:


Also need to catch CommitToVersionControlException.

Tom-Willemsen · 2017-10-11T12:15:55Z

ConfigVersionControl/git_version_control.py

@@ -15,19 +15,18 @@
 # http://opensource.org/licenses/eclipse-1.0.php

 import os


Import appears to be unused

It's not, I'm using os.path in a few places here. Not sure why it gets highlighted, I have seen a few people asking the same question online but have not come across a satisfying answer so far

Tom-Willemsen · 2017-10-11T12:27:52Z

ConfigVersionControl/git_version_control.py

+                    push_interval = PUSH_RETRY_INTERVAL
+                    if first_failure:
+                        print_and_log("Unable to push config changes, will retry in %i seconds"
+                                      % PUSH_RETRY_INTERVAL, "MINOR")


Use .format() to format strings

Tom-Willemsen · 2017-10-19T14:45:10Z

Tested adding index.lock in my .git directory so that it couldn't commit:q - it retried and committed with an appropriate message once I released the lock.

Code looks good, functionally works.

Thomas Lohnert added 18 commits October 2, 2017 16:16

Setting up test repository

377f3e6

Clean up properly, add basic tests

1d33db6

Cut old filewatchers from Blockserver

792f1d2

Cut old filewatcher classes and unwanted Git functionality (pull, rev…

180ee58

…ert)

Remove VC manager from Synoptic manager

b9cf2dc

Remove VC manager from Devices manager

adda45a

Removed VC manager from Config manager, holders, constructor calls

768e3f8

Blockserver runs filewatcher

ae7a707

Add all files on startup

53b9d3f

Remove filewatcher, simplify git vc

6a23116

Added commit message provider, set push interval to 5 min

fc4c7d2

tidy up

7e9ac04

Commit messages for scripts

8b7563a

Tests for message provider

b00e6f1

Git add error handling (e.g. add during save operation)

c974fd5

Fix doc, indent

881e1d2

Merge remote-tracking branch 'remotes/origin/master' into Ticket2624_…

415723c

…file_tracking

Add try/except loop to file operations (retry a few times in case fil…

e248e33

…e is in use)

ThomasLohnert mentioned this pull request Oct 10, 2017

BlockServer: Fails with untracked directories ISISComputingGroup/IBEX#2624

Closed

Tom-Willemsen requested changes Oct 11, 2017

View reviewed changes

Thomas Lohnert added 10 commits October 13, 2017 11:35

Sanity check in load devices, added missing return in save devices

fa103f1

Make compatible with correct GitPython version

092c88f

Replaced assert with assertEqual

b3bae1d

String formatting using .format()

635e747

Retry loop as decorator function, better exception handling

df41779

Improve error handling

70f7f29

Deleting synoptic deletes relevant file

184575c

delete configs & comps from file system, formatting

8a8b8ff

Fix tests: add delete method in MockFileManager

9080e91

Use string format()

b1d61ce

Tom-Willemsen approved these changes Oct 19, 2017

View reviewed changes

Tom-Willemsen merged commit 69311d7 into master Oct 19, 2017

ThomasLohnert mentioned this pull request Oct 23, 2017

If devices file does not exist, create & use blank one #142

Merged

7 tasks

ThomasLohnert mentioned this pull request Jan 15, 2018

BlockServer: Error saving new config causes list of configs not to be updated (caused by unmovable git lock) ISISComputingGroup/IBEX#1769

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ticket2624 file tracking #139

Ticket2624 file tracking #139

ThomasLohnert commented Oct 10, 2017 •

edited by Tom-Willemsen

Loading

Tom-Willemsen left a comment

Tom-Willemsen Oct 11, 2017

Tom-Willemsen Oct 11, 2017

Tom-Willemsen Oct 11, 2017

Tom-Willemsen Oct 11, 2017

Tom-Willemsen Oct 11, 2017

Tom-Willemsen Oct 11, 2017

Tom-Willemsen Oct 11, 2017

Tom-Willemsen Oct 11, 2017

Tom-Willemsen Oct 11, 2017

ThomasLohnert Oct 16, 2017

Tom-Willemsen Oct 11, 2017

Tom-Willemsen commented Oct 19, 2017

		@@ -15,19 +15,18 @@
		# http://opensource.org/licenses/eclipse-1.0.php

		import os

Ticket2624 file tracking #139

Ticket2624 file tracking #139

Conversation

ThomasLohnert commented Oct 10, 2017 • edited by Tom-Willemsen Loading

Description of work

To test

Acceptance criteria

Code Review

Functional Tests

Final steps

Tom-Willemsen left a 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

Tom-Willemsen commented Oct 19, 2017

ThomasLohnert commented Oct 10, 2017 •

edited by Tom-Willemsen

Loading