[Kernel scoping 2/5] Add kernel version to HostInfo #1685
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| #include <string> | ||
| #include <utility> | ||
|
|
||
| #include "src/common/system/kernel_version.h" | ||
| #include "src/vizier/services/agent/shared/manager/manager.h" | ||
|
|
||
| namespace px { | ||
|
|
@@ -43,9 +44,11 @@ class KelvinManager : public Manager { | |
| KelvinManager() = delete; | ||
| KelvinManager(sole::uuid agent_id, std::string_view pod_name, std::string_view host_ip, | ||
| std::string_view addr, int grpc_server_port, std::string_view nats_url, | ||
| std::string_view mds_url, px::system::KernelVersion kernel_version) | ||
| // TODO(@benkilimnik): Kernel version may not be needed in kelvin, only in PEMManager for | ||
| // script selection based on HostInfo. Could use dummy value or refactor Manager class. | ||
|
There was a problem hiding this comment. Agreed that Kelvin may not need kernel version, at least for the purposes we have right now. Is it possible to remove from Kelvin (or is there some common base class where we add kernel version as an arg. making it required as an arg. here)?
Member
Author
There was a problem hiding this comment. There is a common base class I think we could handle this in a number of ways:
protected:
Manager(..., bool use_kernel_version = true);Manager::Manager(..., bool use_kernel_version) {
if (use_kernel_version) {
// Find and set the kernel version
}
// other initialization
}KelvinManager::KelvinManager(...)
: Manager(..., false) { // Pass false to indicate that we don't need to go find the kernel version
// KelvinManager specific initialization
}
Member
There was a problem hiding this comment. I have another proposal for addressing this. What if we mutated the This commit (1795988) shows an example of what I mean. If we like that direction, I think we should also add tests to ensure that a KelvinManager and PEMManager have an I see this as an improvement to option 3 because it ensures that a newly minted PEMManager is properly configured and reduces risk of using the API incorrectly (forgetting to call the kernel version setter).
Member
Author
There was a problem hiding this comment. That seems like a more robust solution. Regarding testing, are you imagining something like this, where we check that the kernel version is initialized after construction? (I think the member function class PEMManagerTest : public ::testing::Test {
protected:
PEMManagerTest() {
agent_info_ = agent::Info{};
agent_info_.agent_id = sole::uuid4();
agent_info_.hostname = "hostname";
agent_info_.address = "address";
agent_info_.pod_name = "pod_name";
agent_info_.host_ip = "host_ip";
agent_info_.kernel_version =
system::ParseKernelVersionString("5.15.0-106-generic").ValueOrDie();
}
agent::Info agent_info_;
};
TEST_F(PEMManagerTest, ConstructorTest) {
auto manager =
PEMManager::Create(agent_info_.agent_id, agent_info_.pod_name, agent_info_.host_ip,
"nats_url", agent_info_.kernel_version)
.ConsumeValueOrDie(); // Remove the semicolon before .ConsumeValueOrDie()
EXPECT_EQ(manager->info()->agent_id, agent_info_.agent_id);
EXPECT_EQ(manager->info()->pod_name, agent_info_.pod_name);
EXPECT_EQ(manager->info()->host_ip, agent_info_.host_ip);
EXPECT_EQ(manager->info()->kernel_version, agent_info_.kernel_version);
}
Member
There was a problem hiding this comment. @benkilimnik that's what I was thinking and I think making it a friend test function sounds good.
Member
Author
There was a problem hiding this comment. Because of the way F20230908 07:05:26.144451 12 statusor.h:148] Check failed: _s.ok() Bad Status: Internal : Failed to read file ../../services/certs/ca.crt (No such file or directory)which I believe comes from Manager::Manager(sole::uuid agent_id, std::string_view pod_name, std::string_view host_ip,
int grpc_server_port, services::shared::agent::AgentCapabilities capabilities,
services::shared::agent::AgentParameters parameters, std::string_view nats_url,
std::string_view mds_url)
: grpc_channel_creds_(SSL::DefaultGRPCClientCreds()),
...I think a test that mocks out parts of the
Member
Author
There was a problem hiding this comment. After some discussion with @ddelnano, I ended up adding a private constructor to There was a problem hiding this comment. I'd be ok with option (5) keep the version in Kelvin if it nets out to "simpler." It appears we already have a "way" to avoid going here, but is the pain worth it? I am ok with either route btw.
Member
There was a problem hiding this comment. Despite advocating for the current direction, I do think that having the kernel version for both is likely simpler. Sorry for causing additional change @benkilimnik, but after reconsidering (5) is probably best. If the Manager class was an area where future test coverage would helpful that might tip the scale, but as we've seen it's very difficult to exercise its code. |
||
| : Manager(agent_id, pod_name, host_ip, grpc_server_port, KelvinManager::Capabilities(), | ||
| KelvinManager::Parameters(), nats_url, mds_url, kernel_version) { | ||
| info()->address = std::string(addr); | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.