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

Ephemeral Networking for FreeBSD #2165

Merged
merged 5 commits into from
Jun 12, 2023
Merged

Conversation

igalic
Copy link
Collaborator

@igalic igalic commented May 3, 2023

Proposed Commit Message

implement ephemeral networking for BSD

After #2127 lay the foundation, we now implement the BSD side of this

Sponsored by: The FreeBSD Foundation

Additional Context

This is based on #2127

Test Steps

I have tested this successfully on libvirt with NoCloud (standard dhcp), on Hetzner (EphemeralDHCPv4) and with config-drive with a static network setup (regards to @goneri )

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@@ -118,6 +120,7 @@ def __init__(self, name, cfg, paths):
self.name = name
self.networking: Networking = self.networking_cls()
self.dhcp_client_priority = [dhcp.IscDhclient, dhcp.Dhcpcd]
self.net_ops = iproute2.Iproute2
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intended to be configurable? defined as class variable and instance variable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's a question for @holmanb

Copy link
Member

Choose a reason for hiding this comment

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

is this intended to be configurable? defined as class variable and instance variable

This is not intended to be configurable at runtime. This is defined as an instance variable to avoid unpickle issues across the upgrade path. It might not be required as a class variable, I don't recall why it is defined both ways.

"-sf",
"/bin/true",
] + (["-cf", config_file, interface] if config_file else [interface])

Copy link
Contributor

Choose a reason for hiding this comment

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

just a personal preference, but for readability, I think I'd rather see the args built up over a few lines and conditionals than trying to get it all into one statement. e.g.

args = [
            path,
            "-1",
            "-v",
            "-lf",
            lease_file,
            "-pf",
            pid_file,
            "-sf",
            "/bin/true",
        ]

if config_file:
    args.extend(["-cf", config_file, interface])

args.append(interface)

I may be alone in this preference, so feel free to ignore :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i like the idea!
but this, too is @holmanb's code.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with that.

Copy link
Collaborator Author

@igalic igalic left a comment

Choose a reason for hiding this comment

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

guide posts for reviewers

if gateway and gateway != "0.0.0.0":
subp.subp(
["route", "change", route, gateway],
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't support source selection

and i had to make this addition in two steps, because when i did it in a single step, the result was nonsense.

cloudinit/net/netops/bsd_netops.py Outdated Show resolved Hide resolved
@github-actions
Copy link

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label May 19, 2023
@holmanb holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label May 22, 2023
@igalic igalic marked this pull request as ready for review June 1, 2023 14:05
@igalic

This comment was marked as resolved.

@igalic igalic requested a review from holmanb June 9, 2023 02:18
Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@holmanb holmanb merged commit 06a1475 into canonical:main Jun 12, 2023
26 checks passed
@igalic igalic deleted the bsd/ephemeral branch June 12, 2023 17:48
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.

None yet

4 participants