Handle the disapperance of net-tools #1167
Conversation
+ Use ip route command when the route command is not available
|
@boumenot - any objections? |
|
I think I missed something, still investigating but I am missing a couple of routes that get set. SO there may be an addition or two, hold off with the merge System comes up and one can login, but the provisioning state never changes :( |
|
@hglkrijger @boumenot do you know where/how the route to the wireserver end point is supposed to get set? I am missing the following entries in my routing table: 168.63.129.16 via 172.16.7.1 dev eth0 proto dhcp I set logging to verbose in the hopes to catch a command being executed to add these routes but I didn't so I am at a bit of a loss ATM Thanks |
|
The route to wireserver is not usually set by the agent, but provided via dhcp in one of the options. For VMs which are not in a vnet (the uncommon case) the agent issues a dhcp request and parses the response, thereby setting the route itself. |
|
@hglkrijger thanks |
|
OK, this is an issue with the DHCP code and not with may changes. This is good to go from my side. |
| @@ -901,7 +901,10 @@ def get_endpoint_from_leases_path(pathglob): | |||
| return endpoint | |||
|
|
|||
| def is_missing_default_route(self): | |||
| routes = shellutil.run_get_output("route -n")[1] | |||
| route_cmd = "route -n" | |||
| if not shellutil.has_command("route"): | |||
There was a problem hiding this comment.
This is not the right way to achieve this. The implementations in osutil/default.py should remain the same, since they work for most distros. You should add an sles.py file to this module which overrides these methods to use the "ip route" command.
There was a problem hiding this comment.
It is inevitable that other distributions also drop net-tools. Creating distribution specific implementations as suggested implies that we'd end up with one obsolete default implementation and number_of_distros.py files, from my perspective that is not great especially since the implementation in number_of_distros.py is the same.
There are other means of doing this kind of indirection, but creating a file for each distro is IMHO not the way to go.
There was a problem hiding this comment.
But that's the way the agent is architected. Distro-specific differences are abstracted. When the majority moves away from net-tools, the default implementation would be changed to reflect that.
There was a problem hiding this comment.
I would like to centralize this behavior too. How about a networkutil.py in the same vein as textutil.py. There are three commands to add: add, del, and show. The actions and probing for the command can happen in this class. In the future, we can revisit this case and have to refactor one location, and any platform specific code will be contained.
I would add networkutil.py under azurelinuxagent/common/utils.
There was a problem hiding this comment.
I can live with this, especially if you probe only once (when the first add/del/show is done) and rely on that decision for the remaining life of the process. A "factory" method that computes, caches, and returns the correct class instance (net-utils or "ip route") seems the most logical approach.
| """ | ||
| Return True if the given command is on the path | ||
| """ | ||
| return not run(cmd, False) |
There was a problem hiding this comment.
Pretty expensive way to determine this, especially since the correct answer can be known a priori based on distro type and version.
There was a problem hiding this comment.
but the design here is a bit different, the distro itself is not abstracted, meaning I cannot ask distro.what_is_your_route_command. The abstraction chose in the agent implementation is distro_name.py whereever specialization may be needed and that gets us back to what I stated above, an obsolete implementation that uses "route" and lost of identical files that use "ip route"
There was a problem hiding this comment.
When the numbers show that most distros use "ip route", we'll change the default to use that.
If you had to do runtime discovery of the presence of a command, you'd want to persist that information so that subsequent invocations of "do this routing thing" wouldn't be probing the system over and over again. (But I don't think you have to do this, for this purpose, at this time.)
There was a problem hiding this comment.
When the numbers show that most distros use "ip route", we'll change the default to use that.
AFAIK all our endorsed distro images and versions have iproute2 utilities installed already, please correct me if I'm wrong. Most might also have the obsolete route/ifconfig/etc. tools as well, but that's no reason to keep them as default forever.
|
Timing this out due to lack of consensus. |
|
Sorry I've been very busy and this kind of slipped through the cracks. I would like to find a way forward so I do not have to carry a patch in the SUSE package forever more. SLES 15 simply does not have the old tools and this patch is a must have. W.r.t. @szarkos comment, yes all distros that are active on Azure should have the iproute tools. I do not know however if BSD also has iproute tools or if BSD sticks to the old tools. If BSD also has the iproute tools then the patch should be simplified and we should just remove the old path. |
|
@rjschwei no worries. BSD is not officially endorsed, and part of what that means is we have no point of contact other than the community to determine whether changes we make to the agent are acceptable. I would suggest we take the simplest path forward for endorsed distros, and ask previous BSD contributors to take a look at it. If you are willing to reopen with the appropriate changes, I can test on all endorsed distros, and loop in some previous BSD committers. |
|
I dropped the conditions and switched over to using the "ip route" commands as the default and only option for route operations. |
|
I decided to retain the "has_command" function as I think it is useful, although no longer used in the updated implementation. |
|
I do not see any new commits..? Feel free to open a new PR if you need to, or re-open this one. |
|
OK, looks like the changes were not picked up because this request is closed. I do not have the rights to re-open, I think. Very superficial search led me to [1] which implies one has to have push access to the project with the PR. [1] https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c |
|
Superseded by #1290 |
The "route" command is obsolete and longer present in SLES 15, switch to using "ip route" command when the "route" command cannot be found.