WIP: Add support for profiles #25

letian · 2025-04-29T20:01:36Z

letian commented

2025-04-29 20:01:36 +00:00

Here's a first draft regarding the support of profiles.

It's quite simple & flexible, but something bugs me with the fact that the profile names are plain strings.

Here's a first draft regarding the support of profiles. It's quite simple & flexible, but something bugs me with the fact that the profile names are plain strings. Related to https://git.nationtech.io/NationTech/Team/issues/85

letian reviewed 2025-04-29 20:06:11 +00:00

letian force-pushed runtime-profiles from 50ca69c02c to 814c37d711

2025-04-29 20:07:50 +00:00

Compare

letian reviewed 2025-04-29 20:08:52 +00:00

harmony/src/modules/lamp.rs Outdated

						
				@@ -78,8 +104,9 @@ impl<T: Topology + K8sclient> Interpret<T> for LAMPInterpret {

				        info!("LAMP deployment_score {deployment_score:?}");

				        todo!();

				        deployment_score

				            .apply_profile(profile)

letian commented

2025-04-29 20:08:52 +00:00

I'm not a big fan of this because it introduces a temporal coupling. We have to remember to call it before creating our interpret otherwise we're screwed.

I'll check for another solution.

I'm not a big fan of this because it introduces a temporal coupling. We have to remember to call it before creating our interpret otherwise we're screwed. I'll check for another solution.

letian commented

2025-04-29 20:39:01 +00:00

Without mentioning it at all, Gemini have the same concerns I have 😆

Weaknesses and Concerns:

Error Handling: The panic!("Not good") in LAMPScore::apply_profile is completely unacceptable for production code. It will crash the entire application if an unknown profile is provided. This violates robustness principles.
Stringly Typed Profiles: Using &String for profile names is not ideal. It introduces potential for typos and doesn't provide compile-time safety. Consider using an enum or a strongly typed wrapper.
Limited Override Flexibility: The current implementation in LAMPScore::apply_profile only overrides ssl_enabled. If we have more settings, this function will become very complex and hard to maintain. It violates the Open/Closed Principle.
Temporal Coupling: The order of operations is important. The apply_profile function is called in Maestro, but the Interpret might still need to be aware of the profile for certain logic. This creates temporal coupling and can lead to bugs.
Complexity in Interpret::execute: Passing the profile down to Interpret::execute adds complexity to the Interpret trait and its implementations. If the Interpret only needs the result of applying the profile (the effective config), it shouldn't need the raw profile string.
Duplication: The LAMPConfig is cloned in apply_profile, which can be inefficient if the config is large.

Without mentioning it at all, Gemini have the same concerns I have 😆 **Weaknesses and Concerns:** * Error Handling: The panic!("Not good") in LAMPScore::apply_profile is completely unacceptable for production code. It will crash the entire application if an unknown profile is provided. This violates robustness principles. * Stringly Typed Profiles: Using &String for profile names is not ideal. It introduces potential for typos and doesn't provide compile-time safety. Consider using an enum or a strongly typed wrapper. * Limited Override Flexibility: The current implementation in LAMPScore::apply_profile only overrides ssl_enabled. If we have more settings, this function will become very complex and hard to maintain. It violates the Open/Closed Principle. * Temporal Coupling: The order of operations is important. The apply_profile function is called in Maestro, but the Interpret might still need to be aware of the profile for certain logic. This creates temporal coupling and can lead to bugs. * Complexity in Interpret::execute: Passing the profile down to Interpret::execute adds complexity to the Interpret trait and its implementations. If the Interpret only needs the result of applying the profile (the effective config), it shouldn't need the raw profile string. * Duplication: The LAMPConfig is cloned in apply_profile, which can be inefficient if the config is large.

letian force-pushed runtime-profiles from 814c37d711 to f80c1dd711

2025-04-29 20:16:08 +00:00

Compare

letian force-pushed runtime-profiles from f80c1dd711 to 2f7c4924c1

2025-04-29 20:31:01 +00:00

Compare

johnride requested changes 2025-04-29 20:44:54 +00:00

johnride left a comment

Voir mes commentaires, il faut en discuter davantage. Je n'ai pas l'impression que c'est la bonne approche.

examples/lamp/src/main.rs

						
				@@ -17,6 +18,10 @@ async fn main() {

				            project_root: "./php".into(),

				            ..Default::default()

				        },

				        profiles: HashMap::from([

johnride commented

2025-04-29 20:44:21 +00:00

On ne veut pas donner la tache de setup du profil a l'utilisateur du Score.

L'utilisateur du Score ne devrait pas etre au courant que les profils existent a ce niveau. Lorsqu'il ouvre l'implementation d'un score il peut voir quelles differences sont faites d'un profil a l'autre, mais comme c'est suggéré ici ça ne correspond pas à l'objectif initial :

Comme le propose le projet score-spec, les configurations doivent être exactement la même pour le développeur (ici, celui qui écrit le code du fichier examples/lamp/src/main.rs ) . La plateforme ensuite va déployer la configuration demandée automatiquement.

Il faut bien garder en tête que c'est le rôle de la plateforme elle-même d'offrir une fonctionnalité de déploiement standard et d'implémenter les fonctionnalités qui correspondent à son profil à elle.

Donc, l'implémentation interne du Score/Interpret peut fournir de l'information sur comment déployer/orchestrer son workload selon différents profils standards, et la plateforme elle même ira consulter le score pour voir quelle configuration doit être appliquée selon son profil à elle.

Aussi, la plateforme fournira une implémentation par défaut et de l'instrumentation automatique selon son implémentation spécifique pour les différents profils. Par exemple, une plateforme de profil PROD pourrait être en single-availability-zone et une autre en mode "warm-failover" et une autre en "decentralized", mais la configuration haut niveau du Score ne doit pas changer d'une implémentation à l'autre.

Donc, fournir un nouveau champ profiles: HashMap ne me semble pas convenir. Je ne suis pas totalement certain par contre.

On ne veut pas donner la tache de setup du profil a l'utilisateur du Score. L'utilisateur du Score ne devrait pas etre au courant que les profils existent a ce niveau. Lorsqu'il ouvre l'implementation d'un score il peut voir quelles differences sont faites d'un profil a l'autre, mais comme c'est suggéré ici ça ne correspond pas à l'objectif initial : Comme le propose le projet score-spec, les configurations doivent être exactement la même pour le développeur (ici, celui qui écrit le code du fichier `examples/lamp/src/main.rs` ) . La plateforme ensuite va déployer la configuration demandée automatiquement. Il faut bien garder en tête que c'est le rôle de la plateforme elle-même d'offrir une fonctionnalité de déploiement standard et d'implémenter les fonctionnalités qui correspondent à son profil à elle. Donc, l'implémentation interne du Score/Interpret peut fournir de l'information sur comment déployer/orchestrer son workload selon différents profils standards, et la plateforme elle même ira consulter le score pour voir quelle configuration doit être appliquée selon son profil à elle. Aussi, la plateforme fournira une implémentation par défaut et de l'instrumentation automatique selon son implémentation spécifique pour les différents profils. Par exemple, une plateforme de profil PROD pourrait être en single-availability-zone et une autre en mode "warm-failover" et une autre en "decentralized", mais la configuration haut niveau du Score ne doit pas changer d'une implémentation à l'autre. Donc, fournir un nouveau champ `profiles: HashMap` ne me semble pas convenir. Je ne suis pas totalement certain par contre.

letian commented

2025-04-29 21:47:53 +00:00

Dans ce cas, ca semble toucher 2 aspects differents en fait:

Le developpeur final devrait pouvoir avoir le controle de configurer certains aspects d'un score en fonction du profil (ce que cette PR introduit). Le score va donc exposer un API public (via sa config) de ce que le developpeur a le controle dessus.
Le maintainer d'un Score devrait "imposer" des facons de faire differentes selon le profil pour s'assurer que son workload est deploye/orchestre comme il le faudrait.

Sauf qu'en ce moment, pour le point 2, ca se ferait directement dans l'implementation Interpret::execute du Score. Ce qui, par extension, force dans cette PR a pousser le profile a la fois dans le apply_profile (ou create_interpret si on veut regler le probleme de temporal coupling) et dans le Interpret::execute. Ce qui est en fait un beau gros smell d'un plus grand probleme de design.

Actuellement, les Score cachent a l'interieur de l'execute le fait qu'ils vont utiliser d'autres Score et que ces Score la pourraient avoir besoin du resultat d'eux meme ou d'un autre score.

Exemple:

+-----------+
|LampScore  |
+-----------+
     |
     V
+------------+
|Build Docker|
|  (action)  |
+------------+
     |
     V
+-----------+
|Image Name |
|  (output) |
+-----------+
     | 
     |
     |
     V
+-----------+
|K8sDeploy  |
|  Score    |
+-----------+
     |
     V
+-----------+
|Use Image  |
|  (action) |
+-----------+
     |
     V
+-----------+
|  Result   |
+-----------+

En ce moment, cela fonctionne car c'est a petite echelle et on a peu de Score qui se reutilisent les uns et les autres. Mais, ca va vite devenir un gros probleme.

Il faudrait donc etre capable de vraiment separer le process en deux etapes:

Prepare l'arbre de tout ce qui va etre fait en liant les dependances entre les extrants <-> intrants (e.g. LampScore -> Build Docker Image -> K8sDeploymentScore)
Execute l'arbre et chainant les extrants avec les intrants correctement (comme une Pipeline, mais possiblement plus complexe)

Tout ceci aurait l'avantage de pouvoir faire comme Terraform avec des dry-run et exprimer un "plan de match" de comment le workload va etre deploye/orchestre.

Il y a encore de la reflexion a avoir sur comment tout ca peut etre design, mais ca serait a faire plus tot que plus tard puisque les smells commencent a sortir de plus en plus.

Dans ce cas, ca semble toucher 2 aspects differents en fait: 1. Le developpeur final devrait pouvoir avoir le controle de configurer certains aspects d'un score en fonction du profil (ce que cette PR introduit). Le score va donc exposer un API _public_ (via sa config) de ce que le developpeur a le controle dessus. 2. Le maintainer d'un Score devrait "imposer" des facons de faire differentes selon le profil pour s'assurer que son workload est deploye/orchestre comme il le faudrait. Sauf qu'en ce moment, pour le point 2, ca se ferait directement dans l'implementation `Interpret::execute` du Score. Ce qui, par extension, force dans cette PR a pousser le `profile` a la fois dans le `apply_profile` (ou `create_interpret` si on veut regler le probleme de temporal coupling) et dans le `Interpret::execute`. Ce qui est en fait un beau gros smell d'un plus grand probleme de design. Actuellement, les Score cachent a l'interieur de l'execute le fait qu'ils vont utiliser d'autres Score et que ces Score la pourraient avoir besoin du resultat d'eux meme ou d'un autre score. Exemple: ``` +-----------+ |LampScore | +-----------+ | V +------------+ |Build Docker| | (action) | +------------+ | V +-----------+ |Image Name | | (output) | +-----------+ | | | V +-----------+ |K8sDeploy | | Score | +-----------+ | V +-----------+ |Use Image | | (action) | +-----------+ | V +-----------+ | Result | +-----------+ ``` En ce moment, cela fonctionne car c'est a petite echelle et on a peu de Score qui se reutilisent les uns et les autres. Mais, ca va vite devenir un gros probleme. Il faudrait donc etre capable de vraiment separer le process en deux etapes: 1. Prepare l'arbre de tout ce qui va etre fait en liant les dependances entre les extrants <-> intrants (e.g. LampScore -> Build Docker Image -> K8sDeploymentScore) 2. Execute l'arbre et chainant les extrants avec les intrants correctement (comme une Pipeline, mais possiblement plus complexe) Tout ceci aurait l'avantage de pouvoir faire comme Terraform avec des dry-run et exprimer un "plan de match" de comment le workload va etre deploye/orchestre. Il y a encore de la reflexion a avoir sur comment tout ca peut etre design, mais ca serait a faire plus tot que plus tard puisque les smells commencent a sortir de plus en plus.

examples/lamp/src/main.rs

						
				@@ -18,2 +19,4 @@

				            ..Default::default()

				        },

				        profiles: HashMap::from([

				            ("dev", LAMPProfile { ssl_enabled: false }),

johnride commented

2025-04-29 20:34:26 +00:00

Pourrait etre un enum :

pub enum DEPLOYMENT_PROFILE {
  DEV,
  STAGING,
  PROD,
  CUSTOM(String),
}

Pourrait etre un enum : ```rust pub enum DEPLOYMENT_PROFILE { DEV, STAGING, PROD, CUSTOM(String), } ```

harmony/src/modules/lamp.rs

						
				@@ -40,0 +48,4 @@

				    fn apply_profile(&self, profile: &String) -> Box<dyn Score<T>> {

				        let profile = match self.profiles.get(profile.as_str()) {

				            Some(profile) => profile,

				            None => panic!("Not good"), // TODO: better handling

johnride commented

2025-04-29 20:35:32 +00:00

instead of panic!() // TODO just use todo!("comment here");

instead of panic!() // TODO just use `todo!("comment here");`

letian reviewed 2025-04-29 21:51:28 +00:00

harmony/src/modules/lamp.rs

						
				@@ -37,6 +45,23 @@ impl Default for LAMPConfig {

				}

				impl<T: Topology + K8sclient> Score<T> for LAMPScore {

				    fn apply_profile(&self, profile: &String) -> Box<dyn Score<T>> {

letian commented

2025-04-29 21:51:28 +00:00

Je n'ai pas le temps de le faire maintenant, mais pour regler le probleme de temporal coupling que cette methode introduit, il faudrait simplement a la supprimer et reutiliser create_interpret pour y mettre la logique de merge la config avec le profile avant de creer l'interpret.

Je n'ai pas le temps de le faire maintenant, mais pour regler le probleme de temporal coupling que cette methode introduit, il faudrait simplement a la supprimer et reutiliser `create_interpret` pour y mettre la logique de merge la config avec le profile avant de creer l'interpret.

letian closed this pull request

2025-08-04 21:42:25 +00:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: NationTech/harmony#25