NationTech/harmony
5
0
Fork 2
Code Issues 37 Pull Requests 16 Actions Packages Projects Releases 1 Wiki Activity

feat(OKDInstallation): Implemented bootstrap of okd worker node, added features to allow both control plane and worker node to use the same bootstrap_okd_node score #198

Merged
wjro merged 2 commits from feat/okd-nodes into master 2025-12-10 19:27:51 +00:00
Conversation 2 Commits 2 Files Changed 7 +395 -292
wjro commented 2025-12-10 17:16:25 +00:00
Owner
Copy Link
No description provided.
wjro added 1 commit 2025-12-10 17:16:29 +00:00
feat(OKDInstallation): Implemented bootstrap of okd worker node, added features to allow both control plane and worker node to use the same bootstrap_okd_node score
All checks were successful
Run Check Script / check (pull_request) Successful in 1m46s
Details
50bd5c5bba
johnride requested changes 2025-12-10 17:28:06 +00:00
Dismissed
johnride left a comment
Owner
Copy Link

Quick review done, overall looks pretty good but deserves some improvements, mostly naming.

Quick review done, overall looks pretty good but deserves some improvements, mostly naming.
harmony/src/modules/okd/bootstrap_04_workers.rs Outdated
@@ -79,2 +71,3 @@
topology: &HAClusterTopology,
) -> Result<Outcome, InterpretError> {
self.render_and_reboot().await?;
OKDNodeInterpret::new(HostRole::Worker)
johnride commented 2025-12-10 17:21:06 +00:00
Owner
Copy Link

On passe par le score et on call .execute dessus directement. On n'instance pas d'Interpret directement en dehors du get_interpret d'un score.

Ca permet d'instrumenter les scores et suivre ce qui se passe comme il faut.

On passe par le score et on call `.execute` dessus directement. On n'instance pas d'Interpret directement en dehors du get_interpret d'un score. Ca permet d'instrumenter les scores et suivre ce qui se passe comme il faut.
harmony/src/modules/okd/bootstrap_okd_node.rs Outdated
@@ -0,0 +26,4 @@
};
#[derive(Debug, Clone, Serialize, new)]
pub struct OKDNodeScore {
johnride commented 2025-12-10 17:24:35 +00:00
Owner
Copy Link

Le nom ne dit pas ce que ca fait. OKDNodeScore ce n'est pas un nom assez descriptif. Quelque chose comme PrepareOKDNodeInstallationScore ou AddNodeToOKDClusterScore. Bref, je n'ai aucune idee de ce que ca fait, ca m'obliger a aller lire le code ce qui ralentis tout ceux qui vont lire ce code dans le futur.

When you are naming consider below 3 questions when you are naming variables, functions or classes and try to get the answers via the name;

Why it exists?
What does it do?
How it is used?

https://medium.com/@pabashani.herath/clean-code-naming-conventions-4cac223de3c6

Le nom ne dit pas ce que ca fait. OKDNodeScore ce n'est pas un nom assez descriptif. Quelque chose comme PrepareOKDNodeInstallationScore ou AddNodeToOKDClusterScore. Bref, je n'ai aucune idee de ce que ca fait, ca m'obliger a aller lire le code ce qui ralentis tout ceux qui vont lire ce code dans le futur. > When you are naming consider below 3 questions when you are naming variables, functions or classes and try to get the answers via the name; > > Why it exists? > What does it do? > How it is used? https://medium.com/@pabashani.herath/clean-code-naming-conventions-4cac223de3c6
wjro commented 2025-12-10 19:22:42 +00:00
Author
Owner
Copy Link

Je l'ai modifié pour avoir le nom OKDNodeInstallationScore, je croix que c'est mieux aligné avec l'info partagé en haut

Je l'ai modifié pour avoir le nom `OKDNodeInstallationScore`, je croix que c'est mieux aligné avec l'info partagé en haut
harmony/src/modules/okd/bootstrap_okd_node.rs Outdated
@@ -0,0 +264,4 @@
// 4. Reboot the nodes to start the OS installation.
self.reboot_targets(&nodes).await?;
// TODO: Implement a step to wait for the control plane nodes to join the cluster
johnride commented 2025-12-10 17:25:42 +00:00
Owner
Copy Link

Fix this comment, not accurate in the context.

Fix this comment, not accurate in the context.
harmony/src/modules/okd/okd_node.rs Outdated
@@ -0,0 +54,4 @@
}
//TODO unsure if this is to be implemented here or not
impl OKDRoleProperties for StorageRole {
johnride commented 2025-12-10 17:27:36 +00:00
Owner
Copy Link

For now we can completely delete the storage role, it is not used. We will use worker nodes for storage, and we will autodetect their storage capabilities and orchestrate at a higher level than the OKD role.

For now we can completely delete the storage role, it is not used. We will use worker nodes for storage, and we will autodetect their storage capabilities and orchestrate at a higher level than the OKD role.
wjro added 1 commit 2025-12-10 19:20:34 +00:00
fix: deleted storage node role, fixed erroneous comment, modified score name to be in line with clean code naming conventions, fixed how the OKDNodeInstallationScore is called via OKDSetup03ControlPlaneScore and OKDSetup04WorkersScore
All checks were successful
Run Check Script / check (pull_request) Successful in 1m45s
Details
d5fadf4f44
johnride approved these changes 2025-12-10 19:26:26 +00:00
johnride left a comment
Owner
Copy Link

LGTM

LGTM
wjro merged commit bfdb11b217 into master 2025-12-10 19:27:51 +00:00
wjro deleted branch feat/okd-nodes 2025-12-10 19:27:52 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
johnride
Labels
Clear labels
Architecture Decision Record
Good first issue
RFC
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
johnride jvtrudel letian stremblay wjro
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: NationTech/harmony#198
No description provided.
Delete Branch "feat/okd-nodes"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?