- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.2k
Modify scale down set processor to add reasons to unremovable nodes #7307
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
Conversation
| Hi @abdelrahman882. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with  Once the patch is verified, the new status will be reflected by the  I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. | 
35f6b05    to
    92f97a5      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the change! Especially for adding unit tests where they were missing.
I've got some suggestions but the comments are all relatively minor.
        
          
                cluster-autoscaler/processors/nodes/scale_down_set_processor.go
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                cluster-autoscaler/processors/nodes/scale_down_set_processor.go
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                cluster-autoscaler/processors/nodes/scale_down_set_processor_test.go
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                cluster-autoscaler/processors/nodes/scale_down_set_processor_test.go
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                cluster-autoscaler/processors/nodes/scale_down_set_processor_test.go
              
                Outdated
          
            Show resolved
            Hide resolved
        
      9053d15    to
    35e5e2e      
    Compare
  
            
          
                cluster-autoscaler/processors/nodes/scale_down_set_processor.go
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                cluster-autoscaler/processors/nodes/scale_down_set_processor.go
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                cluster-autoscaler/processors/nodes/scale_down_set_processor_test.go
              
                Outdated
          
            Show resolved
            Hide resolved
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
35e5e2e    to
    203732f      
    Compare
  
    203732f    to
    7386a22      
    Compare
  
    | I think you can remove the isValidCandidatesFiltering function now. Otherwise | 
7386a22    to
    98396b4      
    Compare
  
    | /lgtm | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just minor comments.
/approve
| [APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abdelrahman882, bskiba, x13n The full list of commands accepted by this bot can be found here. The pull request process is described here 
Needs approval from an approver in each of these files:
 
 Approvers can indicate their approval by writing  | 
98396b4    to
    2754f1f      
    Compare
  
    2754f1f    to
    f945fc4      
    Compare
  
    | /lgtm | 
| @abdelrahman882 this is such an amazing change, and I was about to put this out as a feature request 🙌🏻 Unfortunately I don't see the annotation part (where it is supposed to mention on the node about blockers) present in any of the releases done after this merge, is there any blocker that I can help with? @x13n @omerap12 Edit: Seems like I misunderstood the code. It seems to be only giving this info internally and not storing it in CM or annotating the node | 
What type of PR is this?
/kind bug
/kind cleanup
What this PR does / why we need it:
Modify scale down set processor to add reasons to unremovable nodes so that planner can have these nodes excluded as unremovable nodes with reasons
Special notes for your reviewer:
Does this PR introduce a user-facing change?
NONE