Skip to content

Add synchronization around endpoint modification on RS1-RS4 #556

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

Closed
thaJeztah wants to merge 1 commit into from
Closed

Add synchronization around endpoint modification on RS1-RS4 #556

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 53 additions & 1 deletion internal/hns/hnsendpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import (
"encoding/json"
"net"
"strings"
"sync"
Comment on lines 8 to +9
Copy link
Copy Markdown
Contributor Author

@thaJeztah thaJeztah Apr 8, 2021

Choose a reason for hiding this comment

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

"conflict" before rebasing was here (adjacent line changed)


"github.com/Microsoft/hcsshim/osversion"
"github.com/sirupsen/logrus"
)

Expand Down Expand Up @@ -36,7 +38,7 @@ type HNSEndpoint struct {
SharedContainers []string `json:",omitempty"`
}

//SystemType represents the type of the system on which actions are done
// SystemType represents the type of the system on which actions are done
type SystemType string

// SystemType const
Expand All @@ -46,6 +48,14 @@ const (
HostType SystemType = "Host"
)

var (
// Server versions before 2019 (RS5), including Server 2016 (RS1) do not
// support concurrent add/delete of endpoints. Therefore, we need to use
// this mutex and serialize the add/delete of endpoints on those versions.
endpointMu sync.RWMutex
windowsBuild = osversion.Build()
)

// EndpointAttachDetachRequest is the structure used to send request to the container to modify the system
// Supported resource types are Network and Request Types are Add/Remove
type EndpointAttachDetachRequest struct {
Expand Down Expand Up @@ -76,6 +86,16 @@ type EndpointStats struct {
// HNSEndpointRequest makes a HNS call to modify/query a network endpoint
func HNSEndpointRequest(method, path, request string) (*HNSEndpoint, error) {
endpoint := &HNSEndpoint{}
if windowsBuild < osversion.RS5 {
switch method {
case "GET":
endpointMu.RLock()
defer endpointMu.RUnlock()
case "DELETE", "POST":
endpointMu.Lock()
defer endpointMu.Unlock()
}
}
Comment on lines +89 to +98
Copy link
Copy Markdown
Contributor

@dcantah dcantah Aug 3, 2022

Choose a reason for hiding this comment

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

The only thing that comes to mind for this PR is a conundrum we ran into by calling osversion in another exposed API, namely that retrofitting these APIs with osversion means the caller now has to manifest to get the "new" behavior. The new behavior here if they don't manifest boils down to slower execution as we need to lock/unlock and they lose concurrent creates even if they're on a host that supports it, as osversion will return windows 8's build number if not manifested which will satisfy if windowsBuild < osversion.RS5.

For moby this doesn't matter as it's manifested and I'd say an overwhelming majority of uses of hcsshim is in containerd and moby, but something to think of. @jterry75 and I had played around with the thought of possibly having a fallback for osversion, where we'd parse cmd /c ver's output if we find the program isn't manifested. That is a bit hacky though 😆

err := hnsCall(method, "/endpoints/"+path, request, &endpoint)
if err != nil {
return nil, err
Expand All @@ -87,6 +107,10 @@ func HNSEndpointRequest(method, path, request string) (*HNSEndpoint, error) {
// HNSListEndpointRequest makes a HNS call to query the list of available endpoints
func HNSListEndpointRequest() ([]HNSEndpoint, error) {
var endpoint []HNSEndpoint
if windowsBuild < osversion.RS5 {
endpointMu.RLock()
defer endpointMu.RUnlock()
}
err := hnsCall("GET", "/endpoints/", "", &endpoint)
if err != nil {
return nil, err
Expand Down Expand Up @@ -181,6 +205,10 @@ func (endpoint *HNSEndpoint) Update() (*HNSEndpoint, error) {
if err != nil {
return nil, err
}
if windowsBuild < osversion.RS5 {
endpointMu.Lock()
defer endpointMu.Unlock()
}
err = hnsCall("POST", "/endpoints/"+endpoint.Id, string(jsonString), &endpoint)

return endpoint, err
Expand Down Expand Up @@ -244,6 +272,10 @@ func (endpoint *HNSEndpoint) ContainerAttach(containerID string, compartmentID u
if err != nil {
return err
}
if windowsBuild < osversion.RS5 {
endpointMu.Lock()
defer endpointMu.Unlock()
}
return hnsCall("POST", "/endpoints/"+endpoint.Id+"/attach", string(jsonString), &response)
}

Expand All @@ -263,6 +295,10 @@ func (endpoint *HNSEndpoint) ContainerDetach(containerID string) error {
if err != nil {
return err
}
if windowsBuild < osversion.RS5 {
endpointMu.Lock()
defer endpointMu.Unlock()
}
return hnsCall("POST", "/endpoints/"+endpoint.Id+"/detach", string(jsonString), &response)
}

Expand All @@ -281,6 +317,10 @@ func (endpoint *HNSEndpoint) HostAttach(compartmentID uint16) error {
if err != nil {
return err
}
if windowsBuild < osversion.RS5 {
endpointMu.Lock()
defer endpointMu.Unlock()
}
return hnsCall("POST", "/endpoints/"+endpoint.Id+"/attach", string(jsonString), &response)
}

Expand All @@ -298,6 +338,10 @@ func (endpoint *HNSEndpoint) HostDetach() error {
if err != nil {
return err
}
if windowsBuild < osversion.RS5 {
endpointMu.Lock()
defer endpointMu.Unlock()
}
return hnsCall("POST", "/endpoints/"+endpoint.Id+"/detach", string(jsonString), &response)
Copy link
Copy Markdown
Contributor Author

@thaJeztah thaJeztah Apr 8, 2019

Choose a reason for hiding this comment

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

Is concurrent attach / detach allowed? If so, these are not needed (thought I'd open this PR so that this approach can be discussed, and to make hcsshim the "source of truth" as to what is supported for each Windows version and what not, instead of putting that burden on consumers of this library 😅

Copy link
Copy Markdown
Contributor

@pradipd pradipd Apr 15, 2019

Choose a reason for hiding this comment

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

Not in RS1.

Copy link
Copy Markdown
Contributor Author

@thaJeztah thaJeztah Apr 15, 2019

Choose a reason for hiding this comment

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

so this looks ok (as in; yes: we should protect these endpoints as well), correct?

Would other endpoints need patching as well? (if they're not supported on RS1, they should probably produce an error?);

On a side-note; wondering if os-version checks should be added to (e.g.) HotAttachEndpoint(), which (I think) is not supported on older versions

Copy link
Copy Markdown
Contributor

@pradipd pradipd Apr 15, 2019

Choose a reason for hiding this comment

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

@madhanrm Do we need to add version check around HotAttachEndpoint()?

}

Expand All @@ -316,6 +360,10 @@ func (endpoint *HNSEndpoint) VirtualMachineNICAttach(virtualMachineNICName strin
if err != nil {
return err
}
if windowsBuild < osversion.RS5 {
endpointMu.Lock()
defer endpointMu.Unlock()
}
return hnsCall("POST", "/endpoints/"+endpoint.Id+"/attach", string(jsonString), &response)
}

Expand All @@ -334,5 +382,9 @@ func (endpoint *HNSEndpoint) VirtualMachineNICDetach() error {
if err != nil {
return err
}
if windowsBuild < osversion.RS5 {
endpointMu.Lock()
defer endpointMu.Unlock()
}
return hnsCall("POST", "/endpoints/"+endpoint.Id+"/detach", string(jsonString), &response)
}