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

The execution of failureCondition is inconsistent with the docs #11217

Open
2 of 3 tasks
KingsonKai opened this issue Jun 15, 2023 · 8 comments · May be fixed by #13878
Open
2 of 3 tasks

The execution of failureCondition is inconsistent with the docs #11217

KingsonKai opened this issue Jun 15, 2023 · 8 comments · May be fixed by #13878
Labels
area/docs Incorrect, missing, or mistakes in docs area/templates/resource P3 Low priority type/bug

Comments

@KingsonKai
Copy link

Pre-requisites

  • I have double-checked my configuration
  • I can confirm the issues exists when I tested with :latest
  • I'd like to contribute the fix myself (see contributing guide)

What happened/what you expected to happen?

My failureCondition is defined as follows
successCondition is phase == recover AND status == failed
failureCondition is phase == recover AND status == success
image

The actual execution result of the object is phase == recover AND status == success
image

The official document describes that comma-separated writing is an AND relationship:
image

In the workflow result, it seems that only one of the conditions is judged after execution. It seems that the execution logic of the judgment condition is an OR relationship, not the expected AND relationship.
image

Version

v3.4.8

Paste a small workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflows that uses private images.

My resource object is a CRD, not a regular k8s resource type

Logs from the workflow controller

kubectl logs -n argo deploy/workflow-controller | grep ${workflow}

Logs from in your workflow's wait container

kubectl logs -n argo -c wait -l workflows.argoproj.io/workflow=${workflow},workflow.argoproj.io/phase!=Succeeded
@sarabala1979
Copy link
Member

@KingsonKai can you provide the full workflow yaml which failed ?

@sarabala1979 sarabala1979 added P3 Low priority P1 High priority. All bugs with >=5 thumbs up that aren’t P0, plus: Any other bugs deemed high priority labels Jun 15, 2023
@KingsonKai
Copy link
Author

KingsonKai commented Jun 20, 2023

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: network-cpu-workflow
  namespace: chaosmeta
spec:
  serviceAccountName: chaosmeta-inject-controller-manager
  entrypoint: main
  templates:
    - name: experiment-inject
      inputs:
        parameters:
        - name: experiment
      resource:
        action: create
        failureCondition: status.status == failed
        successCondition: status.phase == recover,status.status == success
        manifest: "{{inputs.parameters.experiment}}"
    - name: raw-suspend
      inputs:
        parameters:
        - name: time
      suspend:
        duration: "{{inputs.parameters.time}}"
    - name: experiment-recover
      inputs:
        parameters:
        - name: experiment
      resource:
        action: patch
        failureCondition: status.phase == recover,status.status == failed
        successCondition: status.phase == recover,status.status == success
        mergeStrategy: json
        flags:
        - experiments.inject.chaosmeta.io
        - "{{inputs.parameters.experiment}}"
        manifest: |
          - op: replace
            path: /spec/targetPhase
            value: recover
    - name: main
      dag:
        tasks:
        - name: before-wait
          template: raw-suspend
          arguments:
            parameters: [{name: time, value: "5s"}]
        - name: pod-network-experiment-inject
          hooks:
            exit:
              template: experiment-recover
              arguments:
                parameters: [{name: experiment, value: "pod-network-experiment"}]
          dependencies: [before-wait]
          template: experiment-inject
          arguments:
            parameters: 
              - name: experiment
                value: |
                  apiVersion: inject.chaosmeta.io/v1alpha1
                  kind: Experiment
                  metadata:
                    name: pod-network-experiment
                    namespace: chaosmeta
                  spec:
                    scope: pod
                    targetPhase: inject
                    experiment:
                      target: network
                      fault: delay
                      duration: 5m
                      args:
                        - key: interface
                          value: 'eth0'
                          valueType: string
                        - key: latency
                          value: '2s'
                          valueType: string
                    selector:
                      - namespace: default
                        name:
                          - nginx-75c57b6fd7-hldwd
        - name: inject-wait
          dependencies: [before-wait]
          template: raw-suspend
          arguments:
            parameters: [{name: time, value: "30s"}]
        - name: inject-wait-2
          dependencies: [inject-wait]
          template: raw-suspend
          arguments:
            parameters: [{name: time, value: "5s"}]
        - name: after-wait
          dependencies: [pod-network-experiment-inject,inject-wait-2]
          template: raw-suspend
          arguments:
            parameters: [{name: time, value: "5s"}]

@KingsonKai
Copy link
Author

KingsonKai commented Jun 20, 2023

this condition can't work: templates.experiment-recover.resource.failureCondition
This is the CRD of the chaos engineering project chaosmeta. When I change the targetPhase to recover, the state will change, but the state of this CRD is composite, and two fields need to be judged at the same time.

If you want to reproduce completely, you need to install chaosmeta. It may be too much work for you. You can follow the chaosmeta documentation to install it with one click.

I feel that other CRDs with composite states should also have such bugs. If you have a ready-made CRD to try, please try to reproduce it

this is the target CRD:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  annotations:
    controller-gen.kubebuilder.io/version: v0.11.1
  name: experiments.inject.chaosmeta.io
spec:
  conversion:
    strategy: Webhook
    webhook:
      clientConfig:
        service:
          name: chaosmeta-inject-webhook-service
          namespace: chaosmeta
          path: /convert
      conversionReviewVersions:
        - v1
  group: inject.chaosmeta.io
  names:
    kind: Experiment
    listKind: ExperimentList
    plural: experiments
    singular: experiment
  scope: Namespaced
  versions:
    - name: v1alpha1
      schema:
        openAPIV3Schema:
          description: Experiment is the Schema for the experiments API
          properties:
            apiVersion:
              description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
              type: string
            kind:
              description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
              type: string
            metadata:
              type: object
            spec:
              description: ExperimentSpec defines the desired state of Experiment
              properties:
                experiment:
                  properties:
                    args:
                      items:
                        properties:
                          key:
                            type: string
                          value:
                            type: string
                          valueType:
                            type: string
                        required:
                          - key
                          - value
                        type: object
                      type: array
                    duration:
                      description: Duration support "h", "m", "s"
                      type: string
                    fault:
                      type: string
                    target:
                      type: string
                  required:
                    - fault
                    - target
                  type: object
                rangeMode:
                  properties:
                    type:
                      description: 'Type Optional: all、percent、count'
                      type: string
                    value:
                      type: integer
                  required:
                    - type
                  type: object
                scope:
                  description: 'Scope Optional: node, pod. type of experiment object'
                  type: string
                selector:
                  description: Selector The internal part of unit is "AND", and the external part is "OR" and de-duplication
                  items:
                    properties:
                      ip:
                        items:
                          type: string
                        type: array
                      label:
                        additionalProperties:
                          type: string
                        type: object
                      name:
                        items:
                          type: string
                        type: array
                      namespace:
                        type: string
                    type: object
                  type: array
                targetPhase:
                  type: string
              required:
                - experiment
                - scope
                - targetPhase
              type: object
            status:
              description: ExperimentStatus defines the observed state of Experiment
              properties:
                createTime:
                  type: string
                detail:
                  properties:
                    inject:
                      items:
                        properties:
                          backup:
                            type: string
                          injectObjectName:
                            type: string
                          message:
                            type: string
                          startTime:
                            type: string
                          status:
                            type: string
                          uid:
                            description: InjectObjectInfo string     `json:"injectObjectInfo,omitempty"`
                            type: string
                          updateTime:
                            type: string
                        type: object
                      type: array
                    recover:
                      items:
                        properties:
                          backup:
                            type: string
                          injectObjectName:
                            type: string
                          message:
                            type: string
                          startTime:
                            type: string
                          status:
                            type: string
                          uid:
                            description: InjectObjectInfo string     `json:"injectObjectInfo,omitempty"`
                            type: string
                          updateTime:
                            type: string
                        type: object
                      type: array
                  type: object
                message:
                  type: string
                phase:
                  type: string
                status:
                  type: string
                updateTime:
                  type: string
              required:
                - createTime
                - detail
                - message
                - phase
                - status
                - updateTime
              type: object
          type: object
      served: true
      storage: true
      subresources:
        status: {}

@terrytangyuan
Copy link
Member

It’s label selector basically https://github.com/argoproj/argo-workflows/blob/master/workflow/executor/resource.go#L186

but complicated use cases may not be supported yet

@stale

This comment was marked as resolved.

@stale stale bot added the problem/stale This has not had a response in some time label Sep 17, 2023
@terrytangyuan terrytangyuan removed the problem/stale This has not had a response in some time label Sep 20, 2023
@agilgur5 agilgur5 added P3 Low priority area/templates/resource and removed P3 Low priority P1 High priority. All bugs with >=5 thumbs up that aren’t P0, plus: Any other bugs deemed high priority labels Oct 6, 2023
@agilgur5
Copy link
Contributor

agilgur5 commented Oct 8, 2023

Hmm #7143 (comment) says that failureCondition may indeed be different

@agilgur5 agilgur5 added the area/docs Incorrect, missing, or mistakes in docs label Oct 8, 2023
@KingsonKai
Copy link
Author

Does it mean that the two conditions separated by commas are "AND" logic in successCondition, but not in failureCondition?

@agilgur5
Copy link
Contributor

Yes, successCondition seems to be an "AND" while failureCondition is an "OR". The match code linked from the above comment is indeed different between the two.

Would you like to file a PR to fix the docs?

@agilgur5 agilgur5 changed the title The condition judgment execution result of the failureCondition is inconsistent with the document description The execution of failureCondition is inconsistent with the docs Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Incorrect, missing, or mistakes in docs area/templates/resource P3 Low priority type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants