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 typo in cmd for reloading bird6 #298

Merged
merged 1 commit into from Dec 2, 2019

Conversation

@neiljerram
Copy link
Member

neiljerram commented Dec 2, 2019

Fixes #286

Fix typo resulting in bird6 never reloading
@neiljerram

This comment has been minimized.

Copy link
Member Author

neiljerram commented Dec 2, 2019

@tmjd I've been mulling about various possible ways that I could test this, but I can't think of anything simple enough to be worth it.

For example, I could make the current test run executables (in the same test container environment) named bird and bird6, and somehow arrange for them to record what signals they receive - perhaps by running something like strace -o strace.txt nohup sleep 3600. But it seems anything as complex as that will cause us trouble down the line, which probably is not justifiable for this typo fix.

WDYT?

@tmjd

This comment has been minimized.

Copy link
Member

tmjd commented Dec 2, 2019

Without this fix the bird6 never would have been restarted with config changes, could you test with the current release and making a change (that should cause bird6 to restart) does not restart it but then with the new version that the same kind of change does cause the process to restart?
From a quick google, it looks like a command like ps -eo pid,lstart,cmd should show the start time.

@tmjd

This comment has been minimized.

Copy link
Member

tmjd commented Dec 2, 2019

I guess if you're automating that it should be enough that checking the start time of the bird6 process would be enough to see that it was restarted. Get start time, make a change, check that start time is different.

@neiljerram

This comment has been minimized.

Copy link
Member Author

neiljerram commented Dec 2, 2019

@tmjd Yes, bird6 would have been restarted (or, in fact, reconfigured in response to SIGHUP) because confd generates 3 config files for bird6, and 2 of those have the correct reload_cmd = pkill -HUP bird6. The bug here would only be observable, transiently, for a change in a specific area of the v1 data model (corresponding to IPv6 IP blocks), and would be cleared by changes in other parts of the data model.

@neiljerram

This comment has been minimized.

Copy link
Member Author

neiljerram commented Dec 2, 2019

About looking at the bird6 start time: the problem is that the existing confd tests don't even run bird at all; they simply check the generated config files. So there is no process whose start time we can check.

Also, I'm pretty sure that the SIGHUP does not cause bird to restart. I believe bird handles that signal as an instruction to reread its config.

@tmjd
tmjd approved these changes Dec 2, 2019
Copy link
Member

tmjd left a comment

LGTM

@neiljerram neiljerram merged commit 8cd7827 into projectcalico:master Dec 2, 2019
2 checks passed
2 checks passed
license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details
@neiljerram neiljerram deleted the neiljerram:bird6-reload branch Dec 2, 2019
@lmm lmm added this to the Calico v3.11.0 milestone Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.