Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion()

[thaJeztah]

Use hcsshim as the source of truth. Also bumps hcsshim;

• microsoft/hcsshim@672e52e...2226e08
• microsoft/go-winio@c599b53...84b4ab4

[thaJeztah]

This version was somewhere between "RS2" and "RS3", so I picked < RS3

[thaJeztah]

Removing this as TP5 is really obsolete

[vikramhh]

Any idea why the original code scoped the use of defaultNetworkSpace to < 14360 only?

[thaJeztah]

Things were still in development on the Windows side, so I think 14360 was a build where new features became available; that's been a long time back, so I'd have to check git history if there's any info about that

[thaJeztah]

15139 is somewhere between RS2 and RS3; from the context, it looks like the loop must be skipped on RS3 and up; I change it to an early return to make it slightly more readable

[lowenna]

Hmmm, no I think you've changed the logic incorrectly here.

[thaJeztah]

oh; why didn't I see this comment; I'll have to check

[thaJeztah]

Hm, I think my logic is correct, but @lowenna @kolyshkin please double check;

(Simplified code, and replaced 15139 with RS3 for brevity)

Old situation

if err != nil {
	if (ver < RS3) && ((err ==InvalidState) || (err == AccessIsDenied)) {
		// retry
	}
	return err
}

ver	error	ver < RS3 ?	err == InvalidState or AccessIsDenied?	action
RS1	nil	-	-	break
RS1	any	✅	❌	return err
RS1	InvalidState	✅	✅	retry
RS1	AccessIsDenied	✅	✅	retry
RS3	nil	-	-	break
RS3	any	❌	- (not evaluated)	return err
RS3	InvalidState	❌	- (not evaluated)	return err
RS3	AccessIsDenied	❌	- (not evaluated)	return err

New situation:

if err != nil {
	if ver >= RS3 {
		// no retry, return error
	}
	if (err == InvalidState) || (err == AccessIsDenied) {
		// retry
	}
	return err
}

ver	error	ver >= RS3 ?	err == InvalidState or AccessIsDenied?	action
RS1	nil	-	-	break
RS1	any	❌	❌	return err
RS1	InvalidState	❌	✅	retry
RS1	AccessIsDenied	❌	✅	retry
RS3	nil	-	-	break
RS3	any	✅	- (not evaluated)	return err
RS3	InvalidState	✅	- (not evaluated)	return err
RS3	AccessIsDenied	✅	- (not evaluated)	return err

[kolyshkin]

The new logic looks correct to me.

By the way, I found an obsoleted TODO comment just above this. I think the part of it needs to be removed, this one:

For RS3, we can remove the retries. Also

[thaJeztah]

renaming these as they overlap with the build package

[thaJeztah]

This is somewhere between RS1 and RS2, so I took RS3

[thaJeztah]

@jhowardmsft PTAL

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@248c136). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #39100   +/-   ##
=========================================
  Coverage          ?   37.31%           
=========================================
  Files             ?      609           
  Lines             ?    45264           
  Branches          ?        0           
=========================================
  Hits              ?    16892           
  Misses            ?    26083           
  Partials          ?     2289

[andrewhsu]

@ddebroy @vikramhh this will help get RS1 tests more stable

[thaJeztah]

rebased to include #40117

[kolyshkin]

LGTM

[tiborvass]

This could possibly be a type alias

[thaJeztah]

ah, hm. good point, forgot about that as an option

I'll do a quick touch-up follow-up

[thaJeztah]

#40135

[tiborvass]

One small nit that shouldn't block this PR

[thaJeztah]

arf. think my IDE had goimports disabled; let me check my config