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

[release 2.54] [BUGFIX] Docker SD: fix the issue of failing to match the first network when the container is reconnected to a new network #14654

Merged

Conversation

DrAuYueng
Copy link
Contributor

In the scenario where the container is connected to multiple networks, if the container is reconnected to a new network, the network mode will not change which will causes the issue of failing to match the first network.

Follows #10490.
Fixes #14647.

@DrAuYueng
Copy link
Contributor Author

cc @bboreham

@bboreham
Copy link
Member

Should this be against release-2.54 branch?

// 3. Reconnect network with user defined networks.
var first string
firstNetworkMode = string(containerNetworkMode)
firstNetwork = networks[firstNetworkMode]
Copy link
Contributor Author

@DrAuYueng DrAuYueng Aug 13, 2024

Choose a reason for hiding this comment

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

I'm thinking about whether this step is still necessary because the next step sorts all non-nil networks in ascending order and gets the first one from the sorted list of networks, which I think is more consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone wants to use a specific network, they can use the following configuration:

scrape_configs:
  - job_name: 'docker-containers'
    docker_sd_configs:
      - host: unix:///var/run/docker.sock
        match_first_network: false
    relabel_configs:
      # Select containers whose name is prefixed by `exporter__`.
      - source_labels: [__meta_docker_container_name]
        regex: /exporter__.  # For some reason, container names begin with a '/' character.
        action: keep
      # Create new label named 'container' with the value from the meta container name label.
      - source_labels: [__meta_docker_container_name]
        target_label: container
        regex: /(. )
      - source_labels: [__meta_docker_network_name]
        regex: mysql_net1
        action: keep

@DrAuYueng
Copy link
Contributor Author

Should this be against release-2.54 branch?

Yep.

@DrAuYueng
Copy link
Contributor Author

@bboreham Ready for review.

Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Is it worth adding an additional check that the network is non-nil, at the point where it crashed in #14647 ?

(Note I rebased and force-pushed your branch onto release-2.54 branch)

@DrAuYueng
Copy link
Contributor Author

Is it worth adding an additional check that the network is non-nil, at the point where it crashed in #14647 ?

Agree, make sense.

@DrAuYueng DrAuYueng force-pushed the docker-network-mode-match branch 6 times, most recently from 0d2c7e9 to c23f097 Compare August 14, 2024 16:06
@DrAuYueng
Copy link
Contributor Author

Updated.

@DrAuYueng DrAuYueng changed the title Fix the issue of failing to match the first network when the container is reconnected to a new network [release 2.54] [BUGFIX] Docker SD: fix the issue of failing to match the first network when the container is reconnected to a new network Aug 18, 2024
Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Code seems ok (though I am not a Docker expert).

Please amend the documentation in docs/configuration/configuration.md to describe the new behaviour. (or help me understand why this is unnecessary)

@DrAuYueng
Copy link
Contributor Author

Please amend the documentation in docs/configuration/configuration.md to describe the new behaviour. (or help me understand why this is unnecessary)

Done.

Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Thanks!

@bboreham bboreham merged commit 2eb24bf into prometheus:release-2.54 Aug 19, 2024
30 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.

Crash "panic: runtime error: invalid memory address or nil pointer dereference"
2 participants