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

fix(chart): job to patch scaledobject stuck in deleting #2222

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Apr 25, 2024

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Fix: #2196

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Type

enhancement, bug_fix


Description

  • Introduced retry strategies in GitHub Actions workflows to improve build reliability.
  • Enhanced Dockerfile to use a more reliable keyserver with a fallback option.
  • Added multiple Helm templates for managing autoscaling with KEDA, including jobs, roles, and role bindings.
  • Modified chart test scripts to conditionally enable autoscaling features based on environment settings.

Changes walkthrough

Relevant files
Enhancement
9 files
chart_test.sh
Enhance autoscaling conditions in chart test script           

tests/charts/make/chart_test.sh

  • Added condition to enable autoscaling only if
    SELENIUM_GRID_AUTOSCALING is true.
  • Adjusted Helm command settings based on autoscaling and existing KEDA
    configurations.
  • +3/-4     
    _nameHelpers.tpl
    Add new Helm template helpers for autoscaling and KEDA     

    charts/selenium-grid/templates/_nameHelpers.tpl

  • Added new template definitions for autoscaling labels and various
    Kubernetes resources related to KEDA.
  • +28/-0   
    build-test.yml
    Implement retry strategy in build-test workflow                   

    .github/workflows/build-test.yml

  • Replaced direct make build command with a retry strategy using
    nick-invision/retry.
  • +5/-1     
    helm-chart-test.yml
    Apply retry strategy in helm-chart-test workflow                 

    .github/workflows/helm-chart-test.yml

  • Applied retry strategy for Docker image building in Helm chart
    testing.
  • +5/-1     
    test-video.yml
    Introduce retry strategy in test-video workflow                   

    .github/workflows/test-video.yml

  • Introduced retry strategy for pre-building Docker images to reduce
    logs in test phase.
  • +9/-5     
    Dockerfile
    Update keyserver URL with fallback in Dockerfile                 

    Base/Dockerfile

  • Modified keyserver URL and added fallback for key retrieval in
    Dockerfile.
  • +1/-1     
    patch-keda-objects-job.yaml
    Add job template for patching KEDA objects                             

    charts/selenium-grid/templates/patch-keda/patch-keda-objects-job.yaml

  • Created a new job template to patch KEDA objects, ensuring clean
    deletion.
  • +35/-0   
    patch-keda-rb.yaml
    Introduce RoleBinding template for KEDA                                   

    charts/selenium-grid/templates/patch-keda/patch-keda-rb.yaml

  • Introduced a new RoleBinding template for KEDA related permissions.
  • +20/-0   
    patch-keda-role.yaml
    Add Role template for KEDA resource management                     

    charts/selenium-grid/templates/patch-keda/patch-keda-role.yaml

  • Added a new Role template to manage permissions for KEDA resources.
  • +37/-0   

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

    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Copy link

    PR Description updated to latest commit (79d11f8)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR includes a variety of changes across multiple files, including shell scripts, Helm templates, and GitHub Actions workflows. The changes involve conditional logic, Kubernetes resource definitions, and retry mechanisms in CI pipelines, which require a careful review to ensure they are correct and do not introduce regressions.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Conditional Logic Error: In the chart_test.sh file, the elif condition seems to be a duplicate of the if condition above it, both checking if "${SELENIUM_GRID_AUTOSCALING}" = "true" and "${TEST_EXISTING_KEDA}" = "true". This might be a copy-paste error and could lead to unexpected behavior.

    Retry Mechanism Overuse: The use of the retry mechanism in GitHub Actions for building Docker images and setting up Kubernetes might mask intermittent issues that should be addressed directly.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Bug
    Revise the condition in the elif block to ensure it can be executed when conditions are met.

    The condition in the elif block is identical to the if block above it, which will cause
    the elif block to never execute. Consider revising the condition to reflect the intended
    logic.

    tests/charts/make/chart_test.sh [105]

    -elif [ "${SELENIUM_GRID_AUTOSCALING}" = "true" ] && [ "${TEST_EXISTING_KEDA}" = "true" ]; then
    +elif [ "${SELENIUM_GRID_AUTOSCALING}" = "true" ] && [ "${TEST_EXISTING_KEDA}" = "false" ]; then
     
    Best practice
    Use a specific version of the GitHub action to ensure stability.

    Using the master branch of a GitHub action can lead to unstable builds if changes are
    pushed to master. It's safer to use a specific version or commit hash.

    .github/workflows/build-test.yml [54]

    -uses: nick-invision/retry@master
    +uses: nick-invision/retry@v2.4.1
     
    Replace deprecated apt-key with a safer key management approach.

    Using apt-key is deprecated. Instead, use signed-by with trusted keys stored in
    /etc/apt/keyrings.

    Base/Dockerfile [58]

    -&& apt-key adv --keyserver hkps://keyserver.ubuntu.com:443 --recv-keys 843C48A565F8F04B || apt-key adv --keyserver keyserver.ubuntu.com --recv-keys 843C48A565F8F04B \
    +&& mkdir -p /etc/apt/keyrings \
    +&& gpg --no-default-keyring --keyring /etc/apt/keyrings/ubuntu-archive-keyring.gpg --keyserver hkps://keyserver.ubuntu.com:443 --recv-keys 843C48A565F8F04B
     
    Use kubectl patch for more reliable and maintainable Kubernetes object modifications.

    The kubectl command in the job uses jq to modify JSON data, which is error-prone and hard
    to maintain. Consider using kubectl patch or a similar Kubernetes-native command if
    possible.

    charts/selenium-grid/templates/patch-keda/patch-keda-objects-job.yaml [33]

    -- "kubectl get ScaledObjects,ScaledJobs -n {{ .Release.Namespace }} -l component.autoscaling=true -o=json | jq '.metadata.finalizers = null' | kubectl apply -f -"
    +- "kubectl patch ScaledObjects,ScaledJobs -n {{ .Release.Namespace }} -l component.autoscaling=true --type=json -p='[{\"op\": \"remove\", \"path\": \"/metadata/finalizers\"}]'"
     
    Use a specific Docker image tag to ensure consistent and predictable builds.

    Using latest as a tag for Docker images can lead to unexpected behavior if the image is
    updated. Specify a fixed version to ensure consistent environments.

    charts/selenium-grid/values.yaml [16]

    -kubectlImage: bitnami/kubectl:latest
    +kubectlImage: bitnami/kubectl:1.21.1
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @VietND96 VietND96 merged commit a83f0d6 into SeleniumHQ:trunk Apr 25, 2024
    2 of 12 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: Helm post-upgrade hook fails when recreating selenium-grid-edge-scaledobject
    1 participant