Regarding the duplication of config and draft

[sehkone]

@minshao

Since the configuration and its draft of an agent are now stored in the Agent, I think it would be better to remove them from the Settings. If we do so, the Settings will keep these:

pub struct Settings {
    pub customer_id: u32,
    pub description: String,
    pub hostname: String,

    pub piglet: bool,
    pub giganto: bool,
    pub reconverge: bool,
    pub hog: bool,
}

piglet, giganto, reconverge, and hog are all about whether they are enabled or disabled. Similar to the config and draft, piglet, reconverge, and hog could be stored in the Agent instead.

Could you share your thoughts on this?

[minshao]

Since this part of code is still under construction, it might be a bit confusing. Node and Settings are kept still to keep the interface with review-web relatively still. Within review-database, InnerSettings is used and defined as following:

#[derive(Clone, Deserialize, Serialize, PartialEq)]
struct InnerSettings {
    customer_id: u32,
    description: String,
    hostname: String,

    giganto: Option<Giganto>,
}

As shown above, settings for Agent has been removed. Now, it is stored in the Agent as a TOML formatted string. (Agent::config, Agent::draft) Giganto is not considered as an agent, and thus is still kept in Settings. If a certain agent need a status field, (mentioned in your comment):

    pub piglet: bool,
    pub giganto: bool,
    pub reconverge: bool,
    pub hog: bool,

we could:

• Add it to Agent.
• Or if it is required to maintain a draft version status, then IMHO, we can keep it in Agent::config and Agent::draft.

[sehkone]

@minshao

It's my mistake that I omit the Giganto part in Settings that I wrote.

[sehkone]

@minshao

I think what I have in my mind is very similar with InnerNode you created. I think InnerNode would suffice except the boolean values for agents, which I think can be added to Agent.

[sehkone]

@minshao

In other words, I suggest the following:

1. Add a boolean in Agent as follows:

pub struct Agent {
    pub node: u32,
    pub key: String,
    pub kind: Kind,
    pub config: Option<Config>,
    pub draft: Option<Config>,
    pub enabled: bool, // added
}

2. Remove Settings and rename InnerSettings with Settings as follows:

struct Settings {
    customer_id: u32,
    description: String,
    hostname: String,

    giganto: Option<Giganto>,
    giganto_enabled: bool, // added
}

3. Remove Node and rename InnerNode with Node as follows:

struct Node {
    id: u32,
    name: String,
    name_draft: Option<String>,
    settings: Option<Settings>,
    settings_draft: Option<Settings>,
    creation_time: DateTime<Utc>,

    agents: Vec<String>,
}

[minshao]

Current code:

struct InnerNode {
  ...
  settings: Option<InnerSettings>,
  settings_draft: Option<InnerSettings>,
}

struct InnerSettings {
  ...
  giganto: Option<Giganto>,
}

struct Giganto { // Noted as Giganto1
  // actual config of giganto
  ...
}

What I have in mind:

struct Node {
  ...
  giganto: Option<Giganto>,
}

struct Giganto { // Noted as Giganto2
  status: bool,
  config: Option<Config>,
  draft: Option<Config>,
}

And to be specific, InnerNode::settings::giganto will be converted into TOML format string and stored in Giganto::config, and InnerNode::settings_draft::giganto in Giganto::draft (Giganto here is marked as Giganto2 in the code above).

In this case, Giganto::status (Giganto2) satisfies the requirement and Giganto (Giganto2) aligns with Agent. Hope this answers your question about giganto_enabled.

[minshao]

As for keeping profile for giganto, we are currently doing that by storing giganto profile in Node or specifically Node::settings and Node::settings_draft as part of Node profile. Long term wise, it would be beneficial to store it separately into an independent table and only keep a key or id in Node, i.e. Giganto table will become a foreign table for Node, same as Agent table to Node.

[minshao]

As for status, do you expect our system to have other status for an agent? If that's the case, using a enum is probably more flexible than bool which allows more customization. For reference, kubernetes node status, it might not be a perfect match, I do still see the possibility that system admin might want more than just enabled or disabled from an agent's or node's status.

[sehkone]

As for status, do you expect our system to have other status for an agent? If that's the case, using a enum is probably more flexible than bool which allows more customization. For reference, kubernetes node status, it might not be a perfect match, I do still see the possibility that system admin might want more than just enabled or disabled from an agent's or node's status.

@minshao

Your point is very reasonable, and using an enum is better. From the discussion with Min, there will be at least one more status that represents when a node is not successfully reloaded with a new configuration. We still have the possibility of having more statuses.

[minshao]

Great, I'll incorporate it into later PR.

[sehkone]

struct Node {
  ...
  giganto: Option<Giganto>,
}

struct Giganto { // Noted as Giganto2
  status: bool,
  config: Option<Config>,
  draft: Option<Config>,
}

@minshao

I understand your plan on this and agree with you, except for the config. I think that since users will get Giganto's configuration from Giganto itself, storing the config in REview is not necessary from the user's point of view.

[minshao]

The reason for config and draft is to make sure no information is lost compared to the current settings::giganto + settings_draft::giganto combo. If we were to remove config and draft from Giganto, then, review-web won't be able to present the giganto information to the user. And if that is Ok, then we can just have:

struct Node {
  ...
  giganto: Option<Status>, // or giganto: bool,
}

and struct Giganto is not necessary in this case.

[sehkone]

The draft needs to be stored in REview because it, along with other drafts from agents, represents the users' input that should be communicated between the UI and REview. However, storing the config is not necessary because users will retrieve it directly from Giganto with the help of REview's reverse proxy.

[minshao]

I see, in that case, how about the following:

struct Node {
  giganto: Option<Giganto>,
}

struct Giganto {
  status: Status,
  draft: Option<Config>,
}

[sehkone]

To ensure we are on the same page, I reiterate the following:

pub struct Agent {
    pub node: u32,
    pub key: String,
    pub kind: Kind,
    pub status: Status,
    pub config: Option<Config>,
    pub draft: Option<Config>,
}

struct Settings {
    customer_id: u32,
    description: String,
    hostname: String,
}

pub struct Node {
    pub id: u32,
    pub name: String,
    pub name_draft: Option<String>,
    pub settings: Option<Settings>,
    pub settings_draft: Option<Settings>,
    pub creation_time: DateTime<Utc>,

    agents: Vec<String>,
    giganto: Option<Giganto>,
}

struct Giganto {
    status: Status,
    draft: Option<Config>,
}

Now, I feel like we need to rename Settings to something like Profile, then we will have the following:

struct Profile {
    customer_id: u32,
    description: String,
    hostname: String,
}

pub struct Node {
    pub id: u32,
    pub name: String,
    pub name_draft: Option<String>,
    pub profile: Option<Profile>,
    pub profile_draft: Option<Profile>,
    pub creation_time: DateTime<Utc>,

    agents: Vec<String>,
    giganto: Option<Giganto>,
}

Could you give your thoughts on this?

[minshao]

Profile sounds good, I think it's a nice fit~

[sophie-cluml]

@minshao Hello, I have one question regarding Agent, can you help me understand what key is ?
https://github.com/petabi/review-database/blob/main/src/tables/agent.rs#L88
I understood that the Agent's key starts with the key of the Node, but I am unsure what is expected to come as the key.

What's your suggestion for me to use instead of "test_key" here?
https://github.com/petabi/review-database/blob/main/src/tables/agent.rs#L282-L288

[minshao]

The key should be something unique to identify the agent in a node.

• For migration of existing nodes in the database, I'm using the string format of the type of the agent as the key, since at most one agent per type is allowed in the current setting. For instance, a certain node will only have one Hog agent, and during migration, "hog".to_string() will be used as the key for that agent of the node.
• However, the new design does allow the user to setup multiple Hog agents in one node theoretically. With that in mind, I do suggest to use the agent_key referred in the code. It is agent specific and accessible to review-web.
• As for testing purpose, any distinguished string should be working fine. The key won't affect other operation for now as long as it's node wise unique.

[sophie-cluml]

Thank you for explaing!

[sehkone]

@minshao
When do you anticipate finishing the work we discussed above? I am planning to overhaul the details of the configurations that agents and Giganto will have because they will retrieve all they need for configuration from REview. This will require code modifications to fully expand the configurations, and I would like to base it on the work you will complete.

[minshao]

I've finished the change to expose the new API in review-database, with some touch ups, I'll make a PR today. However, that does not include Giganto modification yet. It will be included in the next PR.