Skip to content

Respond PortsStatus with sorted ports #13788

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 20, 2022
Merged

Respond PortsStatus with sorted ports #13788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64c0798
[supervisor] respond PortsStatus with order
mustard-mh Oct 14, 2022
137363e
[supervisor] add test cases for ports order
mustard-mh Oct 14, 2022
54f0678
DEBUG: update code image to use order from supervisor
mustard-mh Oct 19, 2022
2569216
[gp-cli] remove sort in gp ports list cmd
mustard-mh Oct 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion WORKSPACE.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ defaultArgs:
publishToNPM: true
publishToJBMarketplace: true
localAppVersion: unknown
codeCommit: 239fa626a44652cebf293a994a3330e90e873daa
codeCommit: f604b771695de9ac3744a1aec9d32112c8873843
codeQuality: stable
noVerifyJBPlugin: false
intellijDownloadUrl: "https://download.jetbrains.com/idea/ideaIU-2022.2.3.tar.gz"
Expand Down
5 changes: 0 additions & 5 deletions components/gitpod-cli/cmd/ports-list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"context"
"fmt"
"os"
"sort"
"time"

supervisor_helper "github.com/gitpod-io/gitpod/gitpod-cli/pkg/supervisor-helper"
Expand Down Expand Up @@ -38,10 +37,6 @@ var listPortsCmd = &cobra.Command{
return
}

sort.Slice(ports, func(i, j int) bool {
return int(ports[i].LocalPort) < int(ports[j].LocalPort)
})

table := tablewriter.NewWriter(os.Stdout)
table.SetHeader([]string{"Port", "Status", "URL", "Name & Description"})
table.SetBorders(tablewriter.Border{Left: true, Top: false, Right: true, Bottom: false})
Expand Down
1 change: 1 addition & 0 deletions components/gitpod-protocol/go/gitpod-service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1756,6 +1756,7 @@ type PortConfig struct {
Visibility string `json:"visibility,omitempty"`
Description string `json:"description,omitempty"`
Name string `json:"name,omitempty"`
Sort uint32 `json:"sort,omitempty"`
Copy link
Member

@akosyakov akosyakov Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it something only for internal usage of supervisor? I don’t think we should have such property here then. These are types for gitpod config.

cc @mustard-mh could you remove it to avoid confusion please

}

// TaskConfig is the TaskConfig message type
Expand Down
6 changes: 5 additions & 1 deletion components/supervisor/pkg/ports/ports-config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ type RangeConfig struct {
*gitpod.PortsItems
Start uint32
End uint32
Sort uint32
}

// Configs provides access to port configurations.
Expand Down Expand Up @@ -79,6 +80,7 @@ func (configs *Configs) Get(port uint32) (*gitpod.PortConfig, ConfigKind, bool)
Visibility: rangeConfig.Visibility,
Description: rangeConfig.Description,
Name: rangeConfig.Name,
Sort: rangeConfig.Sort,
}, RangeConfigKind, true
}
}
Expand Down Expand Up @@ -184,7 +186,7 @@ func parseWorkspaceConfigs(ports []*gitpod.PortConfig) (portConfigs map[uint32]*
}

func parseInstanceConfigs(ports []*gitpod.PortsItems) (portConfigs map[uint32]*gitpod.PortConfig, rangeConfigs []*RangeConfig) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead return an ordered list and avoid specifying the sort order as a property on a map of items?

Copy link
Contributor Author

@mustard-mh mustard-mh Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead return an ordered list and avoid specifying the sort order as a property on a map of items?

I want to do it too (my first implement), but we have ranged ports (i.e. 3000-5000), if someone defined it with large range, the array will be very large too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a concern around the number of bytes needed to store or some other size? I'd expect that the resulting size in bytes would be nearly identical, we're storing n items in both cases.

The extra property of Index probably costs us more as its repeated on each item as opposed to have the sorting implicit which is what a list would do.

Anyhow, just to wanted to flag this up as it felt odd.

Copy link
Contributor Author

@mustard-mh mustard-mh Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

# user can define ranged port like this, and ignore if this num of port is validate or not
ports:
  - port: 100001
  - port: 1-100000

With Sort property defined, we don't need anything.

But if we use a new variable like SortedPorts array, it will contain 100001 items. Or, you need to check if this port number is inside this range or not everytime, it makes code harder to read and wastes resources while getting the ports list

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could store it as

ports: [{range: {from: 1, to: 10000}, ...}, {range: {from: 10001, to: 10001}, ...}

Which would give us the same result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, you need to check if this port number is inside this range or not everytime

It's this case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave it open, and make it a follow-up PR if we are going to change it.

cc @akosyakov @iQQBot

for _, config := range ports {
for index, config := range ports {
if config == nil {
continue
}
Expand All @@ -204,6 +206,7 @@ func parseInstanceConfigs(ports []*gitpod.PortsItems) (portConfigs map[uint32]*g
Visibility: config.Visibility,
Description: config.Description,
Name: config.Name,
Sort: uint32(index),
}
}
continue
Expand All @@ -224,6 +227,7 @@ func parseInstanceConfigs(ports []*gitpod.PortsItems) (portConfigs map[uint32]*g
PortsItems: config,
Start: uint32(start),
End: uint32(end),
Sort: uint32(index),
})
}
return portConfigs, rangeConfigs
Expand Down
23 changes: 22 additions & 1 deletion components/supervisor/pkg/ports/ports.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,8 +740,29 @@ func (pm *Manager) Subscribe() (*Subscription, error) {
func (pm *Manager) getStatus() []*api.PortsStatus {
res := make([]*api.PortsStatus, 0, len(pm.state))
for port := range pm.state {
res = append(res, pm.getPortStatus(port))
portStatus := pm.getPortStatus(port)
// Filter out ports that not served and not inside `.gitpod.yml`
if _, _, ok := pm.configs.Get(port); !ok && !portStatus.Served {
continue
}
res = append(res, portStatus)
}
sort.SliceStable(res, func(i, j int) bool {
// Max number of port 65536
score1 := 100000 + res[i].LocalPort
score2 := 100000 + res[j].LocalPort
if c, _, ok := pm.configs.Get(res[i].LocalPort); ok {
score1 = c.Sort
}
if c, _, ok := pm.configs.Get(res[j].LocalPort); ok {
score2 = c.Sort
}
if score1 != score2 {
return score1 < score2
}
// Ranged ports
return res[i].LocalPort < res[j].LocalPort
})
return res
}

Expand Down
108 changes: 98 additions & 10 deletions components/supervisor/pkg/ports/ports_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,13 @@ import (
"testing"
"time"

"github.com/gitpod-io/gitpod/common-go/log"
gitpod "github.com/gitpod-io/gitpod/gitpod-protocol"
"github.com/gitpod-io/gitpod/supervisor/api"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/sirupsen/logrus"
"golang.org/x/sync/errgroup"

"github.com/gitpod-io/gitpod/common-go/log"
gitpod "github.com/gitpod-io/gitpod/gitpod-protocol"
"github.com/gitpod-io/gitpod/supervisor/api"
)

func TestPortsUpdateState(t *testing.T) {
Expand Down Expand Up @@ -64,8 +63,8 @@ func TestPortsUpdateState(t *testing.T) {
[]*api.PortsStatus{{LocalPort: 8080, Served: true, OnOpen: api.PortsStatus_notify_private}},
[]*api.PortsStatus{{LocalPort: 8080, Served: true, OnOpen: api.PortsStatus_notify_private, Exposed: &api.ExposedPortInfo{OnExposed: api.OnPortExposedAction_notify_private, Visibility: api.PortVisibility_private, Url: "foobar"}}},
[]*api.PortsStatus{{LocalPort: 8080, Served: true, OnOpen: api.PortsStatus_notify_private, Exposed: &api.ExposedPortInfo{OnExposed: api.OnPortExposedAction_notify_private, Visibility: api.PortVisibility_private, Url: "foobar"}}, {LocalPort: 60000, Served: true}},
[]*api.PortsStatus{{LocalPort: 8080, Served: false, OnOpen: api.PortsStatus_notify_private, Exposed: &api.ExposedPortInfo{OnExposed: api.OnPortExposedAction_notify_private, Visibility: api.PortVisibility_private, Url: "foobar"}}, {LocalPort: 60000, Served: true}},
[]*api.PortsStatus{{LocalPort: 8080, Served: false, OnOpen: api.PortsStatus_notify_private, Exposed: &api.ExposedPortInfo{OnExposed: api.OnPortExposedAction_notify_private, Visibility: api.PortVisibility_private, Url: "foobar"}}},
[]*api.PortsStatus{{LocalPort: 60000, Served: true}},
{},
},
},
{
Expand All @@ -86,15 +85,18 @@ func TestPortsUpdateState(t *testing.T) {
{
Desc: "basic port publically exposed",
Changes: []Change{
{Exposed: []ExposedPort{{LocalPort: 8080, Public: false, URL: "foobar"}}},
{Exposed: []ExposedPort{{LocalPort: 8080, Public: true, URL: "foobar"}}},
{Served: []ServedPort{{Port: 8080}}},
{Exposed: []ExposedPort{{LocalPort: 8080, Public: true, URL: "foobar"}}},
{Exposed: []ExposedPort{{LocalPort: 8080, Public: false, URL: "foobar"}}},
},
ExpectedExposure: ExposureExpectation{
{LocalPort: 8080},
},
ExpectedUpdates: UpdateExpectation{
{},
[]*api.PortsStatus{{LocalPort: 8080, OnOpen: api.PortsStatus_notify_private, Exposed: &api.ExposedPortInfo{Visibility: api.PortVisibility_private, Url: "foobar", OnExposed: api.OnPortExposedAction_notify_private}}},
[]*api.PortsStatus{{LocalPort: 8080, OnOpen: api.PortsStatus_notify_private, Exposed: &api.ExposedPortInfo{Visibility: api.PortVisibility_public, Url: "foobar", OnExposed: api.OnPortExposedAction_notify_private}}},
[]*api.PortsStatus{{LocalPort: 8080, Served: true, OnOpen: api.PortsStatus_notify_private}},
[]*api.PortsStatus{{LocalPort: 8080, Served: true, OnOpen: api.PortsStatus_notify_private, Exposed: &api.ExposedPortInfo{Visibility: api.PortVisibility_public, Url: "foobar", OnExposed: api.OnPortExposedAction_notify_private}}},
[]*api.PortsStatus{{LocalPort: 8080, Served: true, OnOpen: api.PortsStatus_notify_private, Exposed: &api.ExposedPortInfo{Visibility: api.PortVisibility_private, Url: "foobar", OnExposed: api.OnPortExposedAction_notify_private}}},
},
},
{
Expand Down Expand Up @@ -746,3 +748,89 @@ func TestPortsConcurrentSubscribe(t *testing.T) {

wg.Wait()
}

func TestManager_getStatus(t *testing.T) {
type fields struct {
orderInYaml []any
state []uint32
}
tests := []struct {
name string
fields fields
want []uint32
}{
{
name: "happy path",
fields: fields{
// The port number (e.g. 1337) or range (e.g. 3000-3999) to expose.
orderInYaml: []any{1002, 1000, "3000-3999", 1001},
state: []uint32{1000, 1001, 1002, 3003, 3001, 3002, 4002, 4000, 5000, 5005},
},
want: []uint32{1002, 1000, 3001, 3002, 3003, 1001, 4000, 4002, 5000, 5005},
},
{
name: "order for ranged ports and inside ranged order by number ASC",
fields: fields{
orderInYaml: []any{1002, "3000-3999", 1009, "4000-4999"},
state: []uint32{5000, 1000, 1009, 4000, 4001, 3000, 3009},
},
want: []uint32{3000, 3009, 1009, 4000, 4001, 1000, 5000},
},
{
name: "served ports order by number ASC",
fields: fields{
orderInYaml: []any{},
state: []uint32{4000, 4003, 4007, 4001, 4006},
},
want: []uint32{4000, 4001, 4003, 4006, 4007},
},

// It will not works because we do not `Run` ports Manger
// As ports Manger will autoExpose those ports (but not ranged port) in yaml
// and they will exists in state
// {
// name: "not ignore ports that not served but exists in yaml",
// fields: fields{
// orderInYaml: []any{1002, 1000, 1001},
// state: []uint32{},
// },
// want: []uint32{1002, 1000, 1001},
// },
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
state := make(map[uint32]*managedPort)
for _, port := range tt.fields.state {
state[port] = &managedPort{
Served: true,
LocalhostPort: port,
TunneledTargetPort: port,
TunneledClients: map[string]uint32{},
}
}
portsItems := []*gitpod.PortsItems{}
for _, port := range tt.fields.orderInYaml {
portsItems = append(portsItems, &gitpod.PortsItems{Port: port})
}
portsConfig, rangeConfig := parseInstanceConfigs(portsItems)
pm := &Manager{
configs: &Configs{
instancePortConfigs: portsConfig,
instanceRangeConfigs: rangeConfig,
},
state: state,
}
got := pm.getStatus()
if len(got) != len(tt.want) {
t.Errorf("Manager.getStatus() length = %v, want %v", len(got), len(tt.want))
}
gotPorts := []uint32{}
for _, g := range got {
gotPorts = append(gotPorts, g.LocalPort)
}
if diff := cmp.Diff(gotPorts, tt.want); diff != "" {
t.Errorf("unexpected exposures (-want +got):\n%s", diff)
}
})
}
}