Skip to content

Conversation

@khanhtc1202
Copy link
Member

What this PR does / why we need it:

This PR is based on @Szarny works (previously at PR #3831) with changes on logging and message. Thanks @Szarny 🙌

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

CLA Assistant Lite bot All Contributors have signed the CLA.

@nghialv
Copy link
Member

nghialv commented Nov 4, 2022

@khanhtc1202 Nice work!
Actually, we have 2 approaches to deal with this issue.

    1. as the way you have implemented, the launcher also connects to the control plane to get the command.
    1. piped takes care of that Restart command as well and exits its process after receiving the command. The launcher will restart the piped by periodically watching the health status of piped process.

Please evaluate both of them and want to hear your thought about them.

@khanhtc1202
Copy link
Member Author

    1. as the way you have implemented, the launcher also connects to the control plane to get the command.
    1. piped takes care of that Restart command as well and exits its process after receiving the command. The launcher will restart the piped by periodically watching the health status of piped process.

Please evaluate both of them and want to hear your thought about them.

Thanks for the comment 🙏 Basically, the launcher existed for managing the piped activity like upgrading (and restarting as it makes in this PR), thus implementing the current way is simplest. For the second one, it's safer since the piped can stop its processes gratefully, but it may overdo since the restarting and upgrading piped are rarely triggered commands and since the upgrading is working well for now, I guess it should be the same with restarting. wdyt?

@Szarny
Copy link
Contributor

Szarny commented Nov 7, 2022

/cla sign

@knanao
Copy link
Member

knanao commented Nov 14, 2022

The way (i) seems to be simple compared to (ii) because the launcher already has handled the upgrading command.
That's why I agree with (i).

@nghialv
Copy link
Member

nghialv commented Nov 14, 2022

@khanhtc1202 Sorry for my late reply.

thus implementing the current way is simplest.

Tbh, I have a different opinion. The second one could be quite straightforward.
With that approach, we just need to add another goroutine to check the command then cancel the root context and exit in this part.
https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/cmd/piped/piped.go#L489-L497

And nothing needs to do on the launcher side since it already has the part that periodically checks sthe Piped running status to restart a new one if needed.

https://github.com/pipe-cd/pipecd/blob/master/pkg/app/launcher/cmd/launcher/launcher.go#L240-L244

@knanao Actually the Upgrade is not a command but just a field inside Piped model.

The way (i) seems to be simple compared to (ii) because the launcher already has handled the upgrading command. That's why I agree with (i).

@khanhtc1202
Copy link
Member Author

@nghialv thanks for the comment. I missed we already have .IsRunning function to check the piped availability. In such case, you're right about that, make piped self stop is more effective way. Lets me apply your suggestion 👍

@nghialv nghialv deleted the feature/3534-restart-piped-launcher branch December 2, 2022 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants