[go: nahoru, domu]

Skip to content
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

Merged
merged 13 commits into from
Feb 7, 2024

Conversation

namkyu1999
Copy link
Member
@namkyu1999 namkyu1999 commented Jan 3, 2024

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.

  1. Upload a YAML file to launch a Chaos Experiment.
Screenshot 2024-01-04 at 4 13 18 PM
  1. When launching a Chaos Experiment, probes are automatically generated.
Screenshot 2024-01-04 at 4 28 58 PM Screenshot 2024-01-04 at 4 28 50 PM
  1. The server made changes to a YAML file (Add annotations)
Screenshot 2024-01-04 at 4 29 23 PM

Types of changes

What types of changes does your code introduce to Litmus? Put an x in the boxes that apply

  • New feature (non-breaking change which adds functionality)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices applies)

Checklist

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.

  • I have read the CONTRIBUTING doc
  • I have signed the commit for DCO to be passed.
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added necessary documentation (if appropriate)

Dependency

  • Please add the links to the dependent PR need to be merged before this (if any).

Special notes for your reviewer:

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>
@namkyu1999 namkyu1999 changed the title feat: auto generate probes feat: Automatically generate probes when creating a chaos experiment using a YAML file. Jan 4, 2024
@namkyu1999 namkyu1999 marked this pull request as ready for review January 4, 2024 07:41
@namkyu1999 namkyu1999 self-assigned this Jan 4, 2024
Copy link
Member
@SahilKr24 SahilKr24 left a 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

Copy link
Member
@hrishavjha hrishavjha left a 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 🚀

Copy link
Member
@S-ayanide S-ayanide left a 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) {
Copy link
Member

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

Copy link
Member Author

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.
image

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.

Copy link
Member Author

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;
}

Copy link
Member

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.

Copy link
Member

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 😿

@S-ayanide S-ayanide added DO NOT MERGE area/resilience-probes Related to Resilience Probes labels Jan 5, 2024
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>
@S-ayanide S-ayanide merged commit 3e858b2 into litmuschaos:master Feb 7, 2024
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/chaoscenter area/resilience-probes Related to Resilience Probes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants