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

multi dhcp client #253

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

sohorx
Copy link
Contributor

@sohorx sohorx commented Feb 27, 2023

Add multi dhcp client support and busybox udhcpc as one available client.

isc-dhcp-client is not always a good choice in term of client capability. In my use case, I work on qmi devices that expose raw ip interfaces which is currently unsupported. (https://gitlab.isc.org/isc-projects/dhcp/-/merge_requests/66)

We do not need to change the dhcp client, dhclient is fine most of the time and luckily, the code base is more than capable of handling two clients and not much work is needed.

@julienfortin
Copy link
Contributor

Changes looks good, I'll do some more testing before merging

@julienfortin
Copy link
Contributor

I can't share the test case suite but there's an issue. All my test are good without the patch. After applying the patch one test fail. I'm not sure if it's relevant but, it fails when trying to remove a gateway.

I don't really have time to debug at the moment, feel free to have another look. I'll be traveling for the next few weeks without my laptop. I'll try to merge it once i'm back

@sohorx
Copy link
Contributor Author

sohorx commented Mar 1, 2023

I did find something in the address addon trying to release dhclient before assigning an ip. It might be related (or it might not :/)

I'm going to change some part for releasing a udhcpc client as well.

@sohorx
Copy link
Contributor Author

sohorx commented Mar 1, 2023

Also, I noticed the moduleBase.merge_modinfo_with_policy_files throwing up warnings on stderr for all policy configured not mentionned in the new dhcp._modinfo.

an exemple with dhcp-wait to 'no'.

# ifup ens4
warning: dhcp: dhcp-wait: this attribute doesn't exist or isn't supported

Should we just fill up the modinfo ?

@julienfortin
Copy link
Contributor

Thanks for the update.

Should we just fill up the modinfo ?

Correct, now that we have a modinfo in this addon we should probably have all attributes mentioned otherwise that will cause some other issues

@julienfortin
Copy link
Contributor

Actually dhcp-wait is a policy not an addon attribute

            dhcp_wait = policymanager.policymanager_api.get_attr_default(
                module_name=self.__class__.__name__, attr='dhcp-wait')
            wait = str(dhcp_wait).lower() != "no"

@sohorx
Copy link
Contributor Author

sohorx commented Mar 7, 2023

Yes this is bothering, making a modinfo's description for a policy would lead to make the user think it's an attribute.

I see two simple way to solve this:

  • filling the modinfo's attr with a desc mentionning of being a policy only.
  • Making a list(or dict) of policy keys in the addon that the merge_modinfo function could filter on.

I'm going to make a commit for a policy list. Do tell me if you think of a better way or if I'm missing something.

@sohorx
Copy link
Contributor Author

sohorx commented Mar 7, 2023

First, sorry for the close/reopen of the PR (missclick got me).

I did check this merge_modinfo warning on the address addon, here a log:

# ifdown ens4
warning: address: ipv6_dad_handling_enabled: this attribute doesn't exist or isn't supported
warning: dhcp: dhcp-wait: this attribute doesn't exist or isn't supported

I don't know how I missed that while implementing the dad handling.

@sohorx sohorx closed this Mar 7, 2023
@sohorx sohorx reopened this Mar 7, 2023
@sohorx
Copy link
Contributor Author

sohorx commented Mar 7, 2023

With the last two commits, dhcp policies won't warn the user of unmergeable attributes.

Thoses commits won't fix the other addons since it's not the goal of this PR (a new one should be made).

We can also improve the _policies class variable later on for ifquery if we want to.

@sohorx
Copy link
Contributor Author

sohorx commented Mar 24, 2023

Hi fellow alumnus ;) does the PR pass the test case suite ?

@julienfortin
Copy link
Contributor

Hi @sohorx, I just got back from PTO. I will give this a go once i'm all caught up on emails and work :)

@julienfortin
Copy link
Contributor

I'm running tests on this today, i'll give an update soon

@sohorx
Copy link
Contributor Author

sohorx commented Sep 7, 2023

Hi @julienfortin, any news on the tests results ?

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.

2 participants