From e41b5656fe7dbf473d71a574bc743bedad533357 Mon Sep 17 00:00:00 2001 From: Ryan Waldheim Date: Thu, 18 Jun 2026 11:50:03 -0700 Subject: [PATCH 1/4] Reapply "Preserve existing users and leaders in NewRepo requests" This reverts commit f6281ed0de206c1d77f60515985f8f2622b782f4. --- ansible-runner/project | 2 +- modules/coactd.py | 41 ++++++++- tests/test_repo_registration_gid.py | 135 ++++++++++++++++++++++++++-- 3 files changed, 168 insertions(+), 10 deletions(-) diff --git a/ansible-runner/project b/ansible-runner/project index d0b52bd..b2ce324 160000 --- a/ansible-runner/project +++ b/ansible-runner/project @@ -1 +1 @@ -Subproject commit d0b52bd0d6274fff6316a3f09e80d014f040cca1 +Subproject commit b2ce32403af61167c34753f6733a90be31beca03 diff --git a/modules/coactd.py b/modules/coactd.py index 634d04f..08a22ef 100644 --- a/modules/coactd.py +++ b/modules/coactd.py @@ -566,18 +566,53 @@ def do_new_repo( self.logger.warning(f"Failed to create grouper POSIX group for {facility}:{repo}: {e}") raise + # Query current users and leaders from database if repo already exists + repo_users = [principal] # Default to just principal for new repos + repo_leaders = [principal] # Default to principal as leader for new repos + repo_req = {'facility': facility, 'name': repo} + REPO_USERS_GQL = gql(""" + query repo($facility: String!, $name: String!) { + repo(filter: {facility: $facility, name: $name}) { + users + leaders + } + } + """) + try: + repo_data = self.back_channel.execute(REPO_USERS_GQL, repo_req) + if repo_data.get('repo'): + # Preserve existing users + if repo_data['repo'].get('users'): + repo_users = repo_data['repo']['users'] + self.logger.info(f"Found existing repo {facility}:{repo} with {len(repo_users)} users: {repo_users}") + # Ensure principal is in the list + if principal not in repo_users: + repo_users.append(principal) + + # Preserve existing leaders + if repo_data['repo'].get('leaders'): + repo_leaders = repo_data['repo']['leaders'] + self.logger.info(f"Found existing repo {facility}:{repo} with {len(repo_leaders)} leaders: {repo_leaders}") + # Ensure principal is in the leaders list + if principal not in repo_leaders: + repo_leaders.append(principal) + except Exception as e: + # Repo doesn't exist yet, use defaults + self.logger.info(f"Repo {facility}:{repo} not found in database, using principal only: {e}") + # run the facility tasks for this repo self.run_playbook( "coact/add_repo.yaml", - facility=facility, + facility=facility, repo=repo, repo_principal=principal, + repo_users=repo_users, gidNumber=repo_gid, groupName=grouper_name ) - leaders = [principal] - users = [principal] + leaders = repo_leaders + users = repo_users # write back to coact the repo information repo_create_req = { diff --git a/tests/test_repo_registration_gid.py b/tests/test_repo_registration_gid.py index b55c8d8..3e60739 100644 --- a/tests/test_repo_registration_gid.py +++ b/tests/test_repo_registration_gid.py @@ -59,6 +59,7 @@ def test_gid_extraction_success_cryoem(self, repo_registration: RepoRegistration # Mock extract_grouper_values to return the GID repo_registration.extract_grouper_values = Mock(return_value=('12345', 'sdf-cryoem-ct-test')) repo_registration.back_channel.execute.side_effect = [ + Exception("Repo not found"), # Query for existing users (repo doesn't exist yet) {'repoUpsert': {'Id': 'repo-456'}}, {'repoUpsertFeature': {'Id': 'feature-slurm'}}, {'repoUpsertFeature': {'Id': 'feature-posix'}} @@ -115,6 +116,7 @@ def test_non_grouper_facility_skips_gid(self, repo_registration, mock_ansible_ru # Setup repo_registration.run_playbook.return_value = mock_ansible_runner repo_registration.back_channel.execute.side_effect = [ + Exception("Repo not found"), # Query for existing users (repo doesn't exist yet) {'repoUpsert': {'Id': 'repo-123'}}, {'repoUpsertFeature': {'Id': 'feature-slurm'}} ] @@ -130,16 +132,22 @@ def test_non_grouper_facility_skips_gid(self, repo_registration, mock_ansible_ru assert result is True # Only add_repo.yaml should run — grouper.yml should NOT be called - # The new implementation passes repo_principal, gidNumber=None, and groupName='' + # The new implementation passes repo_principal, repo_users, gidNumber=None, and groupName='' repo_registration.run_playbook.assert_called_once_with( - 'coact/add_repo.yaml', facility='OTHER', repo='test-repo', repo_principal='test-user', gidNumber=None, groupName='' + 'coact/add_repo.yaml', + facility='OTHER', + repo='test-repo', + repo_principal='test-user', + repo_users=['test-user'], # New parameter added by idempotency fix + gidNumber=None, + groupName='' ) # extract_grouper_values should not be called for non-grouper facilities if hasattr(repo_registration, 'extract_grouper_values') and isinstance(repo_registration.extract_grouper_values, Mock): repo_registration.extract_grouper_values.assert_not_called() - # Should only create slurm feature (2 back_channel calls: repoUpsert + feature) - assert repo_registration.back_channel.execute.call_count == 2 + # Should only create slurm feature (3 back_channel calls: user query + repoUpsert + feature) + assert repo_registration.back_channel.execute.call_count == 3 def test_all_cryoem_repos_use_grouper(self, repo_registration, mock_ansible_runner): """Test that ALL cryoem repos (not just ct/ce) unconditionally use grouper.""" @@ -148,6 +156,7 @@ def test_all_cryoem_repos_use_grouper(self, repo_registration, mock_ansible_runn # Mock extract_grouper_values to return a valid GID for any CryoEM repo repo_registration.extract_grouper_values = Mock(return_value=('54321', 'sdf-cryoem-other-repo')) repo_registration.back_channel.execute.side_effect = [ + Exception("Repo not found"), # Query for existing users (repo doesn't exist yet) {'repoUpsert': {'Id': 'repo-123'}}, {'repoUpsertFeature': {'Id': 'feature-slurm'}}, {'repoUpsertFeature': {'Id': 'feature-posix'}} @@ -176,5 +185,119 @@ def test_all_cryoem_repos_use_grouper(self, repo_registration, mock_ansible_runn repo_registration.logger.info.assert_any_call( "Retrieved repo GID for cryoem:other-repo: 54321" ) - # Should create both slurm and posixgroup features (3 back_channel calls total) - assert repo_registration.back_channel.execute.call_count == 3 + # Should create both slurm and posixgroup features (4 back_channel calls: user query + repoUpsert + 2 features) + assert repo_registration.back_channel.execute.call_count == 4 + + +class TestRepoIdempotency: + """Test idempotency fixes for NewRepo request workflow.""" + + @pytest.fixture + def repo_registration(self) -> RepoRegistration: + """Create a RepoRegistration instance with mocked dependencies.""" + with patch('modules.coactd.GraphQlSubscriber.__init__'), \ + patch('modules.coactd.AnsibleRunner.__init__'): + reg = RepoRegistration( + username='test-user', + password_file='/tmp/test-password', + client_name='test-client', + grouper_password_file='/tmp/test-grouper-password' + ) + reg.logger = Mock() + reg.back_channel = Mock() + reg.run_playbook = Mock() + reg.extract_grouper_values = Mock(return_value=('12345', 'sdf-cryoem-test-repo')) + return reg + + @pytest.fixture + def mock_ansible_runner(self) -> Mock: + """Mock ansible runner with realistic structure.""" + runner = Mock() + runner.events = [] + return runner + + def test_fresh_newrepo_uses_principal_only(self, repo_registration: RepoRegistration, mock_ansible_runner: Mock): + """ + When creating a brand new repo that doesn't exist in the database, + the playbook should receive only the principal user. + """ + # Setup - repo doesn't exist in database + repo_registration.run_playbook.return_value = mock_ansible_runner + repo_registration.back_channel.execute.side_effect = [ + Exception("Repo not found"), # Query fails - repo doesn't exist + {'repoUpsert': {'Id': 'repo-123'}}, # Repo creation succeeds + {'repoUpsertFeature': {'Id': 'feature-slurm'}}, + {'repoUpsertFeature': {'Id': 'feature-posix'}} + ] + + # Execute + result = repo_registration.do_new_repo( + repo='test-new-1', + facility='cryoem', + principal='user1' + ) + + # Verify + assert result is True + + # Check that the playbook was called with only the principal + playbook_call = [c for c in repo_registration.run_playbook.call_args_list + if c[0][0] == 'coact/add_repo.yaml'][0] + assert playbook_call[1]['repo_users'] == ['user1'] + assert playbook_call[1]['repo_principal'] == 'user1' + + # Verify logging indicates repo not found + log_messages = [str(c) for c in repo_registration.logger.info.call_args_list] + assert any('not found in database' in msg for msg in log_messages) + + def test_rerun_newrepo_preserves_existing_users_and_leaders(self, repo_registration: RepoRegistration, mock_ansible_runner: Mock): + """ + Test complete idempotency: re-running NewRepo on an existing repo must preserve + both users and leaders lists, not reset them to just the principal. + """ + # Setup - repo exists with 3 users and 2 leaders + repo_registration.run_playbook.return_value = mock_ansible_runner + repo_registration.back_channel.execute.side_effect = [ + { + 'repo': { + 'users': ['user1', 'user2', 'user3'], + 'leaders': ['user1', 'leader2'] # Multiple leaders + } + }, + {'repoUpsert': {'Id': 'repo-123'}}, + {'repoUpsertFeature': {'Id': 'feature-slurm'}}, + {'repoUpsertFeature': {'Id': 'feature-posix'}} + ] + + # Execute - re-run NewRepo for existing repo + result = repo_registration.do_new_repo( + repo='test-existing', + facility='cryoem', + principal='user1' + ) + + # Verify result + assert result is True + + # Verify Ansible playbook received ALL existing users (for LDAP sync) + playbook_call = [c for c in repo_registration.run_playbook.call_args_list + if c[0][0] == 'coact/add_repo.yaml'][0] + assert set(playbook_call[1]['repo_users']) == {'user1', 'user2', 'user3'}, \ + "Ansible playbook must receive all users for LDAP sync" + + # The second back_channel.execute call is the repoUpsert mutation + upsert_call = repo_registration.back_channel.execute.call_args_list[1] + upsert_data = upsert_call[0][1] # Second argument is the variables + + assert set(upsert_data['repo']['users']) == {'user1', 'user2', 'user3'}, \ + f"Database write must preserve all users, not reset to principal! Got: {upsert_data['repo']['users']}" + + assert set(upsert_data['repo']['leaders']) == {'user1', 'leader2'}, \ + f"Database write must preserve all leaders, not reset to principal! Got: {upsert_data['repo']['leaders']}" + + # Verify logging indicates existing data was found + log_messages = [str(c) for c in repo_registration.logger.info.call_args_list] + assert any('Found existing repo' in msg and '3 users' in msg for msg in log_messages), \ + "Should log that existing users were found" + assert any('2 leaders' in msg for msg in log_messages), \ + "Should log that existing leaders were found" From 0fbccec554a081e22dee346bf00f54e62543626d Mon Sep 17 00:00:00 2001 From: Ryan Waldheim Date: Thu, 18 Jun 2026 14:02:26 -0700 Subject: [PATCH 2/4] don't proceed with playbook execution if graphql fails --- modules/coactd.py | 18 ++++++++++++++---- tests/test_repo_registration_gid.py | 8 ++++---- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/modules/coactd.py b/modules/coactd.py index 08a22ef..53aedad 100644 --- a/modules/coactd.py +++ b/modules/coactd.py @@ -580,8 +580,12 @@ def do_new_repo( """) try: repo_data = self.back_channel.execute(REPO_USERS_GQL, repo_req) - if repo_data.get('repo'): - # Preserve existing users + + # repo: null means "not found" - use defaults + if repo_data.get('repo') is None: + self.logger.info(f"Repo {facility}:{repo} not found in database, creating new with principal only") + elif repo_data.get('repo'): + # Repo exists - preserve existing users and leaders if repo_data['repo'].get('users'): repo_users = repo_data['repo']['users'] self.logger.info(f"Found existing repo {facility}:{repo} with {len(repo_users)} users: {repo_users}") @@ -597,8 +601,14 @@ def do_new_repo( if principal not in repo_leaders: repo_leaders.append(principal) except Exception as e: - # Repo doesn't exist yet, use defaults - self.logger.info(f"Repo {facility}:{repo} not found in database, using principal only: {e}") + # Query failed due to other error - do not proceed + error_msg = f"Failed to query existing repo data for {facility}:{repo}: {e}" + self.logger.error(error_msg) + raise RuntimeError( + f"Cannot safely proceed with NewRepo: database query failed. " + f"Proceeding with defaults could overwrite existing users/leaders. " + f"Original error: {e}" + ) from e # run the facility tasks for this repo self.run_playbook( diff --git a/tests/test_repo_registration_gid.py b/tests/test_repo_registration_gid.py index 3e60739..e3eea4e 100644 --- a/tests/test_repo_registration_gid.py +++ b/tests/test_repo_registration_gid.py @@ -59,7 +59,7 @@ def test_gid_extraction_success_cryoem(self, repo_registration: RepoRegistration # Mock extract_grouper_values to return the GID repo_registration.extract_grouper_values = Mock(return_value=('12345', 'sdf-cryoem-ct-test')) repo_registration.back_channel.execute.side_effect = [ - Exception("Repo not found"), # Query for existing users (repo doesn't exist yet) + {'repo': None}, # Query returns null (repo not found) {'repoUpsert': {'Id': 'repo-456'}}, {'repoUpsertFeature': {'Id': 'feature-slurm'}}, {'repoUpsertFeature': {'Id': 'feature-posix'}} @@ -116,7 +116,7 @@ def test_non_grouper_facility_skips_gid(self, repo_registration, mock_ansible_ru # Setup repo_registration.run_playbook.return_value = mock_ansible_runner repo_registration.back_channel.execute.side_effect = [ - Exception("Repo not found"), # Query for existing users (repo doesn't exist yet) + {'repo': None}, # Query returns null (repo not found) {'repoUpsert': {'Id': 'repo-123'}}, {'repoUpsertFeature': {'Id': 'feature-slurm'}} ] @@ -156,7 +156,7 @@ def test_all_cryoem_repos_use_grouper(self, repo_registration, mock_ansible_runn # Mock extract_grouper_values to return a valid GID for any CryoEM repo repo_registration.extract_grouper_values = Mock(return_value=('54321', 'sdf-cryoem-other-repo')) repo_registration.back_channel.execute.side_effect = [ - Exception("Repo not found"), # Query for existing users (repo doesn't exist yet) + {'repo': None}, # Query returns null (repo not found) {'repoUpsert': {'Id': 'repo-123'}}, {'repoUpsertFeature': {'Id': 'feature-slurm'}}, {'repoUpsertFeature': {'Id': 'feature-posix'}} @@ -224,7 +224,7 @@ def test_fresh_newrepo_uses_principal_only(self, repo_registration: RepoRegistra # Setup - repo doesn't exist in database repo_registration.run_playbook.return_value = mock_ansible_runner repo_registration.back_channel.execute.side_effect = [ - Exception("Repo not found"), # Query fails - repo doesn't exist + {'repo': None}, # Normal "not found" {'repoUpsert': {'Id': 'repo-123'}}, # Repo creation succeeds {'repoUpsertFeature': {'Id': 'feature-slurm'}}, {'repoUpsertFeature': {'Id': 'feature-posix'}} From 8c59e6539406e0a06e7f53da18dc16cd19088670 Mon Sep 17 00:00:00 2001 From: Ryan Waldheim Date: Thu, 18 Jun 2026 14:23:41 -0700 Subject: [PATCH 3/4] update submodule --- ansible-runner/project | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ansible-runner/project b/ansible-runner/project index b2ce324..a200354 160000 --- a/ansible-runner/project +++ b/ansible-runner/project @@ -1 +1 @@ -Subproject commit b2ce32403af61167c34753f6733a90be31beca03 +Subproject commit a2003544b1d62f2668efe04bc9de9fd275d51a78 From 8a836a891c5c41ef25095391e26ead1cf89924bf Mon Sep 17 00:00:00 2001 From: Ryan Waldheim Date: Fri, 19 Jun 2026 13:01:04 -0700 Subject: [PATCH 4/4] update submodule --- ansible-runner/project | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ansible-runner/project b/ansible-runner/project index a200354..9497aed 160000 --- a/ansible-runner/project +++ b/ansible-runner/project @@ -1 +1 @@ -Subproject commit a2003544b1d62f2668efe04bc9de9fd275d51a78 +Subproject commit 9497aed512ae9bd1d42b088fae748ddc2a24bc9a