- 
                Notifications
    
You must be signed in to change notification settings  - Fork 508
 
Add support for publish port ranges #801
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
          
     Open
      
      
            caztanj
  wants to merge
  1
  commit into
  apple:main
  
    
      
        
          
  
    
      Choose a base branch
      
     
    
      
        
      
      
        
          
          
        
        
          
            
              
              
              
  
           
        
        
          
            
              
              
           
        
       
     
  
        
          
            
          
            
          
        
       
    
      
from
caztanj:publish_range
  
      
      
   
  
    
  
  
  
 
  
      
    base: main
Could not load branches
            
              
  
    Branch not found: {{ refName }}
  
            
                
      Loading
              
            Could not load tags
            
            
              Nothing to show
            
              
  
            
                
      Loading
              
            Are you sure you want to change the base?
            Some commits from the old base branch may be removed from the timeline,
            and old review comments may become outdated.
          
          
  
     Open
                    Changes from all commits
      Commits
    
    
  File filter
Filter by extension
Conversations
          Failed to load comments.   
        
        
          
      Loading
        
  Jump to
        
          Jump to file
        
      
      
          Failed to load files.   
        
        
          
      Loading
        
  Diff view
Diff view
There are no files selected for viewing
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
  Add this suggestion to a batch that can be applied as a single commit.
  This suggestion is invalid because no changes were made to the code.
  Suggestions cannot be applied while the pull request is closed.
  Suggestions cannot be applied while viewing a subset of changes.
  Only one suggestion per line can be applied in a batch.
  Add this suggestion to a batch that can be applied as a single commit.
  Applying suggestions on deleted lines is not supported.
  You must change the existing code in this line in order to create a valid suggestion.
  Outdated suggestions cannot be applied.
  This suggestion has been applied or marked resolved.
  Suggestions cannot be applied from pending reviews.
  Suggestions cannot be applied on multi-line comments.
  Suggestions cannot be applied while the pull request is queued to merge.
  Suggestion cannot be applied right now. Please check back later.
  
    
  
    
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.
What happens if I specify
-p 127.0.0.1:1024-65535:1024-65535?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 your response!
I have a bunch of other things running on my machine but if I specify
-p 7001-8020:7001-8020 -p 8022-49325:8022-49325 -p 58000-63000:58000-63000 -p 63765-65535:63765-65535for a total of 49092 ports it works just fine and I cannot see any degraded performance.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.
Cool, thanks for doing the experiment.
Could you try the exact same command with Activity Monitor open and see what you see for memory utilization before and after?
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.
Since everything's getting multiplexed down onto NIO and an event loop group, we might be able to do this. I'll do a little asking around and see if folks more expert than me in this area can see any gotchas.
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.
Memory usage when not publishing any ports:
Virtual Machine Service for container-runtime-linux: 179.4MBcontainer-runtime-linux: 20.5MBMemory usage when publishing the same ports as in my answer above:
Virtual Machine Service for container-runtime-linux: 179.6MBcontainer-runtime-linux: 170.4MBSo the memory usage does increase significantly but I think 170.4MB is still acceptable. Especially considering that most people won't publish that many ports.
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.
Yeah, I'm not surprised by that. It works out to a little under 3.5K worth of memory per port forward.
The way NIO works I don't think you'd see much performance degradation other than what might arise from cache misses if you're sending data concurrently through a lot of different ports at once.
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.
Appreciate you following up with these tests. One other one to try: could you do the same memory experiment for the UDP case?
UDP is a bit different as we need to carry a bit of "connection state" in a LRU cache. I don't think it's a dealbreaker but it'd be good to characterize what goes on there.
I'm also working with some experts to review not your PR, but our NIO port proxy implementation, to make sure this won't break under load and see if we can reduce memory utilization.
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.
The UDP case uses more memory:
Virtual Machine Service for container-runtime-linux: 179.5MBcontainer-runtime-linux: 222.4MBUh oh!
There was an error while loading. Please reload this page.
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.
Thanks, so another 1KB per proxy for the LRU cache entries and UDP proxy context.
I hope to know enough tomorrow to know whether there's work we need to do on the proxy implementation to make sure we can scale reliably.