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

multiple custom actions validators #5134

Merged
merged 39 commits into from
Apr 17, 2023

Conversation

psacc
Copy link
Contributor

@psacc psacc commented Mar 28, 2023

Description of changes

  • add support for Resources async validation in addition to sync validation
  • add support for async validators timeout, caching and retry
  • UrlValidators use async execution with caching and retry
  • Multiple custom actions resources uses async UrlValidators with timeout (and caching + retry)

Tests

  • manual local test: cluster creation with multiple custom actions
  • added coverage for multiple custom actions in tests/integration-tests/tests/pcluster_api/test_api.py (testing async multithreaded validators in lambda)
  • local integration tests execution:
test-suites:
  create:
    test_create.py::test_cluster_creation_with_problematic_preinstall_script:
      dimensions:
        - regions: ["eu-west-1"]
          instances: {{ common.INSTANCES_DEFAULT_ARM }}
          schedulers: ["slurm"]
          oss: ["alinux2"]
  cfn-init:
    test_cfn_init.py::test_replace_compute_on_failure:
      dimensions:
        - regions: ["eu-west-1"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["alinux2"]
          schedulers: ["slurm"]
    test_cfn_init.py::test_install_args_quotes:
      dimensions:
        - regions: ["eu-west-1"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["alinux2"]
          schedulers: ["slurm"]
  pcluster_api:
    test_api_infrastructure.py::test_api_infrastructure_with_default_parameters:
      dimensions:
        - regions: ["eu-west-1"]
    test_api.py::test_cluster_slurm:
      dimensions:
        - regions: ["eu-west-1"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["alinux2"]
          schedulers: ["slurm"]
    test_api.py::test_cluster_awsbatch:
      dimensions:
        - regions: ["eu-west-1"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["alinux2"]
          schedulers: ["awsbatch"]

References

Checklist

  • Make sure you are pointing to the right branch and add a label in the PR title (i.e. 2.x vs 3.x)
  • Check all commits' messages are clear, describing what and why vs how.
  • Make sure to have added unit tests or integration tests to cover the new/modified code.
  • Check if documentation is impacted by this change.

Please review the guidelines for contributing and Pull Request Instructions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@psacc psacc added skip-changelog-update Disables the check that enforces changelog updates in PRs 3.x labels Mar 28, 2023
@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #5134 (b4899ad) into develop (0704826) will increase coverage by 0.07%.
The diff coverage is n/a.

❗ Current head b4899ad differs from pull request most recent head b6797a0. Consider uploading reports for the commit b6797a0 to get more accurate results

@@             Coverage Diff             @@
##           develop    #5134      +/-   ##
===========================================
+ Coverage    89.65%   89.72%   +0.07%     
===========================================
  Files          175      175              
  Lines        15131    15241     +110     
===========================================
+ Hits         13565    13675     +110     
  Misses        1566     1566              
Flag Coverage Δ
unittests 89.72% <ø> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cli/src/pcluster/utils.py Fixed Show fixed Hide fixed
eantonin
eantonin previously approved these changes Apr 11, 2023
cli/src/pcluster/utils.py Outdated Show resolved Hide resolved
cli/src/pcluster/utils.py Show resolved Hide resolved
@psacc psacc marked this pull request as ready for review April 17, 2023 06:53
@psacc psacc requested review from a team as code owners April 17, 2023 06:53
psacc added 2 commits April 17, 2023 09:30
Signed-off-by: Paolo Sacconier <[email protected]>
Signed-off-by: Paolo Sacconier <[email protected]>
eantonin
eantonin previously approved these changes Apr 17, 2023
Copy link
Contributor

@eantonin eantonin left a comment

Choose a reason for hiding this comment

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

LGTM, great work

@@ -121,26 +125,134 @@ def _ec2_wait(region, instances, waiter_type):
waiter.wait(InstanceIds=instances)


@pytest.fixture(scope="session", name="resources_dir")
def resources_dir_fixture():
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, non-blocking: This method and some of the below ones are copied from another class, we may put them under the generic conftest.py to DRY

for i in range(10):
if i % 2 == 0:
cache_affinity = i % 2
url = f"{https_url}&cache_affinity={cache_affinity}"
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick,non-blocking: you can just hardcode 0 in here

Or, if you want to have just the half of the https url with cache, this does not work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, introduced after adding s3 urls, fixed

tests/integration-tests/tests/pcluster_api/test_api.py Outdated Show resolved Hide resolved
@psacc psacc merged commit 0cecd4d into aws:develop Apr 17, 2023
@psacc psacc deleted the multiple_custom_actions_validators branch April 17, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x skip-changelog-update Disables the check that enforces changelog updates in PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants