fix: use "double fork" to invoke port forward within Taskfile #1148

JeffreyDallas · 2025-01-13T18:33:24Z

Description

This pull request changes the following:

Tried various method suggested by stackoverflow, chatGPT, Gemini, all suggesting Taskfile may have some bugs/limitation that causing "port-forward" was created in incorrect process groups. The workaround is to use double fork to invoke a program.
Remove re-enable port-foward from test script

Related Issues

Closes Port-forward called from Taskfile exit itself #1115

Signed-off-by: Jeffrey Tang <[email protected]>

JeffreyDallas · 2025-01-13T18:35:17Z

Reference from Gemini

The Problem: Process Groups and Signal Propagation

The core issue is very likely related to process groups and how signals are propagated within them. Even with setsid (which creates a new session), if the kubectl port-forward process isn't properly placed in its own process group, Taskfile (or its parent process) might be sending signals that inadvertently terminate the kubectl process when a connection is established and then closed.

Here's why this happens:

Process Groups: Processes are organized into process groups. When a signal is sent to a process group, all processes within that group receive the signal.

Taskfile's Process Management: Taskfile, when it finishes executing a task, might be sending signals (like SIGHUP or SIGTERM) to its child processes and their process groups.

Connection Establishment and Closure: When you connect with nc, the following happens:

nc connects to the forwarded port.
kubectl handles the connection, forwarding traffic to the pod.
nc closes the connection.
This connection closure might be triggering some internal cleanup or signal handling within kubectl. If kubectl is still in the same process group as Taskfile, Taskfile's cleanup signals might then reach kubectl, causing it to exit.

The Solution: Explicit Process Group Management

The most reliable solution is to explicitly create a new process group for the kubectl port-forward process. This ensures that signals sent by Taskfile won't affect it

…portforward-quit-task Signed-off-by: Jeffrey Tang <[email protected]> # Conflicts: # Taskfile.helper.yml

github-actions · 2025-01-13T18:44:01Z

Unit Test Results - Linux

1 files ±0 58 suites ±0 4s ⏱️ ±0s
227 tests ±0 227 ✅ ±0 0 💤 ±0 0 ❌ ±0
232 runs ±0 232 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit f7dc8ca. ± Comparison against base commit 522b75d.

♻️ This comment has been updated with latest results.

github-actions · 2025-01-13T18:45:11Z

Unit Test Results - Windows

1 files ±0 58 suites ±0 13s ⏱️ ±0s
227 tests ±0 227 ✅ ±0 0 💤 ±0 0 ❌ ±0
232 runs ±0 232 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit f7dc8ca. ± Comparison against base commit 522b75d.

♻️ This comment has been updated with latest results.

jeromy-cannon · 2025-01-13T18:54:36Z

@JeffreyDallas , I already fixed this: #1132

Signed-off-by: Jeffrey Tang <[email protected]>

github-actions · 2025-01-13T22:27:17Z

E2E Test Report

16 files ±0 121 suites ±0 1h 22m 7s ⏱️ -33s
258 tests ±0 258 ✅ ±0 0 💤 ±0 0 ❌ ±0
261 runs ±0 261 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 9dd76c4. ± Comparison against base commit 0784b46.

github-actions · 2025-01-13T22:27:17Z

E2E Test Report

17 files 126 suites 1h 26m 47s ⏱️
258 tests 258 ✅ 0 💤 0 ❌
269 runs 269 ✅ 0 💤 0 ❌

Results for commit f7dc8ca.

♻️ This comment has been updated with latest results.

codacy-production · 2025-01-13T22:27:33Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `522b75d`¹	✅ 0.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`522b75d`)	Report Missing	Report Missing	Report Missing
Head commit (`f7dc8ca`)	21171	17728	83.74%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#1148)	2	0	0.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

codecov · 2025-01-13T22:28:25Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.97%. Comparing base (9bc6fcd) to head (f7dc8ca).
Report is 36 commits behind head on main.

Files with missing lines	Patch %	Lines
src/commands/mirror_node.ts	0.00%	1 Missing ⚠️
src/commands/relay.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1148      +/-   ##
==========================================
- Coverage   83.37%   82.97%   -0.40%     
==========================================
  Files          77       77              
  Lines       20809    21171     +362     
  Branches     1717     1468     -249     
==========================================
+ Hits        17349    17567     +218     
- Misses       3363     3544     +181     
+ Partials       97       60      -37

Files with missing lines	Coverage Δ
src/commands/mirror_node.ts	`71.02% <0.00%> (ø)`
src/commands/relay.ts	`82.02% <0.00%> (ø)`

... and 35 files with indirect coverage changes

Taskfile.helper.yml

jeromy-cannon

we should support -q/--quiet on all commands/subcommands. Instead of removing the flag, just add the flag to the destroy subcommands.

Signed-off-by: Jeffrey Tang <[email protected]>

## [0.34.0](v0.33.0...v0.34.0) (2025-01-24) ### Features * `solo deployment create` should use the context and cluster provided for where to save the remote config ([#1142](#1142)) ([fe42edd](fe42edd)) * Connect to multicluster deployments and validate remoteConfigs ([#1141](#1141)) ([c78e226](c78e226)) * for v0.59.x or greater set the internal IP address to 127.0.0.1 to avoid an ISS, in config.txt ([#1162](#1162)) ([4ca488b](4ca488b)) * node upgrade command and new e2e tests ([#1133](#1133)) ([1cf5893](1cf5893)) * separate explorer from mirror node install/uninstall ([#1177](#1177)) ([0887fa6](0887fa6)) * solo network destroy should update remote-config ([#1155](#1155)) ([98b028f](98b028f)) * Update solo to load remote config near entry point ([#1176](#1176)) ([473a650](473a650)) ### Bug Fixes * merge error due to change of remote_config_tasks ([#1197](#1197)) ([9d1a8cb](9d1a8cb)) * Normalize mirror node resources path ([#1175](#1175)) ([ab018a7](ab018a7)) * Refactor RemoteConfigTasks class ([#1185](#1185)) ([66cfc4d](66cfc4d)) * remove k8.getKubeConfig ([#1182](#1182)) ([89c557a](89c557a)) * Set mirror node importer startDate ([#1174](#1174)) ([9d9ef53](9d9ef53)) * update hedera explorer chart version and location ([#1188](#1188)) ([0c415ef](0c415ef)) * use "double fork" to invoke port forward within Taskfile ([#1148](#1148)) ([d662d3f](d662d3f))

swirlds-automation · 2025-01-24T15:41:29Z

🎉 This PR is included in version 0.34.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

JeffreyDallas added 4 commits January 10, 2025 13:53

add nohup

f9a870c

Signed-off-by: Jeffrey Tang <[email protected]>

check port-foward

e9a2326

Signed-off-by: Jeffrey Tang <[email protected]>

save

135a4f0

Signed-off-by: Jeffrey Tang <[email protected]>

save

f263913

Signed-off-by: Jeffrey Tang <[email protected]>

JeffreyDallas requested review from a team as code owners January 13, 2025 18:33

JeffreyDallas requested review from rbarkerSL and nathanklick January 13, 2025 18:33

JeffreyDallas marked this pull request as draft January 13, 2025 18:33

JeffreyDallas changed the title ~~fix: use "double forking" to invoke port forward within Taskfile~~ fix: use "double fork" to invoke port forward within Taskfile Jan 13, 2025

Merge commit '4ebae5d6ed897dfca91176fe0092c0cffced68d2' into 01115-D-…

52e56fc

…portforward-quit-task Signed-off-by: Jeffrey Tang <[email protected]> # Conflicts: # Taskfile.helper.yml

JeffreyDallas and others added 3 commits January 13, 2025 13:31

remove async

3c087e2

Signed-off-by: Jeffrey Tang <[email protected]>

remove unused

2b08294

Signed-off-by: Jeffrey Tang <[email protected]>

Merge branch 'main' into 01115-D-portforward-quit-task

9dd76c4

JeffreyDallas marked this pull request as ready for review January 13, 2025 20:00

JeffreyDallas requested a review from jeromy-cannon January 13, 2025 20:00

jeromy-cannon assigned JeffreyDallas Jan 13, 2025

instamenta previously approved these changes Jan 14, 2025

View reviewed changes

jeromy-cannon reviewed Jan 14, 2025

View reviewed changes

Taskfile.helper.yml Outdated Show resolved Hide resolved

jeromy-cannon requested changes Jan 14, 2025

View reviewed changes

add quiet flag

6054f15

Signed-off-by: Jeffrey Tang <[email protected]>

JeffreyDallas dismissed instamenta’s stale review via 6054f15 January 14, 2025 17:40

JeffreyDallas requested a review from leninmehedy as a code owner January 14, 2025 17:40

Merge branch 'main' into 01115-D-portforward-quit-task

7f6678f

jeromy-cannon approved these changes Jan 14, 2025

View reviewed changes

Merge branch 'main' into 01115-D-portforward-quit-task

f7dc8ca

JeffreyDallas merged commit d662d3f into main Jan 14, 2025
40 of 43 checks passed

JeffreyDallas deleted the 01115-D-portforward-quit-task branch January 14, 2025 23:06

swirlds-automation added the released label Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use "double fork" to invoke port forward within Taskfile #1148

fix: use "double fork" to invoke port forward within Taskfile #1148

JeffreyDallas commented Jan 13, 2025 •

edited by jeromy-cannon

Loading

JeffreyDallas commented Jan 13, 2025

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

jeromy-cannon commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025 •

edited

Loading

codacy-production bot commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

jeromy-cannon left a comment

swirlds-automation commented Jan 24, 2025

fix: use "double fork" to invoke port forward within Taskfile #1148

fix: use "double fork" to invoke port forward within Taskfile #1148

Conversation

JeffreyDallas commented Jan 13, 2025 • edited by jeromy-cannon Loading

Description

Related Issues

JeffreyDallas commented Jan 13, 2025

Reference from Gemini

github-actions bot commented Jan 13, 2025 • edited Loading

Unit Test Results - Linux

github-actions bot commented Jan 13, 2025 • edited Loading

Unit Test Results - Windows

jeromy-cannon commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

E2E Test Report

github-actions bot commented Jan 13, 2025 • edited Loading

E2E Test Report

codacy-production bot commented Jan 13, 2025 • edited Loading

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

Footnotes

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

jeromy-cannon left a comment

Choose a reason for hiding this comment

swirlds-automation commented Jan 24, 2025

JeffreyDallas commented Jan 13, 2025 •

edited by jeromy-cannon

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

codacy-production bot commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading