Change cloud test hierarchy, introduce cloud test base #16232
Conversation
travisdowns
changed the title
|
@simonlord I put you on here more as an FYI, as you wrote |
This comment was marked as resolved.
This comment was marked as resolved.
travisdowns
force-pushed
the
td-rptest-refactor-cloud
branch
from
d14f1fb
to
89b1f2d
Compare
A bit of additional typing prior to messing around with RedpandaServiceCloud.
This change changes a bit the inheritance structure of the RedpandaService tree, and introduces the RedpandaCloudTest. On the service side, we split RedpandaServiceCloud out so that it is not a subclass of Service (as it does no use Service nodes or have a typical Service lifecycle). It shares some common interface with RedpandaService but this is currently much smaller than the existing RedpandaServiceBase. RedpandaCloudTest is the base class for tests which run in the cloud, analagous to RedpandaTest for tests which run in vanilla environments. It automatically creates a RedpandaServiceCloud as the .redpanda property, giving you type safe access to only the methods which make sense in the cloud. Issue redpanda-data/core-internal#1002.
travisdowns
force-pushed
the
td-rptest-refactor-cloud
branch
from
89b1f2d
to
4f2a82b
Compare
|
Rebase (+fix error in new KubectlSelfTest): 4f2a82b |
tests/rptest/services/redpanda.py
Outdated
| self._check_attr('context', TestContext) | ||
| self._check_attr('logger', Logger) | ||
|
|
||
| def healthy(self) -> bool: |
There was a problem hiding this comment.
why this is not an @abstractmethod too?
There was a problem hiding this comment.
Good question, it's because cloud service does not implement this yet (we have a bug for it). In any case, we don't need this in the ABC as we don't use it elsewhere in the ABC, so I have removed it.
| f'num_brokers is {num_brokers}, but setting to None for cloud') | ||
| # we save the test context under both names since RedpandaService and Service | ||
| # save them under these two names, respetively | ||
| self.context = self._context = context |
There was a problem hiding this comment.
why not make RedpandaServiceABC accept these via __init__ and pass them via super().__init__ instead?
There was a problem hiding this comment.
It's possible, but is there much of a concrete benefit? This ABC appears in a hierarchy which always provides a .context member. This is also how the KubeServiceMixin behaves, and similarly any other mixin we may add in the future, I guess.
If we pass it, we will need to store it, I guess in self.__context to make it class-private to avoid clobbering the context var from other classes. The behavior of the constructors for the Service hierarchy is not totally straightforward since on the other flavor (RedpandaService) we inherit from Service which isn't really a cooperating base class and wants context as the first positional argument. So run into the mixin/multiple inheritance issue where you need to avoid passing context to a base that doesn't expect it. object doesn't expect it, for example. So you need to then pass context twice from RedpandaService: once as the first positional argument to be consumed (eventually) by Service and once under some other name to be consumed by RedpandaServiceABC.
All for little gain I think. The mixin pattern mostly hides the types from the type-checker anyway (as we are often forwarding **kwargs, essentially erasing the types), which I think is the main benefit of passing it.
Other options are possible, e.g., the ABC could declare an abstract method or property returning a context which the bases would have to implement. This bypasses the constructor.
Despite this wall of text, I'm not convinced one way or the other, so interested to hear your thoughts too.
from RedpandaServiceABC, as it wasn't implemented on the cloud side nor did we use it in the ABC, so remove it. Issue redpanda-data/core-internal#1002.
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44301#018d41cc-24e9-4c36-b6a2-8f026a806748 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44301#018d41f4-394a-47cd-9754-c2e81807ed80 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44301#018d41f4-3944-4a41-8537-b21d5d30ec8b ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44428#018d55e9-a131-4dbd-9210-c3b340c2b6bb ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44428#018d55e9-a12f-4d21-a4ba-37b09988b9d6 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44428#018d55e3-a853-4cb5-a880-3073a7eec494 |
There was a problem hiding this comment.
LGTM
i didn't go line-by-line, but from our htt discussions, i agree with this PR and the principle behind the changes, especially:
On the service side, we split RedpandaServiceCloud out so that it is
not a subclass of Service (as it does not use Service nodes nor have
a typical Service lifecycle).
This change changes a bit the inheritance structure of the
RedpandaService tree, and introduces the RedpandaCloudTest.
On the service side, we split RedpandaServiceCloud out so that it is
not a subclass of Service (as it does not use Service nodes nor have
a typical Service lifecycle). It shares some common interface with
RedpandaService but this is currently much smaller than the existing
RedpandaServiceBase.
RedpandaCloudTest is the base class for tests which run in the cloud,
analagous to RedpandaTest for tests which run in vanilla environments.
It automatically creates a RedpandaServiceCloud as the .redpanda
property, giving you type safe access to only the methods which make
sense in the cloud.
Issue https://github.com/redpanda-data/core-internal/issues/1002.
Backports Required
Release Notes