Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CP-49775 convert SMGC to systemd service #692

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ install: precheck
$(SM_STAGING)/$(SYSTEMD_SERVICE_DIR)
install -m 644 systemd/sr_health_check.timer \
$(SM_STAGING)/$(SYSTEMD_SERVICE_DIR)
install -m 644 systemd/[email protected] \
$(SM_STAGING)/$(SYSTEMD_SERVICE_DIR)
for i in $(UDEV_RULES); do \
install -m 644 udev/$$i.rules \
$(SM_STAGING)$(UDEV_RULES_DIR); done
Expand Down
2 changes: 1 addition & 1 deletion drivers/FileSR.py
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ def _process_replay(self, data):

def _kickGC(self):
util.SMlog("Kicking GC")
cleanup.start_gc(self.session, self.uuid)
cleanup.start_gc_service(self.uuid)

def _isbind(self):
# os.path.ismount can't deal with bind mount
Expand Down
2 changes: 1 addition & 1 deletion drivers/LVHDSR.py
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,7 @@ def _prepareTestMode(self):

def _kickGC(self):
util.SMlog("Kicking GC")
cleanup.start_gc(self.session, self.uuid)
cleanup.start_gc_service(self.uuid)

def ensureCBTSpace(self):
# Ensure we have space for at least one LV
Expand Down
40 changes: 32 additions & 8 deletions drivers/cleanup.py
Original file line number Diff line number Diff line change
Expand Up @@ -3206,6 +3206,10 @@ def gc(session, srUuid, inBackground, dryRun=False):


def start_gc(session, sr_uuid):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this code being retained? I don't see any callers, and the implementation of get_state implies that we only expect the systemd version to be called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly to make it easy for someone who wants to revert to a non-systemd method of doing things to do so
(yes, they'd have to reimplement get-state but the original method of doing so has lock-ordering issues, so leaving it would not have been a service)

"""
This function is used to try to start a backgrounded GC session by forking
the current process. If using the systemd version, call start_gc_service() instead.
"""
# don't bother if an instance already running (this is just an
# optimization to reduce the overhead of forking a new process if we
# don't have to, but the process will check the lock anyways)
Expand All @@ -3230,6 +3234,24 @@ def start_gc(session, sr_uuid):
subprocess.run([__file__, '-b', '-u', sr_uuid, '-g'],
stdout=subprocess.PIPE, stderr=subprocess.PIPE, close_fds=True)

def start_gc_service(sr_uuid, wait=False):
"""
This starts the templated systemd service which runs GC on the given SR UUID.
If the service was already started, this is a no-op.
Because the service is a one-shot with RemainAfterExit=no, when called with
wait=True this will run the service synchronously and will not return until the
run has finished. This is used to force a run of the GC instead of just kicking it
in the background.
"""
sr_uuid_esc = sr_uuid.replace("-", "\\x2d")
util.SMlog(f"Kicking SMGC@{sr_uuid}...")
cmd=[ "/usr/bin/systemctl", "--quiet" ]
if not wait:
cmd.append("--no-block")
cmd += ["start", f"SMGC@{sr_uuid_esc}"]
subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, close_fds=True)


def gc_force(session, srUuid, force=False, dryRun=False, lockSR=False):
"""Garbage collect all deleted VDIs in SR "srUuid". The caller must ensure
Expand Down Expand Up @@ -3263,15 +3285,17 @@ def gc_force(session, srUuid, force=False, dryRun=False, lockSR=False):


def get_state(srUuid):
"""Return whether GC/coalesce is currently running or not. The information
is not guaranteed for any length of time if the call is not protected by
locking.
"""Return whether GC/coalesce is currently running or not. This asks systemd for
the state of the templated SMGC service and will return True if it is "activating"
or "running" (for completeness, as in practice it will never achieve the latter state)
"""
init(srUuid)
if lockGCActive.acquireNoblock():
lockGCActive.release()
return False
return True
sr_uuid_esc = srUuid.replace("-", "\\x2d")
cmd=[ "/usr/bin/systemctl", "is-active", f"SMGC@{sr_uuid_esc}"]
result = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, close_fds=True)
state = result.stdout.decode('utf-8').rstrip()
if state == "activating" or state == "running":
return True
return False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, do we need to consider the case where systemd is not being used? I'm inclined to say not as I think all consumers of this code are now using systemd



def should_preempt(session, srUuid):
Expand Down
1 change: 1 addition & 0 deletions mk/sm.spec.in
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ tests/run_python_unittests.sh
%{_unitdir}/usb-scan.socket
%{_unitdir}/mpathcount.service
%{_unitdir}/mpathcount.socket
%{_unitdir}/[email protected]
%config /etc/udev/rules.d/65-multipath.rules
%config /etc/udev/rules.d/55-xs-mpath-scsidev.rules
%config /etc/udev/rules.d/58-xapi.rules
Expand Down
12 changes: 12 additions & 0 deletions systemd/[email protected]
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[Unit]
Description=Garbage Collector for SR %I
DefaultDependencies=no

[Service]
Type=oneshot
Restart=no
ExecStart=/opt/xensource/sm/cleanup.py -g -u %I
# This is the default, but just to make it clear we may run this
# service multiple times. When running, it will show as "activating";
# when not running, it will show as "dead"
RemainAfterExit=no
Loading