Repository navigation
Conversation
These have been disabled due to nebulous long-ago refactorings since 2017, which is a doubly long time ago. Let's re-enable them and see what happens.
This uses `crm_node -e` to figure out what epoch a node joined the cluster. However, this option was removed by 55ab749 in 2017 and the whole epoch concept doesn't even apply to a non-heartbeat cluster. So, this check can be removed.
I can't find any reference to this argument as far back as the latest state of the 1.1 branch, which is where I stopped looking.
This may affect other attributes besides score. This was introduced by 66f20f7.
These audits check if a resource is running in more than one place which is always going to be the case in the split brain test.
This code was ssh'ing into each host, running cibadmin -Q, writing that to a file, and then scp'ing that file back to a host. There was also some juggling around to handle the first host in the list differently. It also didn't clean up the files it wrote. This is all a little more complicated than it needs to be. Instead, we can just capture stdout from the ssh command and write that to a file on the executor, diff things there, and clean up.
Sometimes, the CIBAudit test fails because different nodes have cib-last-written timestamps that are different. However, nothing else in the CIB is different. So, just remove the timestamp to remove the possibility of any differences there.
I don't think they should be run more than once, certainly not things like the PrimitiveAudit which runs at least once before any of the tests run, and then runs during the tests as well. But that's a bigger problem to solve. The problem here is that every time PrimitiveAudit (or its subclasses) run, they append to various lists without first resetting them. Over time, this means that the same checks get run over and over again. This really drags down the tests. So, just reset the lists.
The clean variable is unnecessary and makes this block kind of weird to read, so just remove it.
* Move the have_systemd test into _find_core_with_coredumpctl so we have one less case to consider in the caller. * Get rid of the unnecessary intermediate found variable.
This is just the same trick where we invert the test and continue, allowing everything else to be unindented.
We don't need to loop through the nodes twice.
* Nothing uses the _trim2int function. * Move the strip call into _trim_string to simplify callers. * Simplify callers by making everything a single call instead of two.
This has been an empty list since a refactoring commit in 2005 (73281ce).
If the split fails with an exception, it's possible one or both members of the tuple won't exist, in which case the logging message will also raise an exception. I don't think logging the tuple members is useful, so just remove it.
We were querying each node in a partition to see if it had quorum, but not doing anything with that information. So, let's start testing it.
f8d1a83 to
99ca038
Compare
| if not self._audit_partition(partition): | ||
| passed = False | ||
|
|
||
| if not any(v == "1" for v in self._node_quorum.values()): |
There was a problem hiding this comment.
The following seems simpler; am I missing something?
if "1" not in self._node_quorum.values():
| (_, out) = self._cm.rsh.call(node, self._cm.templates["QuorumCmd"], verbose=1) | ||
| self._node_quorum[node] = out[0].strip() | ||
|
|
||
| self.debug(f"Node {node}: {self._node_state[node]} - {self._node_epoch[node]} - {self._node_quorum[node]}.") |
There was a problem hiding this comment.
It would be reasonable to just drop - {self._node_epoch[node]} from this line. I don't know whether debug-logging the state and quorum really add any value either though.
| else: | ||
| (rc, result) = self._cm.rsh.call( | ||
| node0, f"crm_diff -VV -cf --new {node_xml} --original {node0_xml}", verbose=1) | ||
| node0, f"crm_diff -VV -c --new {node_xml} --original {node0_xml}", verbose=1) |
| Create a colocation constraint between this resource and some other. | ||
|
|
||
| Arguments: | ||
| resource -- The name of the resource that should be located relative |
There was a problem hiding this comment.
This seems incorrect. resource is the name of the primary resource in the colocation. self.name is the dependent resource in the colocation. I noticed this while trying to understand the _constraints() function.
| for (k, kargs) in self._coloc.items(): | ||
| attrs = {"id": f"{self.name}-with-{k}", "rsc": self.name, "with-rsc": k} | ||
| text += element("rsc_colocation", **attrs) | ||
| text += element("rsc_colocation", **attrs, **kargs) |
There was a problem hiding this comment.
Some more explanation/context in the commit message would be nice. It took me a few minutes to understand why we were adding kargs here.
I think you're saying it should have been added by commit 66f20f7 and was omitted by mistake. Each item is a single colocation constraint where self.rsc is the dependent resource. k is the name of the primary resource. kargs is the set of XML attributes for the colocation, aside from the ones that we set here in attrs. The colocate() function sets kargs to contain score, rsc-role (if specified), with-rsc-role (if specified), and any other attributes that were specified.
| """Fetch a copy of the given node's CIB and return it as a list.""" | ||
| (rc, lines) = self._cm.rsh.call(node, self._cm.templates["CibQuery"], verbose=1) | ||
| if rc != 0: | ||
| logging.log("Could not retrieve configuration") |
There was a problem hiding this comment.
This message is redundant, since the caller logs f"Could not perform audit: No configuration from {node}".
If we drop that message, I don't think it's functionizing this anymore.
My branch includes both of these changes.
|
|
||
| return passed | ||
|
|
||
| def _cleanup_cibs(self): |
There was a problem hiding this comment.
This explicit cleanup isn't necessary if we use a tempfile.TemporaryDirectory as a context manager. My branch includes this change.
| """Perform the audit action.""" | ||
| max_attempts = 3 | ||
| attempt = 0 | ||
| passed = True |
There was a problem hiding this comment.
There are lots more opportunities to drop passed variables for return values. You probably are aware already.
| if not dc_found: | ||
| logging.log(f"DC not found on any of the {len(dc_allowed_list)} allowed " | ||
| f"nodes: {dc_allowed_list} (of {node_list})") | ||
| logging.log("DC not found on any node") |
There was a problem hiding this comment.
Commit message nitpick: "Get rid of the dc_allowed_list check" -- there is no check. We're just dropping the variable in this commit.
| remaining_mb = int(remain) | ||
| except (ValueError, TypeError): | ||
| logging.log(f"Warning: df output '{dfout}' from {node} was invalid [{used}, {remain}]") | ||
| logging.log(f"Warning: df output '{dfout}' from {node} was invalid") |
There was a problem hiding this comment.
From commit message:
If the split fails with an exception, it's possible one or both members
of the tuple won't exist
How could the split fail with an exception? We can assume dfout is a string. It seems to me that an exception could come only from the int() calls.
2 participants
No description provided.