Add synchronization around endpoint modification on RS1-RS4 by thaJeztah · Pull Request #556 · microsoft/hcsshim

thaJeztah · 2019-04-08T12:05:11Z

Server versions before 2019 (RS5), including Server 2016 (RS1) do not support concurrent add/delete of endpoints. Therefore, we need to use a mutex to serialize the add/delete of endpoints on those versions.

This is based on changes in moby/libnetwork#2356, but doing this check inside hcsshim itself.

Note that

I used an RWMutex instead of a Mutex, but happy to change if that doesn't make sense.
Given that this is now performed at a lower level, other POST/DELETE/GET calls will now also use the mutex (previously in Adding synchronization around peerAdd and peerDelete to prevent network connectivity issue moby/libnetwork#2356, only a limited number of calls were protected), so for example, HNSListEndpointRequest() will take a read-lock, as well as some other calls in libnetwork.

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

thaJeztah · 2019-04-08T12:06:40Z

Created this, as I commented on moby/moby#39018 (comment)

ping @mavenugo @pradipd PTAL

thaJeztah · 2019-04-08T12:07:30Z

internal/hns/hnsendpoint.go

 // HNSEndpointRequest makes a HNS call to modify/query a network endpoint
 func HNSEndpointRequest(method, path, request string) (*HNSEndpoint, error) {
 	endpoint := &HNSEndpoint{}
+	if windowsBuild == osversion.RS1 {


Wondering; does this apply to RS2 as well, or is that completely obsolete?

That is obsolete. Also, RS2 is not a LTSC or SAC branch.
Our expectation is customers are on RS1 (server2016) or RS5 (server 2019) or the latest (semi-annual channel) branch.

Main reason for asking was if we should be defensive and, e.g. change this to

if windowsBuild < osversion.RS3 {

or

if windowsBuild < osversion.RS5 {

@pradipd Unfortunately.... I think < RS5 would be more prudent. There are at least plenty of docker users still using RS3 and RS4 :/ And when they switch to using containerd as a runtime, I think we need to be more defensive. (That said, hopefully soon we can disable the use of RS3 and RS4 entirely).

OK. Sounds good to me.

Updated to use

if windowsBuild < osversion.RS5 {

And updated the commit message, and comment accordingly PTAL

thaJeztah · 2019-04-08T12:09:27Z

internal/hns/hnsendpoint.go

+		endpointMu.Lock()
+		defer endpointMu.Unlock()
+	}
 	return hnsCall("POST", "/endpoints/"+endpoint.Id+"/detach", string(jsonString), &response)


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 😅

Not in RS1.

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

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

jterry75 · 2019-04-09T20:25:16Z

@madhanrm @nagiesek - PTAL

dcantah · 2021-04-08T09:50:27Z

@thaJeztah Do we still want/care about this? Was looking into cleaning up all of the dangling hcsshim prs that have been sitting for ages.

thaJeztah · 2021-04-08T10:32:52Z

@dcantah I think it would still be good if consumers of this shim wouldn't need the internal knowledge about what' supported/needed and what not for different versions of Windows.

But please make sure that the changes I made are correct, as I'm not well familiar with the Windows internals! 🤗 😅

Let me at least rebase this PR to get it back in a mergeable state, and replace osversion.Get().Build with osversion.Build() (which I implemented for uses like this);

hcsshim/osversion/osversion_windows.go

Lines 34 to 36 in 58f7ef4

    
           // Build gets the build-number on Windows 
        
           // The calling application must be manifested to get the correct version information. 
        
           func Build() uint16 {

thaJeztah

👍 rebased and changed osversion.Get().Build to osversion.Build()

thaJeztah · 2021-04-08T10:37:59Z

internal/hns/hnsendpoint.go

 	"strings"
+	"sync"


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

dcantah · 2021-04-08T10:41:35Z

@thaJeztah

@dcantah I think it would still be good if consumers of this shim wouldn't need the internal knowledge about what' supported/needed and what not for different versions of Windows.

Agreed on this, didn't know if there was any other context I was missing surrounding this issue in the last two years 😆

dcantah · 2021-04-08T10:41:57Z

@thaJeztah Thanks for this! I'll try and take a peek later today

thaJeztah · 2021-04-08T11:03:31Z

Thank you! I also opened #996 for a minor cleanup (but left a comment in the PR description for possible adding / replacing some intermediate build numbers)

thaJeztah · 2021-06-10T16:21:59Z

Rebased to get a fresh run of CI

Server versions before 2019 (RS5), including Server 2016 (RS1) do not support concurrent add/delete of endpoints. Therefore, we need to use a mutex to serialize the add/delete of endpoints on those versions. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

dcantah · 2022-08-03T23:38:15Z

internal/hns/hnsendpoint.go

+	if windowsBuild < osversion.RS5 {
+		switch method {
+		case "GET":
+			endpointMu.RLock()
+			defer endpointMu.RUnlock()
+		case "DELETE", "POST":
+			endpointMu.Lock()
+			defer endpointMu.Unlock()
+		}
+	}


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 😆

kiashok · 2025-08-20T19:33:35Z

This is for older versions of OS which are no longer supported.

thaJeztah commented Apr 8, 2019

View reviewed changes

thaJeztah force-pushed the fix_ws_2016_concurrent_add_delete branch from 58d0062 to bde32e6 Compare April 9, 2019 14:56

thaJeztah changed the title ~~Add synchronization around endpoint modification on RS1~~ Add synchronization around endpoint modification on RS1-RS4 Apr 17, 2019

thaJeztah force-pushed the fix_ws_2016_concurrent_add_delete branch from bde32e6 to b6bede0 Compare April 17, 2019 09:23

thaJeztah mentioned this pull request Apr 17, 2019

Use hcsshim osversion package for version checks, instead of docker/docker/pkg/system moby/libnetwork#2362

Closed

thaJeztah mentioned this pull request Jun 25, 2019

Add osversion 18362 #581

Closed

TBBle mentioned this pull request Nov 23, 2020

Is it possible to create an empty scratch layer? #853

Closed

thaJeztah force-pushed the fix_ws_2016_concurrent_add_delete branch from b6bede0 to 30eaa67 Compare April 8, 2021 10:37

thaJeztah requested a review from a team as a code owner April 8, 2021 10:37

thaJeztah commented Apr 8, 2021

View reviewed changes

thaJeztah mentioned this pull request Apr 8, 2021

Use osversion.Build() utility, and add a sync.Once #996

Merged

thaJeztah force-pushed the fix_ws_2016_concurrent_add_delete branch from 30eaa67 to c0e8255 Compare June 10, 2021 16:21

thaJeztah force-pushed the fix_ws_2016_concurrent_add_delete branch from c0e8255 to f1eb7b7 Compare August 3, 2022 21:43

dcantah reviewed Aug 3, 2022

View reviewed changes

kiashok closed this Aug 20, 2025

Conversation

thaJeztah commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Apr 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jterry75 commented Apr 9, 2019

Uh oh!

dcantah commented Apr 8, 2021

Uh oh!

thaJeztah commented Apr 8, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcantah commented Apr 8, 2021

Uh oh!

dcantah commented Apr 8, 2021

Uh oh!

thaJeztah commented Apr 8, 2021

Uh oh!

thaJeztah commented Jun 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiashok commented Aug 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

thaJeztah commented Apr 8, 2019 •

edited

Loading