IBM PowerVM specific RMC module #584
Conversation
cloudinit/config/cc_reset_rmc.py
Outdated
| frequency = PER_INSTANCE | ||
| PIDOF = 'pidof' | ||
| RMCCTRL = '/usr/sbin/rsct/bin/rmcctrl' | ||
| RECFGCT = '/usr/sbin/rsct/install/bin/recfgct' |
There was a problem hiding this comment.
This is just odd.
Why are these executables in these paths?
cloud-init should not have to care about the path to an executable. It should be up to the system configuration to set the PATH correctly, such that cloud-init can just call 'recfgct'.
There was a problem hiding this comment.
@smoser , these executable are RMC commands (please read the module description at the beginning of the file to understand more about RMC).
More information about the specific commands can be found at
https://www.ibm.com/support/knowledgecenter/ssw_aix_72/r_commands/rmcctrl.html
https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/r_commands/recfgct.html
Basically these commands are used to manage and reconfigure the Reliable Scalable Cluster Technology (RSCT) subsystems. These subsystems / services are required for communication of the PowerVM hypervisor to the VM.
There was a problem hiding this comment.
@smoser on using absolute path vs using just the executable name without the path, from a security standpoint the former is preferred. There are numerous vulnerabilities that can potentially happen by exploiting the path, where a hacker could introduce an executable with the same name in another directory(or path) and add that location to the PATH variable and give that precedence. In such cases, the program would end up executing the executable injected by the hacker rather than the executable from the correct path. To rule out any such cases of exploitation, using the absolute path of the executable is safer.
There was a problem hiding this comment.
@smoser on using absolute path vs using just the executable name without the path, from a security standpoint the former is preferred. There are numerous vulnerabilities that can potentially happen by exploiting the path
I've never bought this argument. If an attacker can influence a root processes' environment, then they've already become root, or a severe exploit has been found. There are any number of really bad things that can happen at this point (LD_LIBRARY_PATH?). Arguing that cloud-init should use full paths for some things, does not really improve the situation. cloud-init's policy (and that of the vast majority of programs) is to not use full paths, but rather allow the system administrator control over where things are installed.
The real reason that you have hard coded paths here is that /usr/sbin/rsct/bin is never going be in anyone's PATH. For what its worth, the documentation you've pointed at suggests those commands should be in /opt/rsct/bin/rmcctrl
I'll not stick on this... but the fact that your installation differs from the documented installation is the exact reason that PATH should be used.
| continue | ||
| interface = list(info.keys())[info_count] | ||
| if not info[str(list(info.keys())[info_count])]['ipv4']: | ||
| disable_NM_ipv6(interface) |
There was a problem hiding this comment.
According to the description on the header, you expect the VM to have a single IPv6 interface and that's intended to be used to talk to the RMC. Is there a corner case that the VM needs to have more than one IPv6 interface, for example? Can you pinpoint which interface the VM uses to talk to RMC?
There was a problem hiding this comment.
@otubo , the hypervisor specifically uses ipv6 link local address / interface for RMC communication from the VM to the hypervisor . The expectation here is that when the VM is deployed, the VM is deployed with a ipv6 link local address that is used for this communication. In a case the VM has more than one link local ipv6 address, the hypervisor has the option to use any of these for communication.
https://www.ibm.com/support/knowledgecenter/POWER8/p8eig/p8eig_update_rmc.htm
Resource Monitoring Control (RMC) connections between PowerVM® NovaLink and each logical partition are routed through a dedicated internal virtual network. For easier configuration of the internal virtual network, assign an IPv6 link-local address to the RMC virtual network interfaces. To assign IPv6 link-local addresses to the RMC virtual network interfaces on AIX® and Linux logical partitions, the Reliable Scalable Cluster Technology (RSCT) packages must be at version 3.2.1.0 or later.
@otubo @smoser , is unit tests a mandatory requirement to contribute these modules ? While we found UTs for some modules at https://github.com/canonical/cloud-init/tree/master/cloudinit/config/tests and for some other modules at https://github.com/canonical/cloud-init/tree/master/tests/unittests/test_handler, we found some modules don't have unit tests associated with them. |
I'm not a maintainer, perhaps others could confirm, but my guess is that new code should always be backed by unit tests, according to the doc: https://cloudinit.readthedocs.io/en/latest/topics/code_review.html#prerequisites-for-landing-pull-requests |
|
Another general nitpick is to avoid merge commits (not sure how they ended up in the pull request). This makes it hard to squash/cherry-pick on a local branch for testing. Just push the changes directly to your branch aman306/RMC_modules and the PR will be updated automatically. |
cloudinit/config/cc_reset_rmc.py
Outdated
|
|
||
| frequency = PER_INSTANCE | ||
| PIDOF = 'pidof' | ||
| RMCCTRL = '/usr/sbin/rsct/bin/rmcctrl' |
There was a problem hiding this comment.
@Aman306 , we should use the new path namely /opt/rsct/bin . /usr/sbin/rsct/bin/ is the old path and now a symbolic link to /opt/rsct/bin. As pointed out in a previous comment, adding /opt/rsct/bin to PATH variable also should work fine. I'd recommend adding a comment above, mentioning the full path as /opt/rsct/bin and that the user has to set this under the PATH variable and use rmcctrl instead of the full path.
https://www.ibm.com/support/knowledgecenter/SGVKBA_3.2/admin/admin_pdf.pdf
The rmcctrl command is located in /opt/rsct/bin. Add this directory to your PATH, or specify the
full path on the command line
| SED = 'sed' | ||
|
|
||
|
|
||
| def handle(name, _cfg, _cloud, log, _args): |
There was a problem hiding this comment.
since you're not using log here (which is fine..), rename this to _log to make it obvious.
| # interface and NetworkManager is restarted. | ||
| interface_file = '/etc/sysconfig/network-scripts/ifcfg-' + interface | ||
| with open(interface_file) as f: | ||
| if 'IPV6ADDR' in f.read(): |
There was a problem hiding this comment.
try:
contents = util.load_file(interface_file)
except IOError as e:
if e.errno == ENOENT:
# feel free to just raise this if it is fatal to your application.
LOG.debug("interface file %s did not exist\n", interface_file)
else:
raise e
if 'IPV6ADDR' not in contents:
# maybe this is fatal?
LOG.debug("interface file %s did not have IPV6ADDR", interface_file)
return
LOG.debug("editing file %s\n", interface_file)
# you can put all these commands in a single subp.
subp.subp(
SED, '-i',
's/NM_CONTROLLED=yes/NM_CONTROLLED=no/g,
'/^IPV6INIT/d',
...
)
subp.subp(['systemctl', 'restart', 'NetworkManager'])with open(interface_file) as f:
content = f.
| subp.subp([SED, '-i', '/^IPV6INIT/d', interface_file]) | ||
| subp.subp([SED, '-i', '/^IPV6ADDR/d', interface_file]) | ||
| subp.subp([SED, '-i', '/^IPADDR6/d', interface_file]) | ||
| subp.subp(['systemctl', 'restart', 'NetworkManager']) |
There was a problem hiding this comment.
It would be easier for you to write unit tests on this method if it were in python and not sed. That'd also give you the ability to detect easier if you actually changed anything.
I realize you're probably more comfortable with the SED, but its pretty easy to open a file, read its contents, and then iterate over lines in python.
You can then easily make unit tests for different contents and see that you get the expected results.
cloudinit/config/cc_reset_rmc.py
Outdated
| frequency = PER_INSTANCE | ||
| PIDOF = 'pidof' | ||
|
|
||
| # Ensure that /opt/rsct/bin and /opt/rsct/install/bin has been |
There was a problem hiding this comment.
"Ensure that" isn't clear that you expect the system admin or user to take action, rather than having cloud-init ensure it.
Try:
RMCCTRL and RECFGCT are expected to be in system PATH (/opt/rsct/bin and /opt/rsct/install/bin)
| return cloud.Cloud(ds, paths, {}, distros, None) | ||
|
|
||
| def ipv6_interface(self): | ||
| info = netinfo.netdev_info() |
There was a problem hiding this comment.
It feels like these tests probably wont run on all systems. You shuold mock netinfo.netdev_netinfo() with specific results that you want to test.
And i dont think there are actually any tests here, are there?
the 'handle' interface is a real pain to test. thats why we've mostly moved to making them very small that just call out to other functions and writing those other functions to be more easiliy testable.
|
|
||
| def firewall_status_check(self): | ||
| firewall_status = 'systemctl is-active --quiet firewalld' | ||
| restart_status = os.system(firewall_status) |
There was a problem hiding this comment.
this wont work everywhere, and we don't want to depend on state of the host system for the tests.
tests/unittests/test_handler/test_handler_refresh_rmc_and_interface.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@Aman306 ,
thank you for your patience. this is really looking so much better than it started.
I think it woudl be good to get some tests done that actually run on a developer system. The way we do that is by mocking out system specific information, and a bit of re-factor of functions to make that more unit-testable.
some more comments inline... again, thanks for your patience, we are getting there.
cloudinit/config/cc_reset_rmc.py
Outdated
| LOG.debug('Skipping creation of new ct_node_id node') | ||
| return | ||
|
|
||
| orig_path = add_path() |
There was a problem hiding this comment.
if this function returns for any reason (such as exception on line 77) it will leave the PATH polluted.
a try: ... finally will make sure that you re-set the path to the original.
Also, currently add_path will raise exception on any platform without the RSCT_PATH, that will get turned into a cloud-init failure if this module is enable. I think we would prefer a simple return with a debug message "no RSCT_PATH present, module not enabled".
There was a problem hiding this comment.
I agree with first path of the comment where PATH can get polluted
In order to fix this
orig_path = os.environ.get('PATH')
add_path(orig_path)
def add_path(orig_path):
# Adding the RSCT_PATH to env standard path
# So thet cloud init automatically find and
# run RECFGCT to create new node_id.
if not os.path.isdir(RSCT_PATH):
LOG.error("RSCT package is missing!")
return
try:
suff = ":" + orig_path if orig_path else ""
os.environ['PATH'] = RSCT_PATH + suff
except Exception:
util.logexc(LOG, 'Failed to add RSCT path')
raise
On the second part of the comment:
There is a pre-requisite to use this module mentioned at line 30.
So to use these module, user have to install RSCT package.
There was a problem hiding this comment.
I agree with first path of the comment where PATH can get polluted
In order to fix thisorig_path = os.environ.get('PATH') add_path(orig_path)
I don't think i'm being clear. The problem is that os.environ is a global. and if you return from this function (handle) after changing os.environ.['PATH'] without setting it back, the rest of cloud-init's execution will have the modified value. Such a return includes explicit return or exception.
See my suggested 'try...finally' to ensure that we never return without setting it back.
On the second part of the comment:
There is a pre-requisite to use this module mentioned at line 30.
So to use these module, user have to install RSCT package.
All (unconfirmed, but definitely the vast majority) config modules in tree are executed. If there is nothing for them to do, they just return. The user should not really have to configure the list of modules.
@smoser I am not very clear with your above comment. It will be great if you can rephrase/ elaborate a bit.
|
cloudinit/config/cc_reset_rmc.py
Outdated
| from cloudinit import subp | ||
|
|
||
| frequency = PER_INSTANCE | ||
| PIDOF = 'pidof' |
There was a problem hiding this comment.
this isn't used as far as i can see.
| return | ||
|
|
||
| orig_path = os.environ.get('PATH') | ||
| try: |
There was a problem hiding this comment.
the reason that i suggested this code block (lines 72-95) be moved to a function is so that it is easier to unit test and also to read.
-
to unit test: then you can just call that function... you don't need the difficult setup required to call handle(). As I've said before, the pain in calling handle is my fault for designing it that way. sorry 😢 . To avoid the pain, the general solution is just to minimize what handle actually does.
-
to read: you can look at a code block like this and say "oh, all that does is call a function with a modified PATH and then set it back". It also reduces indentation by one level.
try:
add_path(orig_path)
call_your_function()
finally:
if orig_path:
os.environ['PATH'] = orig_path
else:
del os.environ['PATH']if we had a context handler, it reads even better, but writing one for one
caller doesn't seem necessary. Then it might look like this:
with prepended_path(RSCT_PATH):
call_your_function()There was a problem hiding this comment.
I think I get your point.
Using handle in UT it is vary hard to handle datasource in UTs. This is much easier in running UT.
There was a problem hiding this comment.
If you're interested in using it, I was interested in trying, and wrote 'prepended_path' context manager.
import contextlib
import os
@contextlib.contextmanager
def prepended_path(path):
enter_path = os.environ.get("PATH")
new_path = path + ((":" + enter_path) if enter_path else "")
try:
os.environ["PATH"] = new_path
yield new_path
finally:
assert os.environ.get("PATH") == new_path, (
"PATH changed during prepended_path context "
"old=%s new=%s" % (enter_path, new_path))
if enter_path is None:
del os.environ["PATH"]
else:
os.environ["PATH"] = enter_path
print("before: %s" % os.environ.get("PATH"))
with prepended_path("/mybin"):
print("during: %s" % os.environ.get("PATH"))
print("after: %s" % os.environ.get("PATH"))So you could use this just as:
with prepended_path(RSCT_PATH):
reset_rmc()I think that reads really nicely.
|
|
||
|
|
||
| class TestRsctNodeFile(t_help.FilesystemMockingTestCase): | ||
| def setUp(self): |
There was a problem hiding this comment.
i think you can drop this setUp now, as you're not (i think) using the tmpdir that you create.
|
|
||
| class TestRsctNodeFile(t_help.FilesystemMockingTestCase): | ||
|
|
||
| def setUp(self): |
There was a problem hiding this comment.
i think you can drop this setUp now, as you're not (i think) using the tmpdir that you create.
| self.assertNotEqual(node_id_before, node_id_after) | ||
|
|
||
| def test_path(self): | ||
| self.allowed_subp = True |
There was a problem hiding this comment.
don't need allowed_subp here.
| def test_path(self): | ||
| self.allowed_subp = True | ||
| orig_path = os.environ.get('PATH') | ||
| x = cc_reset_rmc.add_path(orig_path) |
There was a problem hiding this comment.
you can actually mock the os.environ dictionary. that way your test case does not permenantly change the environment (as it does right now).
you can see that in cloudinit/tests/test_dhclient_hook.py or cloudinit/tests/test_subp.py.
cloudinit/config/cc_reset_rmc.py
Outdated
| # Adding the RSCT_PATH to env standard path | ||
| # So thet cloud init automatically find and | ||
| # run RECFGCT to create new node_id. | ||
| try: |
There was a problem hiding this comment.
there is no reason to try/catch here, you're letting it re-raise, and nothing should actually trigger an exception.
|
@smoser I am seeing the UTs are failing here. But when I am running on my local system the UTs are actually passing. Can you guide me on that. |
|
@Aman306 , Try this out, it might give you some ideas on how you can make your code more testable, and gives some decent coverage on the happy path. |
| self.assertNotEqual(node_id_before, node_id_after) | ||
|
|
||
| @mock.patch('cloudinit.config.cc_reset_rmc.add_path') | ||
| def test_path(self, add_path): |
There was a problem hiding this comment.
this test doesn't really do anything as it is written.
You're mocking 'add_path', so you actually don't call any functions during this test method.
I guess just drop this test.
| @mock.patch( | ||
| 'cloudinit.config.cc_reset_rmc.rmcctrl', | ||
| side_effect=rmcctrl) | ||
| def test_rsct_node(self, reconfigure_rsct_subsystems, rmcctrl): |
There was a problem hiding this comment.
This test is somewhat fine for a "happy path", you mock out 'reconfigure_rsct_subsystems', which you've asserted will modify the contents of NODE_FILE. and then you check that the NODE_FILE has been updated (which it will have been unless there is a bug in your recfgct() function above.
So really what this test should be called is 'test_reset_rmc_does_not_raise_exc_if_node_file_updated'.
More interesting is probably to test the unhappy path, that if reconfigure_rsct_subsystems does not update NODE_FILE, that you reset_rmc will raise an exception.
So I suggest adding a test: test_reset_rmc_does_raises_unless_node_file_updated.
| return | ||
|
|
||
| orig_path = os.environ.get('PATH') | ||
| try: |
There was a problem hiding this comment.
If you're interested in using it, I was interested in trying, and wrote 'prepended_path' context manager.
import contextlib
import os
@contextlib.contextmanager
def prepended_path(path):
enter_path = os.environ.get("PATH")
new_path = path + ((":" + enter_path) if enter_path else "")
try:
os.environ["PATH"] = new_path
yield new_path
finally:
assert os.environ.get("PATH") == new_path, (
"PATH changed during prepended_path context "
"old=%s new=%s" % (enter_path, new_path))
if enter_path is None:
del os.environ["PATH"]
else:
os.environ["PATH"] = enter_path
print("before: %s" % os.environ.get("PATH"))
with prepended_path("/mybin"):
print("during: %s" % os.environ.get("PATH"))
print("after: %s" % os.environ.get("PATH"))So you could use this just as:
with prepended_path(RSCT_PATH):
reset_rmc()I think that reads really nicely.
| """refresh_ipv6 should ip down and up the interface.""" | ||
| iface = "myeth0" | ||
| ccrmci.refresh_ipv6(iface) | ||
| print(m_subp.call_args_list) |
There was a problem hiding this comment.
drop the print() statements here. they were just debug. sorry.
There was a problem hiding this comment.
OK. The remaining changes I'm asking for:
- delete the file unittests/test_handler/test_handler_reset_rmc.py . the test in it doesn't do anything useful.
- update the '#commit message' that I have started for you in the "description" of the pull request (first comment... whatever you want to call that).
- add these modules , in their proper order and location, to config/cloud.cfg.tmpl. I suggest adding to the end of 'cloud_config_modules' unless you have a different place that you have been running them in your tests.
config/cloud.cfg.tmpl
Outdated
| @@ -146,6 +146,8 @@ cloud_final_modules: | |||
| - phone-home | |||
| - final-message | |||
| - power-state-change | |||
| - reset_rmc | |||
| - refresh_rmc_and_interface | |||
There was a problem hiding this comment.
move these up to before rightscale_userdata. thats really the cutoff for modules.
Reliable Scalable Cluster Technology (RSCT) is a set of software components that together provide a comprehensive clustering environment(RAS features) for IBM PowerVM based virtual machines. RSCT includes the Resource Monitoring and Control (RMC) subsystem. RMC is a generalized framework used for managing, monitoring, and manipulating resources. RMC runs as a daemon process on individual machines and needs creation of unique node id and restarts during VM boot. LP: #1895979
|
Hi @Aman306 This change is included in the most recent cloud-init release, which will soon be included in the Ubuntu distributions Xenial, Bionic, Focal, and Groovy. Since we don't have access to the platform necessary to test this change, the cloud-init team is asking if you can test that this change works as expected in Ubuntu. We hope to be finished testing by the end of the week, so if you can't verify this functionality by then, we cannot guarantee that this change will work as expected in our Ubuntu releases. If you are able to test the feature, please add a comment to our SRU process bug mentioning success or failure. The steps to setup the proposed image are laid out in the cloud-init docs. |
|
Hi Falcon,
I will try to verify the module and close it by Sunday.
Thanks
Aman
----- Original message -----From: James Falcon <notifications@github.com>To: canonical/cloud-init <cloud-init@noreply.github.com>Cc: Aman306 <amansi26@in.ibm.com>, Mention <mention@noreply.github.com>Subject: [EXTERNAL] Re: [canonical/cloud-init] IBM PowerVM specific RMC module (#584)Date: Mon, Dec 7, 2020 11:29 PM
Hi @Aman306
This change is included in the most recent cloud-init release, which will soon be included in the Ubuntu distributions Xenial, Bionic, Focal, and Groovy. Since we don't have access to the platform necessary to test this change, the cloud-init team is asking if you can test that this change works as expected in Ubuntu. We hope to be finished testing by the end of the week, so if you can't verify this functionality by then, we cannot guarantee that this change will work as expected in our Ubuntu releases. If you are able to test the feature, please add a comment to our SRU process bug mentioning success or failure.
The steps to setup the proposed image are laid out in the cloud-init docs.
—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.
|
IBM PowerVM specific RMC module
Reliable Scalable Cluster Technology (RSCT) is a set of software components that together provide a comprehensive clustering environment(RAS features) for IBM PowerVM based virtual machines. RSCT includes the Resource Monitoring and Control (RMC) subsystem. RMC is a generalized framework used for managing, monitoring, and manipulating resources. RMC runs as a daemon
process on individual machines and needs creation of unique node id and restarts during VM boot.
More details refer
https://www.ibm.com/support/knowledgecenter/en/SGVKBA_3.2/admin/bl503_ovrv.htm
To enable the healthy functioning of RMC services on ppcle Linux based VMs.
These modules are used for creating new node_id for every new instance/ virtual machine and ensure that Network Manager is not managing IPv6 interface ( interface is used by RSCT to communicate between RMC daemon and PowerVM Hypervisor).
This node_id is used by RSCT service for managing, monitoring, and manipulating resources.
We ensure that node_id must be unique and shouldn't changed subsequently by cloud-init even after multiple reboot.
On every reboot we are refreshing the RMC connection .
Since we are restricting Network Manager to handle IPv6 interface, on every reboot we are making the interface up.
LP: #1895979
commit message
Add config modules for controlling IBM PowerVM RMC.
Reliable Scalable Cluster Technology (RSCT) is a set of software
components that together provide a comprehensive clustering
environment(RAS features) for IBM PowerVM based virtual machines. RSCT
includes the Resource Monitoring and Control (RMC) subsystem. RMC is a
generalized framework used for managing, monitoring, and manipulating
resources. RMC runs as a daemon process on individual machines and needs
creation of unique node id and restarts during VM boot.
LP: #1895979