-
Notifications
You must be signed in to change notification settings - Fork 694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Automatically generate probes when creating a chaos experiment using a YAML file. #4366
Conversation
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
…uto-generate-probe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chaos-web changes look good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FE changed looks good to me, thanks 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please share the logic where the addProbe mutation is called in the PR! Or how the manifest is appending the probeRef
if a user enters the older probe metadata!
const doesProbeExists = await (experimentHandler as KubernetesYamlService)?.doesProbeMetadataExists( | ||
experiment?.manifest as KubernetesExperimentManifest | ||
); | ||
if (probeWithoutAnnotation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check conflicts with the logic we are trying to enforce. probeWithoutAnnoitation
returns the name of the fault which doesn't have the probeRef
annotation or the new resilience probes in the manifest basically. With the above logic it checks if probeWithoutAnnotation
-> returns a string with the fault name if res probes aren't present, then it goes inside the check and checks doesProbeExists
which are basically checking if legacy probes with inline support are present in the manifest or not. If it is present, then it returns true. The next check verifies if (!true)
this is a case where there is no res probes but an old probe, it bypasses the warning because of if (!true)
whereas it should be shown since res probes are missing.
The previous logic used the if (probeWithoutAnnotation)
to validate if res probe are present or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a flow @S-ayanide , As you can see ChaosExperimentService
directly calls ProbeService(handler)
instead of calling graphql mutation.
Currently, the frontend is calling the SaveChaosExperiment
mutation both on creation and update of an Experiment. This function creates a ChaosExperiment based on a manifest in yaml or json format submitted by the frontend. Previously, it generated the ChaosExperiment without caring whether the probe existed or not.
With the update of Litmus to version 3, probe has become mandatory. Previously, we solved this by enforcing it on the frontend. However, in order for Litmus to provide external APIs in the future, we need to add logic to check for the existence of a probe on the backend as well.
Therefore, if a probeRef does not exist during the process of parsing the manifest, we will call probeService.AddProbe
directly to automatically create a probe. Once generated, we add its annotation via the insertProbeRefAnnotation
function, create a ChaosExperiment, and return a 200 status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this code fit our logic? @S-ayanide
/**
* If probeRef is not present in the manifest then return the fault name
* where the ref is missing otherwise return undefined
*/
const probeWithoutAnnotation = await (
experimentHandler as KubernetesYamlService
)?.checkProbesInExperimentManifest(experiment?.manifest as KubernetesExperimentManifest);
/**
* Checks if probe metadata is already present in the manifest
*
* Returns true if probe metadata is present
*/
const doesProbeExists = await (experimentHandler as KubernetesYamlService)?.doesProbeMetadataExists(
experiment?.manifest as KubernetesExperimentManifest
);
if (doesProbeExists) {
showWarning(getString('probeMetadataExists'));
}
if (probeWithoutAnnotation && !doesProbeExists) {
showError(`${getString('probeInFault')} ${probeWithoutAnnotation} ${getString('probeNotAttachedToRef')}`);
return;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I'm aligned with the above explanation and was thinking the same thing myself. The above logic is also correct. However, according to this statement above
Therefore, if a probeRef does not exist during the process of parsing the manifest, we will call probeService.AddProbe directly to automatically create a probe. Once generated, we add its annotation via the insertProbeRefAnnotation function, create a ChaosExperiment, and return a 200 status.
I don't find the logic implemented in this PR where the parseProbeFromManifest
backend handler is handling this change or the insertProbeRefAnnotation
for example. I only see the probe service being added in this PR and being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the change, the file was so big github squished it 😿
…uto-generate-probe
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Proposed changes
Issue No. #4232
After Litmus version 3.0, Probes are mandatory to make Chaos Experiments.
If we want to launch the Chaos Experiment, We have to add annotations named 'probeRef' to ChaosEngine CRD.
In this PR, I added a feature: If our ChaosEngine CRD contains a probe variable, graphql-server automatically makes new probes and adds annotations to the YAML file.
This allows us to run a Chaos Experiment with a single YAML file. This means that it will be very easy to create experiments for Litmus via the API in the future.
Here's a flow.
Types of changes
What types of changes does your code introduce to Litmus? Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Dependency
Special notes for your reviewer: