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.

Sign up for GitHub

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

Backport PR #882 on branch 7.x (Fix connection reconciliation to handle restarts) #883

Merged
merged 1 commit into from
Nov 16, 2022
Merged

Backport PR #882 on branch 7.x (Fix connection reconciliation to handle restarts) #883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 14 additions & 13 deletions jupyter_client/connect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,8 @@ def _reconcile_connection_info(self, info: KernelConnectionInfo) -> None:
Because some provisioners (like derivations of LocalProvisioner) may have already
written the connection file, this method needs to ensure that, if the connection
file exists, its contents match that of what was returned by the provisioner. If
the file does exist and its contents do not match, a ValueError is raised.
the file does exist and its contents do not match, the file will be replaced with
the provisioner information (which is considered the truth).

If the file does not exist, the connection information in 'info' is loaded into the
KernelManager and written to the file.
Expand All @@ -586,24 +587,24 @@ def _reconcile_connection_info(self, info: KernelConnectionInfo) -> None:
if os.path.exists(self.connection_file):
with open(self.connection_file) as f:
file_info = json.load(f)
# Prior to the following comparison, we need to adjust the value of "key" to
# be bytes, otherwise the comparison below will fail.
file_info["key"] = file_info["key"].encode()
if not self._equal_connections(info, file_info):
raise ValueError(
"Connection file already exists and does not match "
"the expected values returned from provisioner!"
)
# Prior to the following comparison, we need to adjust the value of "key" to
# be bytes, otherwise the comparison below will fail.
file_info["key"] = file_info["key"].encode()
if not self._equal_connections(info, file_info):
os.remove(self.connection_file) # Contents mismatch - remove the file
self._connection_file_written = False
else:
file_exists = True

if not file_exists:
# Load the connection info and write out file. Note, this does not necessarily
# overwrite non-zero port values, so we'll validate afterward.
# Load the connection info and write out file, clearing existing
# port-based attributes so they will be reloaded
for name in port_names:
setattr(self, name, 0)
self.load_connection_info(info)
self.write_connection_file()

# Ensure what is in KernelManager is what we expect. This will catch issues if the file
# already existed, yet it's contents differed from the KernelManager's (and provisioner).
# Ensure what is in KernelManager is what we expect.
km_info = self.get_connection_info()
if not self._equal_connections(info, km_info):
raise ValueError(
Expand Down
38 changes: 15 additions & 23 deletions jupyter_client/tests/test_connect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,15 +240,15 @@ def test_mixin_cleanup_random_ports():


param_values = [
(True, True, None),
(True, False, ValueError),
(False, True, None),
(False, False, ValueError),
(True, True),
(True, False),
(False, True),
(False, False),
]


@pytest.mark.parametrize("file_exists, km_matches, expected_exception", param_values)
def test_reconcile_connection_info(file_exists, km_matches, expected_exception):
@pytest.mark.parametrize("file_exists, km_matches", param_values)
def test_reconcile_connection_info(file_exists, km_matches):

expected_info = sample_info
mismatched_info = sample_info.copy()
Expand All @@ -272,9 +272,10 @@ def test_reconcile_connection_info(file_exists, km_matches, expected_exception):
provisioner_info = info
km.load_connection_info(provisioner_info)
else:
# Let this be the case where the connection file exists, the KM has no values
# prior to reconciliation, but the provisioner has returned different values
# and a ValueError is expected.
# Let this be the case where the connection file exists, and the KM has those values
# that differ from the ones returned by the provisioner. This is the restart-with-
# changed-ports case (typical for remote provisioners).
km.load_connection_info(expected_info)
provisioner_info = mismatched_info
else: # connection file does not exist
if km_matches:
Expand All @@ -284,20 +285,11 @@ def test_reconcile_connection_info(file_exists, km_matches, expected_exception):
provisioner_info = expected_info
else:
# Let this be the case where the connection file does not exist, yet the KM
# has values that do not match those returned from the provisioner and a
# ValueError is expected.
# has values that do not match those returned from the provisioner. This case
# is probably not practical and is equivalent to the True, False case.
km.load_connection_info(expected_info)
provisioner_info = mismatched_info

if expected_exception is None:
km._reconcile_connection_info(provisioner_info)
km_info = km.get_connection_info()
assert km._equal_connections(km_info, provisioner_info)
else:
with pytest.raises(expected_exception) as ex:
km._reconcile_connection_info(provisioner_info)
if file_exists:
assert "Connection file already exists" in str(ex.value)
else:
assert "KernelManager's connection information already exists" in str(ex.value)
assert km._equal_connections(km.get_connection_info(), provisioner_info) is False
km._reconcile_connection_info(provisioner_info)
km_info = km.get_connection_info()
assert km._equal_connections(km_info, provisioner_info)