Skip to content

Conversation

@jdeamicis
Copy link
Contributor

@jdeamicis jdeamicis commented Jan 10, 2023

Description of changes

  • Add integration test for scontrol update nodelist sorting bug.
  • Add the new integration test to the list of recurring integration tests in common.yaml.

Tests

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.

Slurm 22.05 (up to 22.05.7) sorts the nodes in nodelist provided
as nodename field to the `scontrol update` command.
If `scontrol update nodename=nodelist nodeaddr=nodeaddrlist` is given,
this causes a mismatch between the nodenames and the nodeaddrs
because the order of nodeaddrlist is not changed to match the reordering
of nodelist.

The bug above will be fixed in Slurm 22.05.8 and later.

The added test checks that the sorting of the nodelist does not happen.

Signed-off-by: Jacopo De Amicis <jdamicis@amazon.it>
@jdeamicis jdeamicis force-pushed the wip/v3.4/apply_slurm_patch branch from 41a0d83 to c3b4c55 Compare January 10, 2023 13:47
@codecov
Copy link

codecov bot commented Jan 10, 2023

Codecov Report

Merging #4785 (f6b9eaf) into release-3.4 (b826bc8) will not change coverage.
The diff coverage is n/a.

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

@@             Coverage Diff              @@
##           release-3.4    #4785   +/-   ##
============================================
  Coverage        89.04%   89.04%           
============================================
  Files              163      163           
  Lines            14369    14369           
============================================
  Hits             12795    12795           
  Misses            1574     1574           
Impacted Files Coverage Δ
cli/src/pcluster/constants.py 100.00% <ø> (ø)

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

Signed-off-by: Jacopo De Amicis <jdamicis@amazon.it>
Signed-off-by: Jacopo De Amicis <jdamicis@amazon.it>
assert_that(slurm_commands.get_node_attribute(nodes_in_queue1[0], "NodeAddr")).is_equal_to(nodes_in_queue1[0])
assert_that(slurm_commands.get_node_attribute(nodes_in_queue2[0], "NodeAddr")).is_equal_to(nodes_in_queue2[0])

remote_command_executor.run_remote_command("sudo systemctl start supervisord")
Copy link
Contributor

Choose a reason for hiding this comment

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

if this isn't needed I would remove this

Signed-off-by: Jacopo De Amicis <jdamicis@amazon.it>
@jdeamicis jdeamicis merged commit dfbdfa9 into aws:release-3.4 Jan 10, 2023
@jdeamicis jdeamicis deleted the wip/v3.4/apply_slurm_patch branch January 10, 2023 17:29
jdeamicis added a commit to jdeamicis/aws-parallelcluster that referenced this pull request Jan 10, 2023
Add integration test for scontrol update nodelist sorting bug

Slurm 22.05 (up to 22.05.7) sorts the nodes in nodelist provided
as nodename field to the `scontrol update` command.
If `scontrol update nodename=nodelist nodeaddr=nodeaddrlist` is given,
this causes a mismatch between the nodenames and the nodeaddrs
because the order of nodeaddrlist is not changed to match the reordering
of nodelist.

The bug above will be fixed in Slurm 22.05.8 and later.

Signed-off-by: Jacopo De Amicis <jdamicis@amazon.it>
lukeseawalker pushed a commit that referenced this pull request Jan 11, 2023
Add integration test for scontrol update nodelist sorting bug

Slurm 22.05 (up to 22.05.7) sorts the nodes in nodelist provided
as nodename field to the `scontrol update` command.
If `scontrol update nodename=nodelist nodeaddr=nodeaddrlist` is given,
this causes a mismatch between the nodenames and the nodeaddrs
because the order of nodeaddrlist is not changed to match the reordering
of nodelist.

The bug above will be fixed in Slurm 22.05.8 and later.

Signed-off-by: Jacopo De Amicis <jdamicis@amazon.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants