Better handling of concurrency#63
Draft
amarao wants to merge 33 commits into
Draft
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Servers.com dynamic inventory plugin (sc_inventory) with extensive unit/integration test coverage and adds reusable GitHub Actions workflows to better serialize bare-metal-dependent integration runs (concurrency handling).
Changes:
- Added
serverscom.sc_api.sc_inventoryinventory plugin supporting multi-kind discovery, filtering, grouping, and${VAR}substitution. - Added unit + integration tests (including new integration target for the inventory plugin) plus example inventory configs.
- Refactored CI workflow structure for baremetal/L2 integration tests using reusable workflows and shared concurrency control.
Reviewed changes
Copilot reviewed 41 out of 43 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/dynamic_inventory/01_everything.sc_api.yml | Example config: fetch all kinds with no resources: blocks. |
| examples/dynamic_inventory/02_all_cloud_grouped_by_region.sc_api.yml | Example config: cloud instances grouped by region_code. |
| examples/dynamic_inventory/03_all_baremetal_grouped_by_location.sc_api.yml | Example config: bare-metal grouped by location_code. |
| examples/dynamic_inventory/04_baremetal_in_specific_regions.sc_api.yml | Example config: bare-metal filtered to specific regions. |
| examples/dynamic_inventory/05_cloud_labels_private_ip.sc_api.yml | Example config: cloud filtered by labels, using private IPv4. |
| examples/dynamic_inventory/06_exclusion_ams1_except_production.sc_api.yml | Example config: exclusion rules (drop a labeled subset). |
| examples/dynamic_inventory/07_multi_rule_exclusion.sc_api.yml | Example config: multiple exclusion rules (OR). |
| examples/dynamic_inventory/08_env_var_substitution.sc_api.yml | Example config: ${VAR} substitution usage. |
| examples/dynamic_inventory/09_k8s_nodes.sc_api.yml | Example config: k8s bare-metal nodes with private IP addressing. |
| examples/dynamic_inventory/10_multi_kind_inventory.sc_api.yml | Example config: multiple resources blocks with per-kind settings. |
| examples/dynamic_inventory/11_name_regex_two_kinds_same_group.sc_api.yml | Example config: name regex across kinds into same group. |
| ansible_collections/serverscom/sc_api/plugins/inventory/sc_inventory.py | New inventory plugin implementation (filtering/grouping/env subst/strict key validation). |
| ansible_collections/serverscom/sc_api/plugins/inventory/init.py | Inventory package marker for the collection. |
| ansible_collections/serverscom/sc_api/tests/unit/plugins/inventory/test_sc_inventory.py | Unit tests for inventory plugin behavior and validation. |
| ansible_collections/serverscom/sc_api/tests/unit/plugins/inventory/init.py | Unit test package marker. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/tasks/main.yaml | Integration test scenario provisioning + assertions for inventory output. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/tasks/_run.yaml | Helper task file to render inventory config and run ansible-inventory. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/unknown_top_key.sc_api.yml.j2 | Template: unknown top-level key validation test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/unknown_sub_key.sc_api.yml.j2 | Template: unknown resources[] key validation test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/unknown_exclude_key.sc_api.yml.j2 | Template: unknown exclude-rule key validation test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/same_group_combine.sc_api.yml.j2 | Template: merge behavior for same group across multiple blocks. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/region_nomatch.sc_api.yml.j2 | Template: region filter negative case. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/region_match.sc_api.yml.j2 | Template: region filter positive case. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/name_regex.sc_api.yml.j2 | Template: name regex filtering. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/missing_kind.sc_api.yml.j2 | Template: missing kind contract validation. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/label_match.sc_api.yml.j2 | Template: label filter test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/group_static.sc_api.yml.j2 | Template: assign_inventory_group test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/group_by_location.sc_api.yml.j2 | Template: group_by test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/exclude_label.sc_api.yml.j2 | Template: exclusion by label test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/env_subst.sc_api.yml.j2 | Template: ${VAR} substitution success test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/env_subst_missing.sc_api.yml.j2 | Template: ${VAR} substitution missing-var failure test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/default_no_resources.sc_api.yml.j2 | Template: default “no resources” behavior test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/cross_kind_group.sc_api.yml.j2 | Template: same group across kinds (baremetal+cloud). |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/cloud_ipv6_host.sc_api.yml.j2 | Template: cloud ansible_host selection using IPv6. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/cloud_filter.sc_api.yml.j2 | Template: cloud filtering by label. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/baremetal_basic.sc_api.yml.j2 | Template: baremetal-only selection. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/ansible_host_public.sc_api.yml.j2 | Template: baremetal ansible_host mapping test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sc_inventory/templates/all_kinds.sc_api.yml.j2 | Template: explicit multi-kind inventory test. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sbm_server_lifecycle/tasks/test_inventory_discovery.yaml | New integration test verifying SBM discovery via sc_inventory. |
| ansible_collections/serverscom/sc_api/tests/integration/targets/sbm_server_lifecycle/tasks/main.yaml | Adds inventory discovery test into SBM lifecycle integration flow. |
| .github/workflows/tests.yaml | Switches to reusable workflows for baremetal/L2 and updates job dependencies. |
| .github/workflows/baremetal.yaml | New reusable workflow for baremetal integration tests + inventory target. |
| .github/workflows/l2.yaml | New reusable workflow for L2 integration tests, sharing baremetal concurrency group. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+316
to
+336
| def verify_file(self, path): | ||
| if not super(InventoryModule, self).verify_file(path): | ||
| return False | ||
| if not path.endswith( | ||
| ( | ||
| ".sc_api.yml", | ||
| ".sc_api.yaml", | ||
| ".sc_inventory.yml", | ||
| ".sc_inventory.yaml", | ||
| ) | ||
| ): | ||
| return False | ||
| # Must also contain `plugin: serverscom.sc_api.sc_inventory` so we | ||
| # don't claim foreign files that just happen to share the suffix. | ||
| try: | ||
| with open(path, "r") as f: | ||
| content = f.read() | ||
| except (OSError, IOError): | ||
| return False | ||
| return "plugin: %s" % self.NAME in content | ||
|
|
| ) | ||
| extra_vars = config.get("extra_vars") or {} | ||
|
|
||
| compiled_regex = re.compile(name_regex) if name_regex else None |
Comment on lines
+110
to
+120
| baremetal: | ||
| needs: | ||
| - unit_and_sanity | ||
| - lint | ||
| concurrency: | ||
| group: BM1 | ||
| cancel-in-progress: false | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: "3.13" | ||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| - name: Configure integration tests | ||
| run: | | ||
| envsubst < integration_config.yml.template > ansible_collections/serverscom/sc_api/tests/integration/integration_config.yml | ||
| env: | ||
| SC_TOKEN: "${{ secrets.SC_TOKEN }}" | ||
| - name: Integration tests (server1) | ||
| run: ansible-test integration --requirements --python 3.13 --color yes | ||
| ssh_key | ||
| baremetal_locations_info | ||
| dedicated_server_reinstall_quick | ||
| dedicated_server_reinstall_1 | ||
| dedicated_server_power | ||
| uses: ./.github/workflows/baremetal.yaml | ||
| secrets: inherit | ||
|
|
||
| working-directory: ansible_collections/serverscom/sc_api | ||
| - name: Cleanup secrets | ||
| if: always() | ||
| run: | | ||
| dd if=/dev/zero bs=4k count=4 of=ansible_collections/serverscom/sc_api/tests/integration/integration_config.yml | ||
| sync | ||
| sync | ||
|
|
||
| integration_tests_bm_2: | ||
| runs-on: ubuntu-24.04 | ||
| needs: | ||
| - unit_and_sanity | ||
| - lint | ||
| concurrency: | ||
| group: BM2 | ||
| cancel-in-progress: false | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.13" | ||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| - name: Configure integration tests | ||
| run: | | ||
| envsubst < integration_config.yml.template > ansible_collections/serverscom/sc_api/tests/integration/integration_config.yml | ||
| env: | ||
| SC_TOKEN: "${{ secrets.SC_TOKEN }}" | ||
| - name: Integration tests (server2) | ||
| run: ansible-test integration --requirements --python 3.13 --color yes | ||
| baremetal_servers_info | ||
| dedicated_server_reinstall_2 | ||
| dedicated_server_rescue | ||
| dedicated_server_reinstall_raid0 | ||
|
|
||
| working-directory: ansible_collections/serverscom/sc_api | ||
| - name: Cleanup secrets | ||
| if: always() | ||
| run: | | ||
| dd if=/dev/zero bs=4k count=4 of=ansible_collections/serverscom/sc_api/tests/integration/integration_config.yml | ||
| sync | ||
| sync | ||
|
|
||
| integration_tests_bm_3: | ||
| runs-on: ubuntu-24.04 | ||
| needs: | ||
| - unit_and_sanity | ||
| - lint | ||
| concurrency: | ||
| group: BM3 | ||
| cancel-in-progress: false | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.13" | ||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| - name: Configure integration tests | ||
| run: | | ||
| envsubst < integration_config.yml.template > ansible_collections/serverscom/sc_api/tests/integration/integration_config.yml | ||
| env: | ||
| SC_TOKEN: "${{ secrets.SC_TOKEN }}" | ||
| - name: Integration tests (server3) | ||
| run: ansible-test integration --requirements --python 3.13 --color yes | ||
| baremetal_os_list | ||
| dedicated_server_info | ||
| dedicated_server_reinstall_3 | ||
| dedicated_server_ipxe | ||
| dedicated_server_reinstall_noraid | ||
|
|
||
| working-directory: ansible_collections/serverscom/sc_api | ||
| - name: Cleanup secrets | ||
| if: always() | ||
| run: | | ||
| dd if=/dev/zero bs=4k count=4 of=ansible_collections/serverscom/sc_api/tests/integration/integration_config.yml | ||
| sync | ||
| sync | ||
|
|
||
| integration_tests_l2: | ||
| runs-on: ubuntu-24.04 | ||
| needs: | ||
| - integration_tests_bm_1 | ||
| - integration_tests_bm_2 | ||
| - integration_tests_bm_3 | ||
| concurrency: | ||
| group: BM | ||
| cancel-in-progress: false | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: "3.13" | ||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| - name: Configure integration tests | ||
| run: | | ||
| envsubst < integration_config.yml.template > ansible_collections/serverscom/sc_api/tests/integration/integration_config.yml | ||
| env: | ||
| SC_TOKEN: "${{ secrets.SC_TOKEN }}" | ||
| - name: Integration tests | ||
| run: ansible-test integration --requirements --python 3.13 --color yes | ||
| l2_segment | ||
| l2_segment_aliases | ||
| l2_segments_info | ||
|
|
||
| working-directory: ansible_collections/serverscom/sc_api | ||
| - name: Cleanup secrets | ||
| if: always() | ||
| run: | | ||
| dd if=/dev/zero bs=4k count=4 of=ansible_collections/serverscom/sc_api/tests/integration/integration_config.yml | ||
| sync | ||
| sync | ||
| l2: | ||
| needs: baremetal | ||
| uses: ./.github/workflows/l2.yaml | ||
| secrets: inherit |
Comment on lines
+11
to
+59
| - name: Render sc_inventory config (kind=sbm) | ||
| copy: | ||
| content: | | ||
| plugin: serverscom.sc_api.sc_inventory | ||
| resources: | ||
| - kind: sbm | ||
| dest: "{{ inv_tmp.path }}/sbm.sc_api.yml" | ||
| mode: "0600" | ||
|
|
||
| - name: Run ansible-inventory against the sc_inventory config | ||
| command: | ||
| argv: | ||
| - ansible-inventory | ||
| - -i | ||
| - "{{ inv_tmp.path }}/sbm.sc_api.yml" | ||
| - --list | ||
| environment: | ||
| SERVERSCOM_API_TOKEN: "{{ sc_token }}" | ||
| SERVERSCOM_API_URL: "{{ sc_endpoint }}" | ||
| register: inv_run | ||
| changed_when: false | ||
|
|
||
| - name: Stash inventory JSON to a file | ||
| copy: | ||
| content: "{{ inv_run.stdout }}" | ||
| dest: "{{ inv_tmp.path }}/inventory.json" | ||
| mode: "0600" | ||
| changed_when: false | ||
|
|
||
| - name: Load inventory JSON | ||
| include_vars: | ||
| file: "{{ inv_tmp.path }}/inventory.json" | ||
| name: inv_json | ||
|
|
||
| - name: Created SBM server is discovered by sc_inventory | ||
| assert: | ||
| that: | ||
| - test_hostname in inv_json._meta.hostvars | ||
| - inv_json._meta.hostvars[test_hostname].sc_kind == "sbm" | ||
| - inv_json._meta.hostvars[test_hostname].type == "sbm_server" | ||
| fail_msg: >- | ||
| Expected to find SBM host {{ test_hostname }} in sc_inventory output. | ||
| hostvars seen: {{ inv_json._meta.hostvars | list }} | ||
|
|
||
| - name: Cleanup inventory temp dir | ||
| file: | ||
| path: "{{ inv_tmp.path }}" | ||
| state: absent | ||
| changed_when: false |
Implements a new Ansible module for managing iPXE boot configuration on dedicated servers. Supports public/private iPXE modes with automatic deactivation of the opposite feature. Includes unit tests, integration tests, and API layer extensions.
Extract environment-specific RBS (Block Storage) values from integration tests into integration_config.yml.template for portability across test environments. New variables: rbs_test_location_id, rbs_test_location_code, rbs_test_flavor_id, rbs_test_flavor_name. Affected test targets: rbs_flavors_info, rbs_volume, rbs_volume_info, rbs_volume_credentials_reset. Co-Authored-By: Claude Opus 4.6
Extract environment-specific Load Balancer values from integration tests into integration_config.yml.template for portability across test environments. New variables: lb_test_location_id, lb_test_upstream_ip1, lb_test_upstream_ip2, lb_test_network. Affected test targets: load_balancer_instance_l4, load_balancer_instance_l7, load_balancer_instance_info, load_balancer_instances_list. Co-Authored-By: Claude Opus 4.6
…fig.yml Extract environment-specific Cloud Computing values from integration tests into integration_config.yml.template for portability across test environments. New variables: cloud_test_region_id, cloud_test_flavor_id, cloud_test_flavor_name, cloud_test_ssh_key_fingerprint, cloud_test_region_search_pattern, cloud_test_region_search_match, cloud_test_region_search_nomatch. Affected test targets: cloud_computing_instance, cloud_computing_instance_info, cloud_computing_instance_ptr, cloud_computing_instance_state, cloud_computing_instance_reinstall, cloud_computing_instances_info, cloud_computing_regions_info. Co-Authored-By: Claude Opus 4.6
…_config.yml Extract environment-specific Baremetal and Dedicated Server values from integration tests into integration_config.yml.template. New variables: baremetal_test_location_search_pattern, baremetal_test_location_search_match, baremetal_test_location_search_nomatch, baremetal_test_os_location_id, baremetal_test_os_location_code, baremetal_test_os_server_model_id, baremetal_test_os_server_model_name, dedicated_test_reinstall_os_id, dedicated_test_reinstall_ssh_key_fingerprint, dedicated_test_reinstall_ssh_key_name. Also fix hardcoded non_existing_id value in dedicated_server_reinstall_quick assertion. Affected test targets: baremetal_locations_info, baremetal_os_list, dedicated_server_reinstall_long, dedicated_server_reinstall_quick. Co-Authored-By: Claude Opus 4.6
Extract environment-specific SBM (Scalable Baremetal) values from integration tests into integration_config.yml.template. New variables: sbm_test_location_code, sbm_test_flavor_name, sbm_test_os_regex, sbm_test_reinstall_os_name. Affected test targets: sbm_server_lifecycle, sbm_flavor_models_info, sbm_os_list, sbm_servers_info. Co-Authored-By: Claude Opus 4.6
Also, add a test for that. Also, a test for proper joining of the same group kinds for the same group.
- verify_file content check (sc_inventory.py): now reads the file and confirms plugin: serverscom.sc_api.sc_inventory is present in the body. Foreign .sc_api.yml files with a different plugin: key, or no plugin: line at all, are rejected at discovery time rather than reaching parse(). - Unit tests for verify_file rewritten to use real temp files + 3 new cases (wrong-plugin-key, missing-plugin-key, unreadable-path). - Integration cloud test: added public_ipv6 == cloud_vm.public_ipv6_address assertion on the cloud-filter check, so the IPv6 hostvar exposure is verified end-to-end (not just ansible_host: public_ipv6 in the v6 follow-up test).
The problem: PXE-boot tests and L2-segment tests share the same baremetal fleet, and PXE is incompatible with a server being in an L2 segment. Within one workflow run, needs: correctly ordered BM before L2, but the per-job concurrency groups (BM1/BM2/BM3/BM) didn't overlap across parallel workflow runs. Example failure: push #1's integration_tests_l2 is mid-run (servers attached to an L2 segment); push #2 lands and its integration_tests_bm_1 immediately starts (group BM1 is free), attempting a PXE boot on a server still in run #1's L2 segment — PXE fails or produces undefined state. Fix: BM jobs and L2 are moved into two reusable workflows that share workflow-level concurrency: baremetal, so the entire BM phase of run #2 queues until run #1's BM and L2 phases have both released the fleet, while server1/2/3 still run in parallel within a run.
amarao
force-pushed
the
better_handling_of_concurrency
branch
from
78f4701 to
e6b9fb8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.