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

Correct kubectl cli params in keda patch job #2501

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

jbolsens-legion
Copy link
Contributor

@jbolsens-legion jbolsens-legion commented Dec 10, 2024

User description

Description

--wait is a flag by itself, need to add --wait=false else false gets interpreted as an object name and leads to the error:

error: name cannot be provided when a selector is specified

Motivation and Context

Without this, the selenium-patch-scaledobjects-finalizers job will consistently fail

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.

PR Type

Bug fix


Description

  • Fixed the kubectl command parameters in the KEDA patch job by adding --wait=false to ensure false is not interpreted as an object name.
  • This change prevents the selenium-patch-scaledobjects-finalizers job from failing due to incorrect parameter interpretation.

Changes walkthrough 📝

Relevant files
Bug fix
patch-keda-objects-job.yaml
Fix `kubectl` command parameters in KEDA patch job             

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

  • Corrected the kubectl command by adding --wait=false.
  • Prevented false from being interpreted as an object name.
  • +2/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    --wait is a flag by itself, need to add `--wait=false` else `false` gets interpreted as an object name and leads to the error:
    
    > error: name cannot be provided when a selector is specified
    @CLAassistant
    Copy link

    CLAassistant commented Dec 10, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Command Validation
    Verify that the kubectl delete commands with --wait=false flag work as expected and don't cause any unintended side effects during cleanup

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add timeout parameters to prevent potential indefinite command execution

    Add a timeout to the kubectl delete commands to prevent potential indefinite hangs
    in case of resource deletion issues.

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

    -kubectl delete ScaledObjects,ScaledJobs,TriggerAuthentication -n {{ .Release.Namespace }} -l component.autoscaling={{ .Release.Name }} --wait=false || true ;
    -kubectl delete hpa -n {{ .Release.Namespace }} -l component.autoscaling={{ .Release.Name }} --wait=false || true ;
    +kubectl delete ScaledObjects,ScaledJobs,TriggerAuthentication -n {{ .Release.Namespace }} -l component.autoscaling={{ .Release.Name }} --wait=false --timeout=60s || true ;
    +kubectl delete hpa -n {{ .Release.Namespace }} -l component.autoscaling={{ .Release.Name }} --wait=false --timeout=60s || true ;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a timeout parameter is a critical operational safeguard that prevents potential job hangs and resource deadlocks during deletion operations, especially important in Kubernetes cleanup jobs.

    8

    💡 Need additional feedback ? start a PR chat

    @VietND96
    Copy link
    Member

    Good catch! Thank you for the fix.

    @VietND96 VietND96 merged commit 36d98e9 into SeleniumHQ:trunk Dec 11, 2024
    34 of 36 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.

    3 participants