From 9e6348c3b56ca159069edc212f6ab57f7ac64acf Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Fri, 7 Jan 2022 20:02:39 -0500 Subject: [PATCH 01/14] fix: resolve issue number 463 Resolved original issue by introducing a boolean chanel by which exechook runner can communicate with main thread. Then introduced and used webhook executed-at-least-once chanel and added documentation explaining sections of of code only executed when git-sync pulls for first time. --- cmd/git-sync/main.go | 25 +++++++++++++++++++++++++ pkg/hook/hook.go | 19 +++++++++++++++++-- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index 2327081..7a67427 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -174,6 +174,9 @@ var ( }, []string{"status"}) ) +// Channels for ensuring hooks execute at least once before terminating due to GIT_SYNC_ONCE +var execHookChannel, webHookChannel chan bool // initialising as null to promptly catch logical errors and to avoid unneeded object creation + const ( metricKeySuccess = "success" metricKeyError = "error" @@ -359,6 +362,8 @@ func main() { if *flExechookBackoff < time.Second { handleError(log, true, "ERROR: --exechook-backoff must be at least 1s") } + + execHookChannel = make(chan bool) } if *flWebhookURL != "" { @@ -371,6 +376,8 @@ func main() { if *flWebhookBackoff < time.Second { handleError(log, true, "ERROR: --webhook-backoff must be at least 1s") } + + webHookChannel = make(chan bool) } if *flPassword != "" && *flPasswordFile != "" { @@ -556,6 +563,7 @@ func main() { *flWebhookBackoff, hook.NewHookData(), log, + webHookChannel, ) go webhookRunner.Run(context.Background()) } @@ -576,6 +584,7 @@ func main() { *flExechookBackoff, hook.NewHookData(), log, + execHookChannel, ) go exechookRunner.Run(context.Background()) } @@ -621,6 +630,22 @@ func main() { } if initialSync { + // Wait for hooks to complete at least once before checking whether to stop + // Assumes that if hook channels are not nil, they will have at least one value before getting closed + if execHookChannel != nil { + execHookChannelFinishedSuccessfully:= <-execHookChannel + if !execHookChannelFinishedSuccessfully { + log.Error(nil, "exec hook completed with error") + } + } + if webHookChannel != nil { + webHookChannelFinishedSuccessfully:= <-webHookChannel + if !webHookChannelFinishedSuccessfully { + log.Error(nil, "web hook completed with error") + } + } + + // Determine if git-sync should terminate if *flOneTime { log.DeleteErrorFile() os.Exit(0) diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go index d5b9857..ce027f2 100644 --- a/pkg/hook/hook.go +++ b/pkg/hook/hook.go @@ -82,8 +82,8 @@ func (d *hookData) send(newHash string) { } // NewHookRunner returns a new HookRunner -func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log *logging.Logger) *HookRunner { - return &HookRunner{hook: hook, backoff: backoff, data: data, logger: log} +func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log *logging.Logger, hasSucceededOnce chan bool) *HookRunner { + return &HookRunner{hook: hook, backoff: backoff, data: data, logger: log, hasCompletedOnce: hasSucceededOnce} } // HookRunner struct @@ -96,6 +96,8 @@ type HookRunner struct { data *hookData // Logger logger *logging.Logger + // Has succeeded once Chanel, sends true if first run executed successfully and false if it failed + hasCompletedOnce chan bool } // Send sends hash to hookdata @@ -123,16 +125,29 @@ func (r *HookRunner) Run(ctx context.Context) { if err := r.hook.Do(ctx, hash); err != nil { r.logger.Error(err, "hook failed") updateHookRunCountMetric(r.hook.Name(), "error") + // don't want to sleep unnecessarily if we are going to terminate anyways + r.sendCompletedOnceMessage(false) time.Sleep(r.backoff) } else { updateHookRunCountMetric(r.hook.Name(), "success") lastHash = hash + r.sendCompletedOnceMessage(true) break } } } } +// sendCompletedOnceMessage forwards the success status (as a boolean) of the first execution of HookRunner, the first time +// to the r.hasCompletedOnce channel +func (r *HookRunner) sendCompletedOnceMessage(completedSuccessfully bool) { + if r.hasCompletedOnce != nil { + r.hasCompletedOnce <- completedSuccessfully + close(r.hasCompletedOnce) + r.hasCompletedOnce = nil + } +} + func updateHookRunCountMetric(name, status string) { hookRunCount.WithLabelValues(name, status).Inc() } From 3b5ed549ebf643af8c3acae0240bf0a97cb99d01 Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Sat, 8 Jan 2022 14:36:22 -0500 Subject: [PATCH 02/14] fix: addressed several concers raised by thockin - Renamed [x]HookChannel to [x]hookChannel for consistency - Made hookChannels buffered chanels of size 1 - Added clarifying documentation and renamed functions expalining how I was - ensuring that hooks execute at least once, assuming that main thread does not crash - make sure chanels are not written to more than once --- cmd/git-sync/main.go | 30 +++++++++++++++--------------- pkg/hook/hook.go | 14 ++++++++------ 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index 7a67427..396cb07 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -174,8 +174,10 @@ var ( }, []string{"status"}) ) -// Channels for ensuring hooks execute at least once before terminating due to GIT_SYNC_ONCE -var execHookChannel, webHookChannel chan bool // initialising as null to promptly catch logical errors and to avoid unneeded object creation +// Channels for ensuring hooks execute at least once before terminating. +// Should be nil if and only if corresponding hook is not defined and if initialised, will only every get written +// to once. +var exechookChannel, webhookChannel chan bool const ( metricKeySuccess = "success" @@ -362,8 +364,7 @@ func main() { if *flExechookBackoff < time.Second { handleError(log, true, "ERROR: --exechook-backoff must be at least 1s") } - - execHookChannel = make(chan bool) + exechookChannel = make(chan bool, 1) } if *flWebhookURL != "" { @@ -376,8 +377,7 @@ func main() { if *flWebhookBackoff < time.Second { handleError(log, true, "ERROR: --webhook-backoff must be at least 1s") } - - webHookChannel = make(chan bool) + webhookChannel = make(chan bool, 1) } if *flPassword != "" && *flPasswordFile != "" { @@ -563,7 +563,7 @@ func main() { *flWebhookBackoff, hook.NewHookData(), log, - webHookChannel, + webhookChannel, ) go webhookRunner.Run(context.Background()) } @@ -584,7 +584,7 @@ func main() { *flExechookBackoff, hook.NewHookData(), log, - execHookChannel, + exechookChannel, ) go exechookRunner.Run(context.Background()) } @@ -630,17 +630,17 @@ func main() { } if initialSync { - // Wait for hooks to complete at least once before checking whether to stop + // Wait for hooks to complete at least once, if not nil, before checking whether to stop program // Assumes that if hook channels are not nil, they will have at least one value before getting closed - if execHookChannel != nil { - execHookChannelFinishedSuccessfully:= <-execHookChannel - if !execHookChannelFinishedSuccessfully { + if exechookChannel != nil { + exechookChannelFinishedSuccessfully := <-exechookChannel + if !exechookChannelFinishedSuccessfully { log.Error(nil, "exec hook completed with error") } } - if webHookChannel != nil { - webHookChannelFinishedSuccessfully:= <-webHookChannel - if !webHookChannelFinishedSuccessfully { + if webhookChannel != nil { + webhookChannelFinishedSuccessfully := <-webhookChannel + if !webhookChannelFinishedSuccessfully { log.Error(nil, "web hook completed with error") } } diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go index ce027f2..86febea 100644 --- a/pkg/hook/hook.go +++ b/pkg/hook/hook.go @@ -96,7 +96,8 @@ type HookRunner struct { data *hookData // Logger logger *logging.Logger - // Has succeeded once Chanel, sends true if first run executed successfully and false if it failed + // hasCompletedOnce is used to send true if and only if first run executed successfully and false otherwise to some receiver. + // should be initialised to a buffered channel of size 1. Is only meant to be used within sendCompletedOnceMessageIfApplicable hasCompletedOnce chan bool } @@ -126,21 +127,22 @@ func (r *HookRunner) Run(ctx context.Context) { r.logger.Error(err, "hook failed") updateHookRunCountMetric(r.hook.Name(), "error") // don't want to sleep unnecessarily if we are going to terminate anyways - r.sendCompletedOnceMessage(false) + r.sendCompletedOnceMessageIfApplicable(false) time.Sleep(r.backoff) } else { updateHookRunCountMetric(r.hook.Name(), "success") lastHash = hash - r.sendCompletedOnceMessage(true) + r.sendCompletedOnceMessageIfApplicable(true) break } } } } -// sendCompletedOnceMessage forwards the success status (as a boolean) of the first execution of HookRunner, the first time -// to the r.hasCompletedOnce channel -func (r *HookRunner) sendCompletedOnceMessage(completedSuccessfully bool) { +// If hasCompletedOnce is nil, does nothing. Otherwise, forwards the caller provided success status (as a boolean) of +// HookRunner to receivers of hasCompletedOnce, closes said chanel, and sets hasCompletedOnce to nil. +// Using this function to write to hasCompletedOnce ensures it is only every written to once. +func (r *HookRunner) sendCompletedOnceMessageIfApplicable(completedSuccessfully bool) { if r.hasCompletedOnce != nil { r.hasCompletedOnce <- completedSuccessfully close(r.hasCompletedOnce) From a362b4a5f4bdb0eb848925deec8f176b9feee200 Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Sat, 8 Jan 2022 16:23:13 -0500 Subject: [PATCH 03/14] feat: added tests for this PR that test exechook paired with --one-time flag --- test_e2e.sh | 77 ++++++++++++++++++++++++ test_exechook_command_fail_with_sleep.sh | 22 +++++++ test_exechook_command_with_sleep.sh | 22 +++++++ 3 files changed, 121 insertions(+) create mode 100755 test_exechook_command_fail_with_sleep.sh create mode 100755 test_exechook_command_with_sleep.sh diff --git a/test_e2e.sh b/test_e2e.sh index c69f2f6..a42a77a 100755 --- a/test_e2e.sh +++ b/test_e2e.sh @@ -143,6 +143,8 @@ SLOW_GIT_FETCH=/slow_git_fetch.sh ASKPASS_GIT=/askpass_git.sh EXECHOOK_COMMAND=/test_exechook_command.sh EXECHOOK_COMMAND_FAIL=/test_exechook_command_fail.sh +EXECHOOK_COMMAND_SLEEPY=/test_exechook_command_with_sleep.sh +EXECHOOK_COMMAND_FAIL_SLEEPY=/test_exechook_command_fail_with_sleep.sh RUNLOG="$DIR/runlog.exechook-fail-retry" rm -f $RUNLOG touch $RUNLOG @@ -1072,6 +1074,81 @@ function e2e::exechook_fail_retry() { fi } +############################################## +# Test exechook-success with GIT_SYNC_ONE_TIME +############################################## +function exechook_success_once_helper() { + # First sync + echo "$FUNCNAME 2" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 2" + + GIT_SYNC \ + --period=${2}ms \ + --one-time \ + --repo="file://$REPO" \ + --branch="$MAIN_BRANCH" \ + --root="$ROOT" \ + --link="link" \ + --exechook-command="$EXECHOOK_COMMAND_SLEEPY" \ + >> "$1" 2>&1 & + sleep 7 + assert_link_exists "$ROOT"/link + assert_file_exists "$ROOT"/link/file + assert_file_exists "$ROOT"/link/exechook + assert_file_exists "$ROOT"/link/link-exechook + assert_file_eq "$ROOT"/link/file "$FUNCNAME 2" + assert_file_eq "$ROOT"/link/exechook "$FUNCNAME 2" + assert_file_eq "$ROOT"/link/link-exechook "$FUNCNAME 2" +} + +function e2e::exechook_success_once() { + for i in $(seq 0 50 200); do + for j in $(seq 2); do + clean_root + init_repo + exechook_success_once_helper "$1" "$i" + done + done +} + +############################################## +# Test exechook-fail with GIT_SYNC_ONE_TIME +############################################## +function exechook_fail_once_helper() { + cat /dev/null > "$RUNLOG" + # First sync - return a failure to ensure that we try again + echo "$FUNCNAME 2" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 2" + + GIT_SYNC \ + --period="$2"ms \ + --one-time \ + --repo="file://$REPO" \ + --branch="$MAIN_BRANCH" \ + --root="$ROOT" \ + --link="link" \ + --exechook-command="$EXECHOOK_COMMAND_FAIL_SLEEPY" \ + --exechook-backoff=1s \ + >> "$1" 2>&1 & + + # Check that exechook was called + sleep 10 + RUNS=$(cat "$RUNLOG" | wc -l) + if [[ "$RUNS" < 2 ]]; then + fail "exechook called $RUNS times, it should be at least 2" + fi +} + +function e2e::exechook_fail_once() { + for i in $(seq 0 50 200); do + for j in $(seq 2); do + clean_root + init_repo + exechook_fail_once_helper "$1" "$i" + done + done +} + ############################################## # Test webhook success ############################################## diff --git a/test_exechook_command_fail_with_sleep.sh b/test_exechook_command_fail_with_sleep.sh new file mode 100755 index 0000000..492e4a1 --- /dev/null +++ b/test_exechook_command_fail_with_sleep.sh @@ -0,0 +1,22 @@ +#!/bin/sh +# +# Copyright 2021 The Kubernetes Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Use for e2e test of --exechook-command. +# This option takes no command arguments, so requires a wrapper script. + +sleep 4 +date >> /var/log/runs +exit 1 diff --git a/test_exechook_command_with_sleep.sh b/test_exechook_command_with_sleep.sh new file mode 100755 index 0000000..c494308 --- /dev/null +++ b/test_exechook_command_with_sleep.sh @@ -0,0 +1,22 @@ +#!/bin/sh +# +# Copyright 2020 The Kubernetes Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Use for e2e test of --exechook-command. +# This option takes no command arguments, so requires a wrapper script. + +sleep 4 +cat file > exechook +cat ../link/file > link-exechook From d2a4320e6c17d7ce9f679c89db9f724b9fd56fcd Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Mon, 10 Jan 2022 18:42:34 -0500 Subject: [PATCH 04/14] fix: execute hook check only on --one-time Execute hook check only if --one-time is true and corrected log spelling of webhook and exechook --- cmd/git-sync/main.go | 46 ++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index 396cb07..a8bde93 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -364,7 +364,10 @@ func main() { if *flExechookBackoff < time.Second { handleError(log, true, "ERROR: --exechook-backoff must be at least 1s") } - exechookChannel = make(chan bool, 1) + + if *flOneTime { + exechookChannel = make(chan bool, 1) + } } if *flWebhookURL != "" { @@ -377,7 +380,10 @@ func main() { if *flWebhookBackoff < time.Second { handleError(log, true, "ERROR: --webhook-backoff must be at least 1s") } - webhookChannel = make(chan bool, 1) + + if *flOneTime { + webhookChannel = make(chan bool, 1) + } } if *flPassword != "" && *flPasswordFile != "" { @@ -630,25 +636,27 @@ func main() { } if initialSync { - // Wait for hooks to complete at least once, if not nil, before checking whether to stop program - // Assumes that if hook channels are not nil, they will have at least one value before getting closed - if exechookChannel != nil { - exechookChannelFinishedSuccessfully := <-exechookChannel - if !exechookChannelFinishedSuccessfully { - log.Error(nil, "exec hook completed with error") - } - } - if webhookChannel != nil { - webhookChannelFinishedSuccessfully := <-webhookChannel - if !webhookChannelFinishedSuccessfully { - log.Error(nil, "web hook completed with error") - } - } - - // Determine if git-sync should terminate + // Determine if git-sync should terminate for one of several reasons if *flOneTime { + // Wait for hooks to complete at least once, if not nil, before checking whether to stop program + // Assumes that if hook channels are not nil, they will have at least one value before getting closed + exitCode := 0 // if all hooks succeeded, exit code is 0, else set to 1 + if exechookChannel != nil { + exechookChannelFinishedSuccessfully := <-exechookChannel + if !exechookChannelFinishedSuccessfully { + log.Error(nil, "exechook completed with error") + exitCode = 1 + } + } + if webhookChannel != nil { + webhookChannelFinishedSuccessfully := <-webhookChannel + if !webhookChannelFinishedSuccessfully { + log.Error(nil, "webhook completed with error") + } + exitCode = 1 + } log.DeleteErrorFile() - os.Exit(0) + os.Exit(exitCode) } if isHash, err := git.RevIsHash(ctx); err != nil { log.Error(err, "can't tell if rev is a git hash, exiting", "rev", git.rev) From f67669662b3f17c10421be0b8e81642d1dbfac1a Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Mon, 10 Jan 2022 18:48:23 -0500 Subject: [PATCH 05/14] fix: corrected comment for hook channels to refflect true functionality --- cmd/git-sync/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index a8bde93..05d909f 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -174,7 +174,7 @@ var ( }, []string{"status"}) ) -// Channels for ensuring hooks execute at least once before terminating. +// Channels for ensuring hooks execute at least once before terminating when --one-time flag is set. // Should be nil if and only if corresponding hook is not defined and if initialised, will only every get written // to once. var exechookChannel, webhookChannel chan bool From 0df6e005d3f143f640437e22b5cd0ec98a146bd5 Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Mon, 10 Jan 2022 19:00:37 -0500 Subject: [PATCH 06/14] refactor: fixed in-line documentation and added line-wraps where needed --- cmd/git-sync/main.go | 18 ++++++++++-------- pkg/hook/hook.go | 16 ++++++++++------ 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index 05d909f..a71ac69 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -174,9 +174,9 @@ var ( }, []string{"status"}) ) -// Channels for ensuring hooks execute at least once before terminating when --one-time flag is set. -// Should be nil if and only if corresponding hook is not defined and if initialised, will only every get written -// to once. +// Channels for ensuring hooks execute at least once before terminating when +// --one-time flag is set. Should be nil if and only if corresponding hook is +// not defined and if initialised, will only ever get written to once. var exechookChannel, webhookChannel chan bool const ( @@ -475,8 +475,8 @@ func main() { run: cmdRunner, } - // This context is used only for git credentials initialization. There are no long-running operations like - // `git clone`, so hopefully 30 seconds will be enough. + // This context is used only for git credentials initialization. There are + // no long-running operations like `git clone`, so hopefully 30 seconds will be enough. ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) if *flUsername != "" { @@ -638,9 +638,11 @@ func main() { if initialSync { // Determine if git-sync should terminate for one of several reasons if *flOneTime { - // Wait for hooks to complete at least once, if not nil, before checking whether to stop program - // Assumes that if hook channels are not nil, they will have at least one value before getting closed - exitCode := 0 // if all hooks succeeded, exit code is 0, else set to 1 + // Wait for hooks to complete at least once, if not nil, before + // checking whether to stop program. + // Assumes that if hook channels are not nil, they will have at + // least one value before getting closed + exitCode := 0 // is 0 if all hooks succeed, else is 1 if exechookChannel != nil { exechookChannelFinishedSuccessfully := <-exechookChannel if !exechookChannelFinishedSuccessfully { diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go index 86febea..06e68cd 100644 --- a/pkg/hook/hook.go +++ b/pkg/hook/hook.go @@ -96,8 +96,10 @@ type HookRunner struct { data *hookData // Logger logger *logging.Logger - // hasCompletedOnce is used to send true if and only if first run executed successfully and false otherwise to some receiver. - // should be initialised to a buffered channel of size 1. Is only meant to be used within sendCompletedOnceMessageIfApplicable + // hasCompletedOnce is used to send true if and only if first run executed + // successfully and false otherwise to some receiver. Should be + // initialised to a buffered channel of size 1. + // Is only meant for use within sendCompletedOnceMessageIfApplicable. hasCompletedOnce chan bool } @@ -126,7 +128,7 @@ func (r *HookRunner) Run(ctx context.Context) { if err := r.hook.Do(ctx, hash); err != nil { r.logger.Error(err, "hook failed") updateHookRunCountMetric(r.hook.Name(), "error") - // don't want to sleep unnecessarily if we are going to terminate anyways + // don't want to sleep unnecessarily terminating anyways r.sendCompletedOnceMessageIfApplicable(false) time.Sleep(r.backoff) } else { @@ -139,9 +141,11 @@ func (r *HookRunner) Run(ctx context.Context) { } } -// If hasCompletedOnce is nil, does nothing. Otherwise, forwards the caller provided success status (as a boolean) of -// HookRunner to receivers of hasCompletedOnce, closes said chanel, and sets hasCompletedOnce to nil. -// Using this function to write to hasCompletedOnce ensures it is only every written to once. +// If hasCompletedOnce is nil, does nothing. Otherwise, forwards the caller +// provided success status (as a boolean) of HookRunner to receivers of +// hasCompletedOnce, closes said chanel, and sets hasCompletedOnce to nil. +// Using this function to write to hasCompletedOnce ensures it is only ever +// written to once. func (r *HookRunner) sendCompletedOnceMessageIfApplicable(completedSuccessfully bool) { if r.hasCompletedOnce != nil { r.hasCompletedOnce <- completedSuccessfully From b8970bef857640b035e90cc81b8e62848f80fd2e Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Mon, 10 Jan 2022 19:20:28 -0500 Subject: [PATCH 07/14] fix: reduced time spent sleeping in SLEEPTY exechooks --- test_exechook_command_fail_with_sleep.sh | 2 +- test_exechook_command_with_sleep.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test_exechook_command_fail_with_sleep.sh b/test_exechook_command_fail_with_sleep.sh index 492e4a1..321f3d5 100755 --- a/test_exechook_command_fail_with_sleep.sh +++ b/test_exechook_command_fail_with_sleep.sh @@ -17,6 +17,6 @@ # Use for e2e test of --exechook-command. # This option takes no command arguments, so requires a wrapper script. -sleep 4 +sleep 3 date >> /var/log/runs exit 1 diff --git a/test_exechook_command_with_sleep.sh b/test_exechook_command_with_sleep.sh index c494308..78d633c 100755 --- a/test_exechook_command_with_sleep.sh +++ b/test_exechook_command_with_sleep.sh @@ -17,6 +17,6 @@ # Use for e2e test of --exechook-command. # This option takes no command arguments, so requires a wrapper script. -sleep 4 +sleep 3 cat file > exechook cat ../link/file > link-exechook From 8e098f8c2adbfb1bf519d03e704a1a6df818e5ad Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Mon, 10 Jan 2022 19:23:29 -0500 Subject: [PATCH 08/14] fix: made exechook_once tests more efficient * Removed multiple iterations of tests * Reduced sleep time after GIT_SYNC command execution --- test_e2e.sh | 46 +++++++++++++--------------------------------- 1 file changed, 13 insertions(+), 33 deletions(-) diff --git a/test_e2e.sh b/test_e2e.sh index a42a77a..98da8c6 100755 --- a/test_e2e.sh +++ b/test_e2e.sh @@ -1077,13 +1077,13 @@ function e2e::exechook_fail_retry() { ############################################## # Test exechook-success with GIT_SYNC_ONE_TIME ############################################## -function exechook_success_once_helper() { +function e2e::exechook_success_once() { # First sync - echo "$FUNCNAME 2" > "$REPO"/file - git -C "$REPO" commit -qam "$FUNCNAME 2" + echo "$FUNCNAME 1" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 1" GIT_SYNC \ - --period=${2}ms \ + --period=100ms \ --one-time \ --repo="file://$REPO" \ --branch="$MAIN_BRANCH" \ @@ -1091,37 +1091,27 @@ function exechook_success_once_helper() { --link="link" \ --exechook-command="$EXECHOOK_COMMAND_SLEEPY" \ >> "$1" 2>&1 & - sleep 7 + sleep 4 assert_link_exists "$ROOT"/link assert_file_exists "$ROOT"/link/file assert_file_exists "$ROOT"/link/exechook assert_file_exists "$ROOT"/link/link-exechook - assert_file_eq "$ROOT"/link/file "$FUNCNAME 2" - assert_file_eq "$ROOT"/link/exechook "$FUNCNAME 2" - assert_file_eq "$ROOT"/link/link-exechook "$FUNCNAME 2" -} - -function e2e::exechook_success_once() { - for i in $(seq 0 50 200); do - for j in $(seq 2); do - clean_root - init_repo - exechook_success_once_helper "$1" "$i" - done - done + assert_file_eq "$ROOT"/link/file "$FUNCNAME 1" + assert_file_eq "$ROOT"/link/exechook "$FUNCNAME 1" + assert_file_eq "$ROOT"/link/link-exechook "$FUNCNAME 1" } ############################################## # Test exechook-fail with GIT_SYNC_ONE_TIME ############################################## -function exechook_fail_once_helper() { +function e2e::exechook_fail_once() { cat /dev/null > "$RUNLOG" # First sync - return a failure to ensure that we try again - echo "$FUNCNAME 2" > "$REPO"/file - git -C "$REPO" commit -qam "$FUNCNAME 2" + echo "$FUNCNAME 1" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 1" GIT_SYNC \ - --period="$2"ms \ + --period=100ms \ --one-time \ --repo="file://$REPO" \ --branch="$MAIN_BRANCH" \ @@ -1132,23 +1122,13 @@ function exechook_fail_once_helper() { >> "$1" 2>&1 & # Check that exechook was called - sleep 10 + sleep 4 RUNS=$(cat "$RUNLOG" | wc -l) if [[ "$RUNS" < 2 ]]; then fail "exechook called $RUNS times, it should be at least 2" fi } -function e2e::exechook_fail_once() { - for i in $(seq 0 50 200); do - for j in $(seq 2); do - clean_root - init_repo - exechook_fail_once_helper "$1" "$i" - done - done -} - ############################################## # Test webhook success ############################################## From 0246fb509f532d6831225b02da56dbf724c02fa5 Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Mon, 10 Jan 2022 19:53:11 -0500 Subject: [PATCH 09/14] feat: added e2e --one-time tests for webhook --- test_e2e.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/test_e2e.sh b/test_e2e.sh index 98da8c6..0f9d965 100755 --- a/test_e2e.sh +++ b/test_e2e.sh @@ -1091,7 +1091,10 @@ function e2e::exechook_success_once() { --link="link" \ --exechook-command="$EXECHOOK_COMMAND_SLEEPY" \ >> "$1" 2>&1 & - sleep 4 + + # Original test sleeps for 2 seconds, so sleep for 2 seconds more than + # hook's sleep time + sleep 5 assert_link_exists "$ROOT"/link assert_file_exists "$ROOT"/link/file assert_file_exists "$ROOT"/link/exechook @@ -1122,7 +1125,9 @@ function e2e::exechook_fail_once() { >> "$1" 2>&1 & # Check that exechook was called - sleep 4 + # Original test sleeps for 2 seconds, so sleep for 2 seconds more than + # hook's sleep time + sleep 5 RUNS=$(cat "$RUNLOG" | wc -l) if [[ "$RUNS" < 2 ]]; then fail "exechook called $RUNS times, it should be at least 2" @@ -1225,6 +1230,79 @@ function e2e::webhook_fail_retry() { docker_kill "$CTR" } +############################################## +# Test webhook success with --one-time +############################################## +function e2e::webhook_success_once() { + HITLOG="$DIR/hitlog" + + # First sync + cat /dev/null > "$HITLOG" + CTR=$(docker_run \ + -v "$HITLOG":/var/log/hits \ + e2e/test/test-ncsvr \ + 80 'sleep 3 && echo -e "HTTP/1.1 200 OK\r\n"') + IP=$(docker_ip "$CTR") + echo "$FUNCNAME 1" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 1" + + GIT_SYNC \ + --period=100ms \ + --one-time \ + --repo="file://$REPO" \ + --branch="$MAIN_BRANCH" \ + --root="$ROOT" \ + --webhook-url="http://$IP" \ + --webhook-success-status=200 \ + --link="link" \ + >> "$1" 2>&1 & + + # check that basic call works + sleep 5 # preserve original diff between hook sleep and test sleep time + HITS=$(cat "$HITLOG" | wc -l) + if [[ "$HITS" < 1 ]]; then + fail "webhook 1 called $HITS times" + fi + + docker_kill "$CTR" +} + +############################################## +# Test webhook fail with --one-time +############################################## +function e2e::webhook_fail_retry() { + HITLOG="$DIR/hitlog" + + # First sync - return a failure to ensure that we try again + cat /dev/null > "$HITLOG" + CTR=$(docker_run \ + -v "$HITLOG":/var/log/hits \ + e2e/test/test-ncsvr \ + 80 'sleep 3 && echo -e "HTTP/1.1 500 Internal Server Error\r\n"') + IP=$(docker_ip "$CTR") + echo "$FUNCNAME 1" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 1" + + GIT_SYNC \ + --period=100ms \ + --one-time \ + --repo="file://$REPO" \ + --branch="$MAIN_BRANCH" \ + --root="$ROOT" \ + --webhook-url="http://$IP" \ + --webhook-success-status=200 \ + --link="link" \ + >> "$1" 2>&1 & + + # Check that webhook was called + sleep 5 # preserve original diff between hook sleep and test sleep time + HITS=$(cat "$HITLOG" | wc -l) + if [[ "$HITS" < 1 ]]; then + fail "webhook 1 called $HITS times" + fi + docker_kill "$CTR" +} + ############################################## # Test webhook fire-and-forget ############################################## From 043c356c03d1b6dc3c68ff1c40db9ed3d3b8df85 Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Fri, 14 Jan 2022 03:23:20 -0500 Subject: [PATCH 10/14] fix: fixed e2e tests Resolved a few issues with e2e tests discovered after running test_e2d.sh. Just finished setting up Linux (Fedora) environment in which this script can be run. --- test_e2e.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test_e2e.sh b/test_e2e.sh index 0f9d965..302dfab 100755 --- a/test_e2e.sh +++ b/test_e2e.sh @@ -163,6 +163,8 @@ function GIT_SYNC() { -v "$(pwd)/askpass_git.sh":"$ASKPASS_GIT":ro \ -v "$(pwd)/test_exechook_command.sh":"$EXECHOOK_COMMAND":ro \ -v "$(pwd)/test_exechook_command_fail.sh":"$EXECHOOK_COMMAND_FAIL":ro \ + -v "$(pwd)/test_exechook_command_with_sleep.sh":"$EXECHOOK_COMMAND_SLEEPY":ro \ + -v "$(pwd)/test_exechook_command_fail_with_sleep.sh":"$EXECHOOK_COMMAND_FAIL_SLEEPY":ro \ -v "$RUNLOG":/var/log/runs \ -v "$DOT_SSH/id_test":"/etc/git-secret/ssh":ro \ --env XDG_CONFIG_HOME=$DIR \ @@ -1129,7 +1131,7 @@ function e2e::exechook_fail_once() { # hook's sleep time sleep 5 RUNS=$(cat "$RUNLOG" | wc -l) - if [[ "$RUNS" < 2 ]]; then + if [[ "$RUNS" < 1 ]]; then fail "exechook called $RUNS times, it should be at least 2" fi } @@ -1270,7 +1272,7 @@ function e2e::webhook_success_once() { ############################################## # Test webhook fail with --one-time ############################################## -function e2e::webhook_fail_retry() { +function e2e::webhook_fail_retry_once() { HITLOG="$DIR/hitlog" # First sync - return a failure to ensure that we try again From 8827bf7489b8c0d8f21aacc22eb98534b895d22d Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Fri, 14 Jan 2022 13:32:01 -0500 Subject: [PATCH 11/14] refactor: imporved efficiency and code style * changed sendCompletedOnceMessageIfApplicable so that it kills thread after executing * moved logic for waiting on HookRunner into function defined in HookRunner itself --- cmd/git-sync/main.go | 39 ++++++++++++++------------------------- pkg/hook/hook.go | 25 +++++++++++++++++++++++-- 2 files changed, 37 insertions(+), 27 deletions(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index a71ac69..408798a 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -174,11 +174,6 @@ var ( }, []string{"status"}) ) -// Channels for ensuring hooks execute at least once before terminating when -// --one-time flag is set. Should be nil if and only if corresponding hook is -// not defined and if initialised, will only ever get written to once. -var exechookChannel, webhookChannel chan bool - const ( metricKeySuccess = "success" metricKeyError = "error" @@ -364,10 +359,6 @@ func main() { if *flExechookBackoff < time.Second { handleError(log, true, "ERROR: --exechook-backoff must be at least 1s") } - - if *flOneTime { - exechookChannel = make(chan bool, 1) - } } if *flWebhookURL != "" { @@ -380,10 +371,6 @@ func main() { if *flWebhookBackoff < time.Second { handleError(log, true, "ERROR: --webhook-backoff must be at least 1s") } - - if *flOneTime { - webhookChannel = make(chan bool, 1) - } } if *flPassword != "" && *flPasswordFile != "" { @@ -557,6 +544,10 @@ func main() { // Startup webhooks goroutine var webhookRunner *hook.HookRunner if *flWebhookURL != "" { + var webhookChannel chan bool + if *flOneTime { + webhookChannel = make(chan bool, 1) + } webhook := hook.NewWebhook( *flWebhookURL, *flWebhookMethod, @@ -577,6 +568,10 @@ func main() { // Startup exechooks goroutine var exechookRunner *hook.HookRunner if *flExechookCommand != "" { + var exechookChannel chan bool + if *flOneTime { + exechookChannel = make(chan bool, 1) + } exechook := hook.NewExechook( cmd.NewRunner(log), *flExechookCommand, @@ -642,19 +637,13 @@ func main() { // checking whether to stop program. // Assumes that if hook channels are not nil, they will have at // least one value before getting closed - exitCode := 0 // is 0 if all hooks succeed, else is 1 - if exechookChannel != nil { - exechookChannelFinishedSuccessfully := <-exechookChannel - if !exechookChannelFinishedSuccessfully { - log.Error(nil, "exechook completed with error") - exitCode = 1 - } + exitCode := 0 // is 0 if all hooks succeed, else is 1 + if err = exechookRunner.WaitForCompletion(); err != nil { + log.Error(err, "exechook completed with error") + exitCode = 1 } - if webhookChannel != nil { - webhookChannelFinishedSuccessfully := <-webhookChannel - if !webhookChannelFinishedSuccessfully { - log.Error(nil, "webhook completed with error") - } + if err = webhookRunner.WaitForCompletion(); err != nil { + log.Error(err, "webhook completed with error") exitCode = 1 } log.DeleteErrorFile() diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go index 06e68cd..d1b89c2 100644 --- a/pkg/hook/hook.go +++ b/pkg/hook/hook.go @@ -18,6 +18,8 @@ package hook import ( "context" + "fmt" + "runtime" "sync" "time" @@ -143,17 +145,36 @@ func (r *HookRunner) Run(ctx context.Context) { // If hasCompletedOnce is nil, does nothing. Otherwise, forwards the caller // provided success status (as a boolean) of HookRunner to receivers of -// hasCompletedOnce, closes said chanel, and sets hasCompletedOnce to nil. +// hasCompletedOnce, closes said chanel, and terminates this goroutine. // Using this function to write to hasCompletedOnce ensures it is only ever // written to once. func (r *HookRunner) sendCompletedOnceMessageIfApplicable(completedSuccessfully bool) { if r.hasCompletedOnce != nil { r.hasCompletedOnce <- completedSuccessfully close(r.hasCompletedOnce) - r.hasCompletedOnce = nil + runtime.Goexit() } } +// WaitForCompletion waits for HookRunner to send completion message to +// calling thread and returns either true if HookRunner executed successfully +// and some error otherwise. +// Assumes that r.hasCompletedOnce is not nil, but if it is, returns an error +func (r *HookRunner) WaitForCompletion() error { + // Make sure function should be called + if r.hasCompletedOnce == nil { + return fmt.Errorf("HookRunner.WaitForCompletion called on async runner") + } + + // Perform wait on HookRunner + exechookChannelFinishedSuccessfully := <-r.hasCompletedOnce + if !exechookChannelFinishedSuccessfully { + return fmt.Errorf("exechook completed with error") + } + + return nil +} + func updateHookRunCountMetric(name, status string) { hookRunCount.WithLabelValues(name, status).Inc() } From 5490b721d8e37e3a0872e395a126b6c76f36a40d Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Fri, 14 Jan 2022 13:53:09 -0500 Subject: [PATCH 12/14] fix: made tests more efficient by waiting less --- test_e2e.sh | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/test_e2e.sh b/test_e2e.sh index 302dfab..40bb137 100755 --- a/test_e2e.sh +++ b/test_e2e.sh @@ -1092,11 +1092,9 @@ function e2e::exechook_success_once() { --root="$ROOT" \ --link="link" \ --exechook-command="$EXECHOOK_COMMAND_SLEEPY" \ - >> "$1" 2>&1 & + >> "$1" 2>&1 - # Original test sleeps for 2 seconds, so sleep for 2 seconds more than - # hook's sleep time - sleep 5 + sleep 2 assert_link_exists "$ROOT"/link assert_file_exists "$ROOT"/link/file assert_file_exists "$ROOT"/link/exechook @@ -1124,15 +1122,13 @@ function e2e::exechook_fail_once() { --link="link" \ --exechook-command="$EXECHOOK_COMMAND_FAIL_SLEEPY" \ --exechook-backoff=1s \ - >> "$1" 2>&1 & + >> "$1" 2>&1 # Check that exechook was called - # Original test sleeps for 2 seconds, so sleep for 2 seconds more than - # hook's sleep time - sleep 5 + sleep 2 RUNS=$(cat "$RUNLOG" | wc -l) if [[ "$RUNS" < 1 ]]; then - fail "exechook called $RUNS times, it should be at least 2" + fail "exechook called $RUNS times, it should be at least 1" fi } @@ -1257,10 +1253,10 @@ function e2e::webhook_success_once() { --webhook-url="http://$IP" \ --webhook-success-status=200 \ --link="link" \ - >> "$1" 2>&1 & + >> "$1" 2>&1 # check that basic call works - sleep 5 # preserve original diff between hook sleep and test sleep time + sleep 2 HITS=$(cat "$HITLOG" | wc -l) if [[ "$HITS" < 1 ]]; then fail "webhook 1 called $HITS times" @@ -1294,10 +1290,10 @@ function e2e::webhook_fail_retry_once() { --webhook-url="http://$IP" \ --webhook-success-status=200 \ --link="link" \ - >> "$1" 2>&1 & + >> "$1" 2>&1 # Check that webhook was called - sleep 5 # preserve original diff between hook sleep and test sleep time + sleep 2 HITS=$(cat "$HITLOG" | wc -l) if [[ "$HITS" < 1 ]]; then fail "webhook 1 called $HITS times" From 394b2c71490744c0b50fbdf59de17768ce6c725e Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Sat, 15 Jan 2022 15:19:31 -0500 Subject: [PATCH 13/14] fix: perform hook null check before waiting Last 2 commits introduced fatal null-pointer exception when waiting on HookRunner object when not set, e.g. waiting for WebhookRunner when no webhook is defined. Fixed bug and got test_e2e.sh to run successfully. --- cmd/git-sync/main.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index 408798a..895a6e1 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -638,13 +638,17 @@ func main() { // Assumes that if hook channels are not nil, they will have at // least one value before getting closed exitCode := 0 // is 0 if all hooks succeed, else is 1 - if err = exechookRunner.WaitForCompletion(); err != nil { - log.Error(err, "exechook completed with error") - exitCode = 1 + if exechookRunner != nil { + if err = exechookRunner.WaitForCompletion(); err != nil { + log.Error(err, "exechook completed with error") + exitCode = 1 + } } - if err = webhookRunner.WaitForCompletion(); err != nil { - log.Error(err, "webhook completed with error") - exitCode = 1 + if webhookRunner != nil { + if err = webhookRunner.WaitForCompletion(); err != nil { + log.Error(err, "webhook completed with error") + exitCode = 1 + } } log.DeleteErrorFile() os.Exit(exitCode) From ad6a34abb5161c47c82070bf43b388b849a3cdd9 Mon Sep 17 00:00:00 2001 From: ChrisERo Date: Sat, 15 Jan 2022 15:23:06 -0500 Subject: [PATCH 14/14] refactor: corrected variable names and logs inside HookRunner.WaitForCompletion --- pkg/hook/hook.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go index d1b89c2..13f92e3 100644 --- a/pkg/hook/hook.go +++ b/pkg/hook/hook.go @@ -167,9 +167,9 @@ func (r *HookRunner) WaitForCompletion() error { } // Perform wait on HookRunner - exechookChannelFinishedSuccessfully := <-r.hasCompletedOnce - if !exechookChannelFinishedSuccessfully { - return fmt.Errorf("exechook completed with error") + hookRunnerFinishedSuccessfully := <-r.hasCompletedOnce + if !hookRunnerFinishedSuccessfully { + return fmt.Errorf("hook completed with error") } return nil