Do not support SIGUSR1 and SIGUSR2 syscall handling in windows #970

cyli · 2016-09-22T02:52:12Z

Since they don't exist in Windows. While I was at it, since I had to create a bunch of windows-specific and non-windows-specific files, I moved the signal handling code to utils so that it can also be used by the signer (that way we don't have windows/non-windows files for both server and signer).

Technically I think the only change that would have been necessary to make the windows client build is the const.go change, and I don't think we care about supporting windows for server/signer, but the server and signer changes are added for completeness (and if we want to run full unit tests in windows).

Unfortunately we don't have any windows CI systems yet. :| Although I did add at least cross-compiling to windows to our CI runs.

Addresses part of #551.
Fixes #791.

Note: possibly we want to rebase this to 0.4.0 so that it can be added to 0.4.1

HuKeping · 2016-09-22T03:03:55Z

Haven't tried notary on Windows, but changes LGTM

riyazdf

Thank you for fixing this! LGTM but would be great to kick the tires on this PR on a windows box before merge (cc @endophage)

riyazdf · 2016-09-22T16:19:15Z

docs/reference/signer-config.md

@@ -210,6 +210,42 @@ The environment variables for the older passwords are optional, but Notary
 Signer will not be able to decrypt older keys if they are not provided, and
 attempts to sign data using those keys will fail.

+## Hot logging level reload
+We don't support completely reloading notary signer configuration files yet at present. What we support for now is:


It might be worth making a note about how this is not supported in Windows

Great idea, done. :)

riyazdf · 2016-09-22T16:23:40Z

utils/configuration.go

+
+// SetupSignalTrap is a utility to trap supported signals hand handle them (currently by increasing logging)
+func SetupSignalTrap(handler func(os.Signal)) chan os.Signal {
+	if len(notary.NotarySupportedSignals) == 0 {


non-blocking-nit: can we test this case with some mocking? I'm fine with assigning this const to empty for the test and deferring to return it back to its original value, to mock what Windows would do

👍 I was actually going to see if I could try to set up CI for windows (not sure if we want to do it in this PR) so that we can get coverage numbers for it, in which case this should be tested. I'll follow up on this later today and if it proves to be a ton of work I'll stub out the const instead. :)

Ok, I think we can probably set this up. In case it goes horribly badly and doesn't work, I've added a mocking test for this in a commit we can revert once we have the CI set up.

endophage · 2016-09-22T18:04:32Z

const_windows.go

@@ -0,0 +1,8 @@
+// +build windows


Fine with leaving it in for explicit-ness but for my own understanding, we don't technically need it when using the *_windows.go filename right?

yep, not needed. I just like having the labels since we also have a nowindows that matches against everything else.

endophage · 2016-09-22T18:07:18Z

utils/configuration_nowindows.go

+			return
+		}
+	case syscall.SIGUSR2:
+		if err := AdjustLogLevel(false); err != nil {


Curious why the error case for SIGUSR1 is being covered but this one isn't. Can't work out why that would be that case.

I think it's this line, when it testsDebug + SIGUSR1 -> Debug: https://github.com/docker/notary/pull/970/files#diff-d39909358e7e39607c560d5d0fad0823R26

Hopefully I've fixed this so we test the same set of log level adjustments for both AdjustLogLevel and the signal handler. We'll see what coverage says.

Signed-off-by: Ying Li <[email protected]>

endophage

LGTM on @cyli's changes. Somebody should probably LGTM again for my one-liner.

endophage · 2016-09-22T21:43:21Z

Can run the client tests and build+run notary client on windows 👍

riyazdf · 2016-09-22T21:44:42Z

LGTM on @endophage's commit, so overall LGTM from me 👍

Merge on green after rebase for hotfix/0.4.1!

…both for using with handles and with AdjustLogLevel Signed-off-by: Ying Li <[email protected]>

Add cross-compiling for supported platforms to our CircleCI builders. Signed-off-by: Ying Li <[email protected]>

…igOrAbsolute Signed-off-by: David Lawrence <[email protected]> (github: endophage)

endophage · 2016-09-22T23:15:44Z

utils/configuration_test.go

+	old := notary.NotarySupportedSignals
+	notary.NotarySupportedSignals = nil
+	defer func() {
+		notary.NotarySupportedSignals = old


I think not deferring this and moving it to after testSetSignalTrap will solve the race checker problem.

Cool, thanks!

cyli · 2016-09-23T01:46:18Z

If I rebase to master, the race condition goes away... strange.

Update: Oh, it's because of the code coverage calculation script changes - since I'm not building against every single package anymore. Just using that files makes the race condition go away. I'm not sure that's a good thing. I wonder if there's any conflict with some other package...

cyli · 2016-09-23T04:12:57Z

Oh, no. It's because covermode is still count on this branch, as opposed to atomic, which does more precise counting when there's parallelism.

…here no signals are supported. Revert this commit when we have actual CI tests running under windows. Signed-off-by: Ying Li <[email protected]>

Signed-off-by: Ying Li <[email protected]>

riyazdf · 2016-09-23T17:19:01Z

newest changes LGTM

GordonTheTurtle added status/0-triage labels Sep 22, 2016

cyli force-pushed the windows-syscall branch 4 times, most recently from 41beaf8 to 393783c Compare September 22, 2016 07:21

cyli changed the title ~~WIP: Do not support SIGUSR1 and SIGUSR2 syscall handling in windows~~ Do not support SIGUSR1 and SIGUSR2 syscall handling in windows Sep 22, 2016

cyli modified the milestone: Notary 0.4.1 Sep 22, 2016

riyazdf approved these changes Sep 22, 2016

View reviewed changes

endophage reviewed Sep 22, 2016

View reviewed changes

Do not support SIGUSR1 and SIGUSR2 syscall handling in windows

eb87d5c

Signed-off-by: Ying Li <[email protected]>

endophage approved these changes Sep 22, 2016

View reviewed changes

cyli and others added 3 commits September 22, 2016 14:45

Update tests so the same set of log level expectations can be tested …

39b42d9

…both for using with handles and with AdjustLogLevel Signed-off-by: Ying Li <[email protected]>

Add windows as a supported cross-compile platform.

e4f5500

Add cross-compiling for supported platforms to our CircleCI builders. Signed-off-by: Ying Li <[email protected]>

fixing path escaping in JSON for TestConfigFileTLSCanBeRelativeToConf…

89acf3b

…igOrAbsolute Signed-off-by: David Lawrence <[email protected]> (github: endophage)

cyli changed the base branch from master to hotfix/0.4.1 September 22, 2016 21:54

cyli force-pushed the windows-syscall branch from 8e4f189 to 2f99270 Compare September 22, 2016 21:54

endophage reviewed Sep 22, 2016

View reviewed changes

cyli force-pushed the windows-syscall branch from 2f99270 to db0930c Compare September 23, 2016 00:21

Add an extra mocking test for what happens if running under windows w…

eb802aa

…here no signals are supported. Revert this commit when we have actual CI tests running under windows. Signed-off-by: Ying Li <[email protected]>

cyli force-pushed the windows-syscall branch from db0930c to 7717956 Compare September 23, 2016 04:15

covermode should be atomic if there are goroutines

b91f182

Signed-off-by: Ying Li <[email protected]>

cyli force-pushed the windows-syscall branch from 7717956 to b91f182 Compare September 23, 2016 04:30

cyli merged commit 85f320e into hotfix/0.4.1 Sep 23, 2016

cyli deleted the windows-syscall branch September 23, 2016 17:36

This was referenced Sep 30, 2016

Bump notary version to 0.4.2 moby/moby#27074

Merged

Increase/decrease logging level on notary-signer #791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not support SIGUSR1 and SIGUSR2 syscall handling in windows #970

Do not support SIGUSR1 and SIGUSR2 syscall handling in windows #970

cyli commented Sep 22, 2016 •

edited

Loading

HuKeping commented Sep 22, 2016

riyazdf left a comment

riyazdf Sep 22, 2016

cyli Sep 22, 2016

riyazdf Sep 22, 2016 •

edited

Loading

cyli Sep 22, 2016

cyli Sep 22, 2016

endophage Sep 22, 2016

cyli Sep 22, 2016

endophage Sep 22, 2016

riyazdf Sep 22, 2016 •

edited

Loading

cyli Sep 22, 2016

endophage left a comment

endophage commented Sep 22, 2016

riyazdf commented Sep 22, 2016 •

edited

Loading

endophage Sep 22, 2016

cyli Sep 22, 2016

cyli commented Sep 23, 2016 •

edited

Loading

cyli commented Sep 23, 2016

riyazdf commented Sep 23, 2016

Do not support SIGUSR1 and SIGUSR2 syscall handling in windows #970

Do not support SIGUSR1 and SIGUSR2 syscall handling in windows #970

Conversation

cyli commented Sep 22, 2016 • edited Loading

HuKeping commented Sep 22, 2016

riyazdf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riyazdf Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riyazdf Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endophage left a comment

Choose a reason for hiding this comment

endophage commented Sep 22, 2016

riyazdf commented Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli commented Sep 23, 2016 • edited Loading

cyli commented Sep 23, 2016

riyazdf commented Sep 23, 2016

cyli commented Sep 22, 2016 •

edited

Loading

riyazdf Sep 22, 2016 •

edited

Loading

riyazdf Sep 22, 2016 •

edited

Loading

riyazdf commented Sep 22, 2016 •

edited

Loading

cyli commented Sep 23, 2016 •

edited

Loading