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

cmd/containerboot: switch to IPN bus monitoring instead of polling. #6658

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

danderson
Copy link
Member

We still have to shell out to tailscale up because the container image's API includes "arbitrary flags to tailscale up", unfortunately. But this should still speed up startup a little, and also enables k8s-bound containers to update their device information as new netmap updates come in.

Fixes #6657

Signed-off-by: David Anderson danderson@tailscale.com

@danderson danderson requested a review from maisem December 7, 2022 20:38
@danderson
Copy link
Member Author

CIFuzz failure is some runtime error in the fuzz infrastructure, not in the change.

cmd/containerboot/main.go Show resolved Hide resolved
case ipn.NeedsMachineAuth:
log.Printf("machine authorization required, please visit the admin panel")
case ipn.Running:
// Technically, all we want is to keep monitoring the bus for
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it instead make sense to run this in a goroutine and have that push the relevant notifications to the main goroutine? That way you can keep the cancel the watcher goroutine if something fails or times out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly. I was trying to avoid channels and goroutines, because it's already quite tricky to sequence things when it's just ~1 linear state, and as long as the IPN watch is active you have to consume the updates without blocking. Splitting the logic would force me to reimplement a chunk of ipn/ipnlocal to coalesce changes and track overall state.

I'll give it a try in a followup change, getting this one to pass tests was tricky enough that I don't want to lose it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, thanks!

We still have to shell out to `tailscale up` because the container image's
API includes "arbitrary flags to tailscale up", unfortunately. But this
should still speed up startup a little, and also enables k8s-bound containers
to update their device information as new netmap updates come in.

Fixes #6657

Signed-off-by: David Anderson <danderson@tailscale.com>
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.

Teach cmd/containerboot to record node FQDN in kubernetes.
2 participants