Skip to content

Make unix dgram metrics sink connectionless #1702

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

Merged
merged 7 commits into from
Jun 8, 2021

Conversation

tobim
Copy link
Member

@tobim tobim commented Jun 1, 2021

The previous way of setting up the UDS datagram metrics relied on a fake connection that made it possible to use the iostream abstraction. However this fake connection would get destroyed if the listening end deleted it's socket.

This commit introduces a wrapper around sockaddr_un and the client fd and uses sendto instead of write.

📔 Description

📝 Checklist

  • All user-facing changes have changelog entries.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

Code review by file.

Test by running VAST with

vast:
  enable-metrics: true
  metrics:
    uds-sink:
      enable: true
      path: "/tmp/vast-metrics.sock"
      type: "datagram"

And run and restart socat UNIX-RECV:/tmp/vast-metrics.sock while the VAST node is running.

@tobim tobim added the bug Incorrect behavior label Jun 1, 2021
@tobim tobim requested a review from a team June 1, 2021 14:06
tobim added 2 commits June 1, 2021 16:19
The previous way of setting up the UDS datagram metrics relied on
a fake connection that made it possible to use the iostream
abstraction. However this fake connection would get destroyed if
the listening end deleted it's socket.

This commit introduces a wrapper around sockaddr_un and the client
fd and uses sendto instead of write.
@tobim tobim force-pushed the story/ch24267/reconnect-uds-dgram-metrics branch from 49d8957 to 87649a3 Compare June 1, 2021 14:19
tobim and others added 4 commits June 4, 2021 20:49
Co-authored-by: Dominik Lohmann <mail@dominiklohmann.de>
- Make it explicit that it is a move only structure
- Remove the socket from the filesystem immediately after bind, it
  isn't required any more.
- Create the filesystem path for the sending socket in a temporary
  directory instead of next to the destination.
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

This seems to work fine in a really short test, and the code looks good as well now. Just had one small question.

@tobim tobim enabled auto-merge June 8, 2021 08:14
@tobim tobim merged commit a74a56c into master Jun 8, 2021
@tobim tobim deleted the story/ch24267/reconnect-uds-dgram-metrics branch June 8, 2021 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants