Skip to content

cmd/containerd: replace deprecated windows.IsAnInteractiveSession()#7497

Merged
estesp merged 1 commit intocontainerd:mainfrom
thaJeztah:replace_IsAnInteractiveSession
Oct 11, 2022
Merged

cmd/containerd: replace deprecated windows.IsAnInteractiveSession()#7497
estesp merged 1 commit intocontainerd:mainfrom
thaJeztah:replace_IsAnInteractiveSession

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Oct 8, 2022

The //nolint comment was added in edc671d (#4626) to suppress the deprecation warning.

The IsAnInteractiveSession was deprecated, and IsWindowsService is marked as the recommended replacement.

For details, see golang/sys@280f808

CL 244958 includes isWindowsService function that determines if a
process is running as a service. The code of the function is based on
public .Net implementation.

IsAnInteractiveSession function implements similar functionality, but
is based on an old Stackoverflow post., which is not as authoritative
as code written by Microsoft for their official product.

This change copies CL 244958 isWindowsService function into svc package
and makes it public. The intention is that future users will prefer
IsWindowsService to IsAnInteractiveSession.

@thaJeztah
cmd/containerd: replace deprecated windows.IsAnInteractiveSession()
8fc68db 
The `IsAnInteractiveSession` was deprecated, and `IsWindowsService` is marked
as the recommended replacement.

For details, see golang/sys@280f808

> CL 244958 includes isWindowsService function that determines if a
> process is running as a service. The code of the function is based on
> public .Net implementation.
>
> IsAnInteractiveSession function implements similar functionality, but
> is based on an old Stackoverflow post., which is not as authoritative
> as code written by Microsoft for their official product.
>
> This change copies CL 244958 isWindowsService function into svc package
> and makes it public. The intention is that future users will prefer
> IsWindowsService to IsAnInteractiveSession.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Oct 8, 2022

@kevpar @dcantah perhaps you're able to review this one; before we merge this, it would be good to have some eyes from people at Microsoft on the changes made in golang/sys@280f808 / https://go-review.googlesource.com/c/sys/+/259397

From the commit, it looks like there's the change is a bit "fuzzy-wuzzy" ("we assume StackOverflow is less authoritative, and even though the .NET code looked bad, we assume that one is more correct");

// The below technique looks a bit hairy, but it's actually
// exactly what the .NET framework does for the similarly named function:
// https://github.com/dotnet/extensions/blob/f4066026ca06984b07e90e61a6390ac38152ba93/src/Hosting/WindowsServices/src/WindowsServiceHelpers.cs#L26-L31

Perhaps one of you know the right person inside Microsoft to verify if the new approach is indeed correct (or if changes are needed in golang.org/x/sys.

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Oct 8, 2022 •
edited
Loading

In case useful; the original implementation in .NET was added in dotnet/extensions@43435a8 (part of dotnet/extensions#1083)

Which, in itself is also a bit fuzzy-wuzzy (emphasis mine);

This takes the sample we had for windows service integration and makes it an official package.
I added some opt in/out config switches and a little bit of auto detection. It turns out there's not a solid mechanic for identifying you're running in a service.

Sample they refer to may be the one from https://github.com/dotnet/AspNetCore.Docs/tree/0ea501cde5c82060aef61bceb30ea831f832a176/aspnetcore/fundamentals/host/generic-host/samples/2.x/GenericHostSample

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Oct 9, 2022

Could someone give CI a kick? Looks unrelated 😅

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Oct 9, 2022

Could someone give CI a kick? Looks unrelated 😅

done

@fuweid fuweid added the platform/windows Windows label Oct 10, 2022
kzys
kzys approved these changes Oct 10, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

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

Looks good to me. But wanna have experts :)

cc: @kevpar @jterry75 @dcantah

@jterry75
Copy link
Copy Markdown
Contributor

jterry75 commented Oct 11, 2022

Oh wow. LGTM based on the .NET code and the inclusion of it via golang sys

estesp
estesp approved these changes Oct 11, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 32aa33a into containerd:main Oct 11, 2022
@thaJeztah thaJeztah deleted the replace_IsAnInteractiveSession branch October 11, 2022 20:31
@thaJeztah thaJeztah mentioned this pull request Oct 11, 2022
Merged
@dcantah
Copy link
Copy Markdown
Member

dcantah commented Oct 11, 2022

Couldn't get to this in time, but LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kzys kzys kzys approved these changes

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

platform/windows Windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@thaJeztah @fuweid @jterry75 @dcantah @kzys @estesp