Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: terminate goroutines gracefully #21

Merged
merged 3 commits into from
Feb 7, 2024

Conversation

starbops
Copy link
Member

@starbops starbops commented Feb 4, 2024

IMPORTANT: Please do not create a Pull Request without creating an issue first.

Problem:

The context is not propagated appropriately. Goroutines do not work together correctly if some of them encounter failure.

Solution:

Passing down context and grouping goroutines to make them clean up resources after receiving the interrupt/terminate signal.

Related Issue:

harvester/harvester#5072

Test plan:

Create an IPPool object (please adjust the content according to your environment setup).

cat <<EOF | kubectl apply -f -
apiVersion: network.harvesterhci.io/v1alpha1
kind: IPPool
metadata:
  name: net-48
  namespace: default
spec:
  ipv4Config:
    serverIP: 192.168.48.77
    cidr: 192.168.48.0/24
    pool:
      start: 192.168.48.81
      end: 192.168.48.90
    router: 192.168.48.1
  networkName: default/net-48
EOF

Wait a few seconds for the agent Pod to become ready. Monitor the log (keep it opened and start a new shell for the next step):

$ kubectl -n harvester-system logs default-net-48-agent -f
Defaulted container "agent" out of: agent, ip-setter (init)
time="2024-02-04T13:35:12Z" level=info msg="Starting VM DHCP Agent: default-net-48-agent"
time="2024-02-04T13:35:12Z" level=info msg="Starting HTTP server"
time="2024-02-04T13:35:12Z" level=info msg="Listening on port: 8080"
time="2024-02-04T13:35:12Z" level=info msg="monitor ippool default/net-48"
time="2024-02-04T13:35:12Z" level=info msg="(dhcp.Run) starting DHCP service on nic eth1"
time="2024-02-04T13:35:12Z" level=info msg="(eventhandler.EventListener) starting IPPool event listener"
time="2024-02-04T13:35:12Z" level=info msg="(controller.Run) starting IPPool controller"
time="2024-02-04T13:35:33Z" level=info msg="(controller.sync) UPDATE default/net-48"

Remove the IPPool object to trigger agent Pod teardown.

kubectl delete ippools.network.harvesterhci.io net-48

Go back to the log monitor. The teardown logs should look like the following:

$ kubectl -n harvester-system logs default-net-48-agent -f
...
time="2024-02-05T08:29:04Z" level=info msg="(controller.sync) UPDATE default/net-48"
time="2024-02-05T08:29:04Z" level=info msg="Stopping HTTP server"
time="2024-02-05T08:29:04Z" level=info msg="(eventhandler.Stop) stopping IPPool event listener"
time="2024-02-05T08:29:04Z" level=info msg="(controller.Stop) stopping IPPool controller"
time="2024-02-05T08:29:04Z" level=info msg="(eventhandler.Run) IPPool event listener terminated"
time="2024-02-05T08:29:04Z" level=info msg="(dhcp.Stop) stopping DHCP service on nic eth1"
http: Server closed

Signed-off-by: Zespre Chang <zespre.chang@suse.com>
@starbops starbops marked this pull request as ready for review February 5, 2024 02:44
cmd/agent/run.go Outdated Show resolved Hide resolved
cmd/agent/run.go Outdated Show resolved Hide resolved
Signed-off-by: Zespre Chang <zespre.chang@suse.com>
cmd/controller/run.go Outdated Show resolved Hide resolved
cmd/agent/run.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Yu-Jack Yu-Jack left a comment

Choose a reason for hiding this comment

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

If my thought is correct, it might be a issue here, please take a look my comments in sequence.

pkg/agent/agent.go Outdated Show resolved Hide resolved
pkg/agent/ippool/event.go Outdated Show resolved Hide resolved
pkg/agent/ippool/controller.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Yu-Jack Yu-Jack left a comment

Choose a reason for hiding this comment

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

Just a NIT, I will approve it if you're not planning to change.

pkg/agent/ippool/event.go Outdated Show resolved Hide resolved
pkg/agent/ippool/event.go Outdated Show resolved Hide resolved
Signed-off-by: Zespre Chang <zespre.chang@suse.com>
Co-authored-by: Jack Yu <jack.yu@suse.com>
Copy link
Contributor

@Yu-Jack Yu-Jack left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for that.

Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@starbops starbops merged commit 88e708b into harvester:main Feb 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants