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

Allow Podman to send the container's hostname to Netavark #2254

Merged

Conversation

gtjoseph
Copy link
Contributor

@gtjoseph gtjoseph commented Nov 25, 2024

  • Added ContainerHostname to NetworkOptions. Podman will set this
    and Netavark will read it.

  • Added the container_name_as_hostname option to the
    CONTAINERS table in containers.conf. Currently, if you don't
    explicitly set a hostname when creating a container, podman will
    set it to the short ID. If this option set to true and a
    hostname isn't explicitly set, podman will use the container's
    name, with characters not in the set [0-9a-zA-Z.-] removed,
    as the hostname instead of the short ID. Set to false by default
    to preserve existing behavior.

Signed-off-by: George Joseph [email protected]

Required by containers/podman#24675
Required by containers/netavark#1130

gtjoseph added a commit to gtjoseph/podman that referenced this pull request Nov 25, 2024
@gtjoseph
Copy link
Contributor Author

The use_container_name_as_hostname addition might be a bit controversial. I have no problem with backing it out if needed.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

I am fine with a config filed for this if it useful to you. @mheon WDYT?

However if you add a field please also update the default contianers.conf file and add some basic tests, for example ead8bdd

pkg/config/config.go Outdated Show resolved Hide resolved
@mheon
Copy link
Member

mheon commented Nov 26, 2024

Added field is fine. Makes more sense to me than using the container ID honestly.
LGTM once the one comment is addressed.

@gtjoseph gtjoseph force-pushed the main-netavark-hostname branch from 31b6f9e to eb88c0e Compare November 26, 2024 17:44
@gtjoseph gtjoseph requested a review from Luap99 November 26, 2024 17:48
pkg/config/default.go Outdated Show resolved Hide resolved
@gtjoseph gtjoseph force-pushed the main-netavark-hostname branch from eb88c0e to a88971b Compare November 26, 2024 18:40
@gtjoseph gtjoseph requested a review from Luap99 November 26, 2024 18:42
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented Nov 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gtjoseph, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mheon
Copy link
Member

mheon commented Nov 26, 2024

/lgtm

* Added ContainerHostname to NetworkOptions. Podman will set this
  and Netavark will read it.

* Added the `container_name_as_hostname` option to the
  CONTAINERS table in containers.conf.  Currently, if you don't
  explicitly set a hostname when creating a container, podman will
  set it to the short ID. If this option set to `true` and a
  hostname isn't explicitly set, podman will use the container's
  name, with characters not in the set `[0-9a-zA-Z.-]` removed,
  as the hostname instead of the short ID. Set to false by default
  to preserve existing behavior.

Signed-off-by: George Joseph <[email protected]>
@gtjoseph gtjoseph force-pushed the main-netavark-hostname branch from a88971b to 2a3e1c1 Compare November 28, 2024 20:35
@openshift-ci openshift-ci bot removed the lgtm label Nov 28, 2024
gtjoseph added a commit to gtjoseph/podman that referenced this pull request Nov 28, 2024
gtjoseph added a commit to gtjoseph/podman that referenced this pull request Nov 29, 2024
@rhatdan
Copy link
Member

rhatdan commented Nov 30, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 30, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit ebbd7f3 into containers:main Nov 30, 2024
13 checks passed
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.

4 participants