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

Rename leader election to main #1518

Merged
merged 3 commits into from
Jul 23, 2024
Merged

Conversation

renancloudwalk
Copy link
Contributor

@renancloudwalk renancloudwalk commented Jul 23, 2024

PR Type

Enhancement, Tests


Description

  • Renamed the leader election script to main.sh and enhanced it with additional functionalities.
  • Added command-line options parsing for binary, instances, iterations, and leader restart.
  • Implemented functions for starting instances, checking liveness, and leader election.
  • Added logic for running tests and handling leader restarts.
  • Updated the GitHub Actions workflow to call the renamed script main.sh.

Changes walkthrough 📝

Relevant files
Enhancement
main.sh
Rename and enhance leader election script                               

chaos/experiments/main.sh

  • Renamed the script from leader-election.sh to main.sh.
  • Added command-line options parsing for binary, instances, iterations,
    and leader restart.
  • Implemented functions for starting instances, checking liveness, and
    leader election.
  • Added logic for running tests and handling leader restarts.
  • +347/-1 
    Tests
    e2e-generic.yml
    Update workflow to use renamed script                                       

    .github/workflows/e2e-generic.yml

    • Updated the workflow to call the renamed script main.sh.
    +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @renancloudwalk renancloudwalk enabled auto-merge (squash) July 23, 2024 18:56
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Logging and Monitoring
    The script uses cargo run to start instances but lacks proper logging and monitoring for these operations. Consider adding structured tracing to monitor the instances' lifecycle and operations.

    Tokio Task Spawning
    The script does not use spawn_named or spawn_blocking_named for spawning tasks. Ensure to use these functions for better task management and monitoring.

    Leader Election Timeout
    The leader election process has a fixed timeout of 120 seconds. Consider making this configurable to allow flexibility during different test scenarios.

    Dynamic Field Logging
    The script logs dynamic fields directly in the messages. Use structured tracing fields instead to log these dynamic fields.

    Copy link

    github-actions bot commented Jul 23, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a check to ensure the jq command is available before using it

    Consider adding a check to ensure that the jq command is available on the system
    before using it in the check_liveness and check_leader functions. This will prevent
    the script from failing unexpectedly if jq is not installed.

    chaos/experiments/main.sh [112-116]

     check_liveness() {
         local port=$1
    +    if ! command -v jq &> /dev/null; then
    +        echo "jq command not found. Please install jq to proceed."
    +        exit 1
    +    fi
         curl -s http://0.0.0.0:$port \
             --header "content-type: application/json" \
             --data '{"jsonrpc":"2.0","method":"stratus_health","params":[],"id":1}' | jq '.result'
     }
     
    Suggestion importance[1-10]: 9

    Why: This suggestion is crucial as it prevents the script from failing unexpectedly if jq is not installed, ensuring robustness.

    9
    Add a check to ensure the killport command is available before using it

    Add a check to ensure that the killport command is available on the system before
    using it in the cleanup and run_test functions. This will prevent the script from
    failing unexpectedly if killport is not installed.

    chaos/experiments/main.sh [54-58]

     cleanup() {
         exit_code=$?
         echo "Cleaning up..."
    +    if ! command -v killport &> /dev/null; then
    +        echo "killport command not found. Please install killport to proceed."
    +        exit 1
    +    fi
         for port in "${ports[@]}"; do
             killport --quiet $port || true
         done
     
    Suggestion importance[1-10]: 9

    Why: This check is important to prevent the script from failing if killport is not installed, ensuring the cleanup process runs smoothly.

    9
    Add a check to ensure the cargo command is available before using it

    Add a check to ensure that the cargo command is available on the system before using
    it in the start_instance function. This will prevent the script from failing
    unexpectedly if cargo is not installed.

    chaos/experiments/main.sh [89-98]

     start_instance() {
         local address=$1
         local grpc_address=$2
         local rocks_path_prefix=$3
         local log_file=$4
         local candidate_peers=$5
         local tokio_console_address=$6
         local metrics_exporter_address=$7
    +    if ! command -v cargo &> /dev/null; then
    +        echo "cargo command not found. Please install cargo to proceed."
    +        exit 1
    +    fi
     
    Suggestion importance[1-10]: 9

    Why: Ensuring cargo is available before using it is critical to avoid unexpected script failures, thus improving script reliability.

    9
    Best practice
    Initialize the leader_port variable to an empty string at the beginning of the function

    To avoid potential issues with uninitialized variables, consider initializing the
    leader_port variable to an empty string at the beginning of the run_test function.

    chaos/experiments/main.sh [160-164]

     run_test() {
         local instances=()
         local all_addresses=()
    +    local leader_port=""
     
    Suggestion importance[1-10]: 7

    Why: Initializing leader_port to an empty string is a good practice to avoid potential issues with uninitialized variables, enhancing code reliability.

    7

    @renancloudwalk renancloudwalk merged commit b8ae55d into main Jul 23, 2024
    30 checks passed
    @renancloudwalk renancloudwalk deleted the rename-leader-election-to-main branch July 23, 2024 21:08
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant