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

Update config.py #439

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kajal072001
Copy link

Addressing #438
Changes

  1. Integrate Configuration Management:
    Use the enhanced PodmanConfig class to dynamically retrieve and apply network options from containers.conf.

  2. Utilize Detailed Logging:
    Configure logging to capture detailed information for debugging purposes.

  3. Validate Network Driver Support:
    Confirm that the Pasta network driver supports the -T option and handles port mappings as expected.


from podman.api import cached_property
from podman.api.path_utils import get_xdg_config_home
from podman import PodmanClient
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is giving a circular import. Please, remove it. There are ways to call the client such as podman_client = self.client

"""
try:
# Initialize Podman client
podman_client = PodmanClient(base_url="unix://run/podman/io.podman")
Copy link
Contributor

Choose a reason for hiding this comment

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

This line relates to the fix above

Copy link
Contributor

@inknos inknos left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, I'll help you getting it through. Let's start with the first comments and then I'll get back with an in depth analysis tomorrow or in the next days

Could you also sign your commit?

git commit -s --amend

Comment on lines +256 to +284
if __name__ == "__main__":
try:
# Define container parameters
network_name = "pasta" # Replace with your network name if different
port_mapping = "3128:3128" # Host port : Container port
image = "your-image" # Replace with your actual image
container_name = "your-container" # Replace with your desired container name

# Additional parameters (if any)
additional_kwargs = {
# Example: Environment variables
# "environment": {"ENV_VAR": "value"},
# Example: Volume mounts
# "volumes": {"/host/path": {"bind": "/container/path", "mode": "rw"}},
}

# Create and start the container
container = create_container_with_pasta(
network_name=network_name,
port_mapping=port_mapping,
image=image,
container_name=container_name,
**additional_kwargs
)

print(f"Container '{container.name}' is running with TCP namespace forwarding from host port {port_mapping.split(':')[0]} to container port {port_mapping.split(':')[1]}.")

except Exception as e:
print(f"Failed to create and start container: {e}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I am perplexed about the if __name__ == "__main__": block. What would be it's use case?

There are circular imports failures related to it, so I would remove it. The example looks nice though so actually it could be documented.

@inknos
Copy link
Contributor

inknos commented Oct 29, 2024

@kajal072001 any update on this pr?

@inknos
Copy link
Contributor

inknos commented Nov 12, 2024

@kajal072001 any update on my comments? :)

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.

This doesn't look right or useful to me. Containers.conf should be parsed on the server never the client.
There a few exceptions (mainly connections) but everything else should already be read by the podman service. Beside also the config file parsing here be wrong anyway.

I don't think this PR is addressing the raised issue so I don't think it makes sense

Comment on lines +187 to +192
network_config = self.attrs.get("network", {})
for network_name, config in network_config.items():
# Assuming network options are stored as 'pasta_options', 'bridge_options', etc.
options_key = f"{network_name}_options"
if options_key in network_config:
network_opts[network_name] = network_config[options_key]
Copy link
Member

Choose a reason for hiding this comment

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

besides pasta_options none of the other fields exists in containers.conf so it should not invent anything.

Comment on lines +223 to +225
# Append the port mapping using '-T' flag
network_opts += ["-T", port_mapping]
logging.debug(f"Updated network options for '{network_name}': {network_opts}")
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong, -T maps container ports to the host and you allow only one string. Overall I don't follow this at all.

return network_opts


def create_container_with_pasta(network_name: str, port_mapping: str, image: str, container_name: str, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

There is nothing special about pasta API wise, all the networks settings should be able to be set already via the normal API.

Copy link
Contributor

openshift-ci bot commented Dec 13, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kajal072001
Once this PR has been reviewed and has the lgtm label, please assign jwhonce for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

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.

3 participants