feat: Probes/Added backend implementation for probes as resource #4116

Saranya-jena · 2023-08-07T13:10:05Z

Proposed changes

Added Resilience Probes as resource:
Resilience Probes are reusable pluggable checks that could be used with any chaos experiment, this features promotes minification of YAML by generating all probe metadata in the backend and only providing a reference in chaos engine annotations. Adhering to the 'Write once, Use anywhere' paradigm, this approach promotes the reuse of the same/new probe instead of creating a brand new one each time a chaos experiment is executed/edited. Currently, it supports HTTP, CMD, Kubernetes and Prometheus Probes.

Summarize your changes here to communicate with the maintainers and make sure to put the link to that issue

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: Saranya-jena <saranya.jena@harness.io>

S-ayanide

Left a few comments, PTAL

S-ayanide · 2023-08-09T03:55:13Z

chaoscenter/graphql/definitions/shared/probe.graphqls

+"""
+Types of AzureDataDiskCachingModes
+"""
+enum AzureDataDiskCachingMode {
+  None
+  ReadOnly
+  ReadWrite
+}
+
+"""
+Types of AzureDataDiskKind
+"""
+enum AzureDataDiskKind {
+  Shared
+  Dedicated
+  Managed
+}


These can be removed

S-ayanide · 2023-08-09T03:56:00Z

chaoscenter/graphql/definitions/shared/probe.graphqls

+"""
+Type for HostPath Volume
+"""
+enum HostPathType {
+  DirectoryOrCreate
+  Directory
+  FileOrCreate
+  File
+  Socket
+  CharDevice
+  BlockDevice
+}
+
+"""
+StorageMedium defines ways that storage can be allocated to a volume.
+"""
+enum StorageMedium {
+  Memory
+  HugePages
+}
+
+"""
+MountPropagationMode describes mount propagation.
+"""
+enum MountPropagationMode {
+  None
+  HostToContainer
+  Bidirectional
+}
+
+"""
+Quantity is a fixed-point representation of a number.
+It provides convenient marshaling/unmarshaling in JSON and YAML,
+"""
+enum Quantity {
+  i
+  d
+  s
+  DecimalExponent
+  BinarySI
+  DecimalSI
+}


These types can be removed too

S-ayanide · 2023-08-09T03:57:00Z

chaoscenter/graphql/definitions/shared/probe.graphqls

+"""
+EvaluationWindow is the time period for which the SLO probe will work
+"""
+type EvaluationWindow {
+  """
+  Start time of evaluation
+  """
+  evaluationStartTime: Int
+  """
+  End time of evaluation
+  """
+  evaluationEndTime: Int
+}
+
+"""
+Defines the input properties of EvaluationWindow
+"""
+input EvaluationWindowInput {
+  """
+  Start time of evaluation
+  """
+  evaluationStartTime: Int
+  """
+  End time of evaluation
+  """
+  evaluationEndTime: Int
+}


These are SLO properties and can be removed

S-ayanide · 2023-08-09T03:59:52Z

chaoscenter/graphql/definitions/shared/probe.graphqls

+  """
+  User who has updated the experiment
+  """
+  updatedBy: UserDetails!


updatedBy would be optional for ExecutedByExperiment

updatedBy: UserDetails

Otherwise it would throw error like Probe must not be null if the updatedBy is set to "" by any other component

S-ayanide · 2023-08-09T04:07:27Z

chaoscenter/graphql/server/graph/probe.resolvers.go

+	logrus.WithFields(logFields).Info("request received to get probe YAML")
+
+	err := authorization.ValidateRole(ctx, projectID,
+		authorization.MutationRbacRules[authorization.AddProbe],


Since this is GetProbeYAML this permission could be mapped to GetProbe authorization permission

S-ayanide · 2023-08-09T04:15:44Z

chaoscenter/graphql/server/pkg/probe/types.go

+// CmdProbeInputs contains all the inputs required for cmd probe
+// Duplicate type of v1alpha1.CmdProbeInputs because of extra nil pointer reference to Source object
+// as it is optional for CMD Probe Input
+type CmdProbeInputs struct {
+	// Command need to be executed for the probe
+	Command string `json:"command,omitempty"`
+	// Comparator check for the correctness of the probe output
+	Comparator v1alpha1.ComparatorInfo `json:"comparator,omitempty"`
+	// The source where we have to run the command
+	// It will run in inline(inside experiment itself) mode if source is nil
+	Source *v1alpha1.SourceDetails `json:"source,omitempty"`
+}


This can be removed and type CMDProbeAttributes struct can have this instead

CmdProbeInputs v1alpha1.CmdProbeInputs `json:"cmdProbe/inputs,omitempty"`

S-ayanide · 2023-08-09T04:16:48Z

chaoscenter/graphql/server/pkg/probe/types.go

+// HTTPProbeInputs contains all the inputs required for http probe
+type HTTPProbeInputs struct {
+	// URL which needs to curl, to check the status
+	URL string `json:"url,omitempty"`
+	// InsecureSkipVerify flag to skip certificate checks
+	InsecureSkipVerify bool `json:"insecureSkipVerify,omitempty"`
+	// Method define the http method, it can be get or post
+	Method v1alpha1.HTTPMethod `json:"method,omitempty"`
+	// ResponseTimeout Flag to hold the flag to response timeout for the httpProbe
+	ResponseTimeout int `json:"responseTimeout,omitempty"`
+}
+
+// HTTPMethod define the http method details
+type HTTPMethod struct {
+	Get  *v1alpha1.GetMethod  `json:"get,omitempty"`
+	Post *v1alpha1.PostMethod `json:"post,omitempty"`
+}


These two types can be removed and type HTTPProbeAttributes struct can have this instead

HTTPProbeInputs v1alpha1.HTTPProbeInputs `json:"httpProbe/inputs,omitempty"`

This was done since operator was old and didn't support the correct omitempty fields. Since its synced now we can directly reference the type as a single source of truth

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

gdsoumya · 2023-08-10T11:22:54Z

chaoscenter/graphql/server/graph/probe.resolvers.go

+	}
+
+	p := probe.NewProbeRepository(projectID)
+	response, err := p.UpdateProbe(ctx, request)


Probably needs same probe validity checks as done in addProbe

I think we are still missing the part where we validate the probe type and the probe properties set match, for eg. if a user updates the probe but removes the probe properties that will be a problem and should be caught.

gdsoumya · 2023-08-10T11:25:11Z

chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler.go

@@ -1548,6 +1573,8 @@ func (c *ChaosExperimentHandler) RunChaosWorkFlow(ctx context.Context, projectID
 			Completed:       false,
 			ResiliencyScore: &resScore,
 			ExecutionData:   string(parsedData),
+			Probes:          probes,
+			//TODO add probes


Stale comment? I see probe is already there. If it's something else needs a better descriptive comment

gdsoumya · 2023-08-10T11:34:06Z

@Saranya-jena as external maintainers don't have the full context of the feature might be good to add some PR description that gives the full context or create public issues that describe the need for the feature and its goals. Without the full context, it will be difficult to do a proper review.

Saranya-jena · 2023-08-11T07:00:16Z

Hey @gdsoumya thanks for suggestion, I have updated the description!

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

gdsoumya · 2023-08-11T08:42:28Z

chaoscenter/graphql/server/graph/probe.resolvers.go

+	}
+
+	p := probe.NewProbeRepository(projectID)
+	response, err := p.UpdateProbe(ctx, request)


I think we are still missing the part where we validate the probe type and the probe properties set match, for eg. if a user updates the probe but removes the probe properties that will be a problem and should be caught.

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

Saranya-jena · 2023-08-11T10:52:20Z

Added nil checks for probe properties, PTAL @gdsoumya @S-ayanide

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

…muschaos#4116) * Added probes graphql schema in backend Signed-off-by: Saranya-jena <saranya.jena@harness.io> * Added mongo schema for probes and updated experiment schema Signed-off-by: Saranya-jena <saranya.jena@harness.io> * Added handler functions for probes Signed-off-by: Saranya-jena <saranya.jena@harness.io> * Updated operator version in subscriber Signed-off-by: Saranya-jena <saranya.jena@harness.io> * removed redundant types in GQL n mongo schema Signed-off-by: Saranya-jena <saranya.jena@harness.io> * Added checks for updateProbe Signed-off-by: Saranya-jena <saranya.jena@harness.io> * Added nil checks for update probe Signed-off-by: Saranya-jena <saranya.jena@harness.io> * fixed imports Signed-off-by: Saranya-jena <saranya.jena@harness.io> * resolved conflicts and errors Signed-off-by: Saranya-jena <saranya.jena@harness.io> --------- Signed-off-by: Saranya-jena <saranya.jena@harness.io>

Saranya-jena added 4 commits August 7, 2023 18:34

Added probes graphql schema in backend

19661aa

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

Added mongo schema for probes and updated experiment schema

7c64911

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

Added handler functions for probes

e8028f2

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

Updated operator version in subscriber

9ba6452

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

Saranya-jena requested review from S-ayanide, amityt, Jonsy13 and SarthakJain26 August 7, 2023 13:10

Saranya-jena self-assigned this Aug 7, 2023

updated branch

4fd8542

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

Saranya-jena requested a review from gdsoumya August 7, 2023 13:14

S-ayanide requested changes Aug 9, 2023

View reviewed changes

removed redundant types in GQL n mongo schema

635e82d

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

gdsoumya requested changes Aug 10, 2023

View reviewed changes

S-ayanide approved these changes Aug 11, 2023

View reviewed changes

Added checks for updateProbe

90fdb5d

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

gdsoumya requested changes Aug 11, 2023

View reviewed changes

Saranya-jena added 2 commits August 11, 2023 16:19

Added nil checks for update probe

3a9312b

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

fixed imports

964723e

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

gdsoumya approved these changes Aug 11, 2023

View reviewed changes

Saranya-jena added 2 commits August 23, 2023 12:58

updated branch

c32dc74

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

resolved conflicts and errors

54a50ce

Signed-off-by: Saranya-jena <saranya.jena@harness.io>

Saranya-jena merged commit 5d4e610 into litmuschaos:master Aug 23, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Probes/Added backend implementation for probes as resource #4116

feat: Probes/Added backend implementation for probes as resource #4116

Saranya-jena commented Aug 7, 2023 •

edited by S-ayanide

Loading

S-ayanide left a comment

S-ayanide Aug 9, 2023

S-ayanide Aug 9, 2023

S-ayanide Aug 9, 2023

S-ayanide Aug 9, 2023

S-ayanide Aug 9, 2023

S-ayanide Aug 9, 2023

S-ayanide Aug 9, 2023

gdsoumya Aug 10, 2023

gdsoumya Aug 11, 2023

gdsoumya Aug 10, 2023

gdsoumya commented Aug 10, 2023

Saranya-jena commented Aug 11, 2023

gdsoumya Aug 11, 2023

Saranya-jena commented Aug 11, 2023

feat: Probes/Added backend implementation for probes as resource #4116

feat: Probes/Added backend implementation for probes as resource #4116

Conversation

Saranya-jena commented Aug 7, 2023 • edited by S-ayanide Loading

Proposed changes

Types of changes

Checklist

Dependency

Special notes for your reviewer:

S-ayanide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdsoumya commented Aug 10, 2023

Saranya-jena commented Aug 11, 2023

Choose a reason for hiding this comment

Saranya-jena commented Aug 11, 2023

Saranya-jena commented Aug 7, 2023 •

edited by S-ayanide

Loading