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

Improve render status for porch #603

Open
liamfallon opened this issue Apr 8, 2024 · 1 comment
Open

Improve render status for porch #603

liamfallon opened this issue Apr 8, 2024 · 1 comment
Assignees
Labels
area/platform area/porch Porch related issues bug Something isn't working triaged
Milestone

Comments

@liamfallon
Copy link
Member

Original issue URL: kptdev/kpt#3637
Original issue user: https://github.com/ChristopherFry
Original issue created at: 2022-10-26T23:41:30Z
Original issue last updated at: 2022-11-15T21:43:33Z
Original issue body: Creating an umbrella issue to track requested improvements and issues with the Porch render status.

Porch does not set Exit Code field on render status

When kpt functions are executed through Porch, the ExitCode field in renderStatus.result and renderStatus.result.Items[] are always set to zero regardless if a function errors.

This can be reproduced by adding set-namespace mutator to the Kptfile, and setting configPath input to an non-ConfigMap and non-SetNamespace resource.

In Porch, the following is retuned:

kind: PackageRevisionResources
apiVersion: porch.kpt.dev/v1alpha1
...
status:
  renderStatus:
    result:
      kind: FunctionResultList
      apiVersion: kpt.dev/v1
      metadata:
        name: fnresults
        creationTimestamp:
      ExitCode: 0
      Items:
      - Image: gcr.io/kpt-fn/set-namespace:v0.4.1
        ExecPath: ''
        Stderr: ''
        ExitCode: 0
        Results:
        - message: unknown functionConfig Kind=Kptfile ApiVersion=kpt.dev/v1, expect
            `SetNamespace` or `ConfigMap`
          severity: error
    error: "fn.render: pkg .:\n\tpkg.render:\n\tpipeline.run: error: function failure"

whereas with kpt fn render the exit code field is set:

apiVersion: kpt.dev/v1
kind: FunctionResultList
metadata:
  name: fnresults
exitCode: 1
items:
  - image: gcr.io/kpt-fn/set-namespace:v0.4.1
    stderr: 'failed to evaluate function: error: function failure'
    exitCode: 1
    results:
      - message: unknown functionConfig Kind=Kptfile ApiVersion=kpt.dev/v1, expect `SetNamespace` or `ConfigMap`
        severity: error

The impact of this is that clients of Porch will not be able to use the ErrorCode field to determine if a function failed.

Porch does not set Stderr field on render status

Similar to the exit code field, Porch also does not set the Stderr field.

This can be reproduced by adding an invalid function to the Kptfile.

In Porch, the following is returned:

status:
  renderStatus:
    result:
      kind: FunctionResultList
      apiVersion: kpt.dev/v1
      metadata:
        name: fnresults
        creationTimestamp:
      ExitCode: 0
      Items:
      - Image: gcr.io/kpt-fn/set-namespace:v5.0.0
        ExecPath: ''
        Stderr: ''
        ExitCode: 0
        Results:
    error: "fn.render: pkg .:\n\tpkg.render:\n\tpipeline.run: func eval \"gcr.io/kpt-fn/set-namespace:v5.0.0\"
      failed: rpc error: code = Unknown desc = unable to get the grpc client to the
      pod for gcr.io/kpt-fn/set-namespace:v5.0.0: unable to get the entrypoint for
      gcr.io/kpt-fn/set-namespace:v5.0.0: GET https://gcr.io/v2/kpt-fn/set-namespace/manifests/v5.0.0:
      MANIFEST_UNKNOWN: Failed to fetch \"v5.0.0\" from request \"/v2/kpt-fn/set-namespace/manifests/v5.0.0\"."

Whereas with kpt fn render the stderr field is set:

apiVersion: kpt.dev/v1
kind: FunctionResultList
metadata:
  name: fnresults
exitCode: 1
items:
  - image: gcr.io/kpt-fn/set-namespace:v5.0.0
    stderr: |-
      docker: Error response from daemon: manifest for gcr.io/kpt-fn/set-namespace:v5.0.0 not found: manifest unknown: Failed to fetch "v5.0.0" from request "/v2/kpt-fn/set-namespace/manifests/v5.0.0".
      See 'docker run --help'.
    exitCode: 125

The impact of this is that clients of Porch cannot always iterate through the Items list to determine which function failed. Fortunately the error is returned on the top level error field however it's not associated with a function.

Exclude the Exec Path field from render results

Since functions are executed server side in Porch, this field will likely not be valuable to Porch clients and may leak internal implementation details.

status:
  renderStatus:
    result:
      kind: FunctionResultList
      apiVersion: kpt.dev/v1
      metadata:
        name: fnresults
        creationTimestamp:
      ExitCode: 0
      Items:
      - Image: gcr.io/kpt-fn/set-namespace:v5.0.0
        ExecPath: ''
        Stderr: ''
        ExitCode: 0
        Results: ...

Inconsistent field casing

The casing of the fields in FunctionResultList is inconsistent with Porch and kpt fn render. The fields in kpt fn render are camelCase whereas the same fields in Porch are mostly PascalCase.

This can be seen in the examples above.

Requesting FunctionResultList always includes all functions, including those not executed

This is a feature request for the FunctionResultList (for both Porch and kpt) to include all functions, including any functions that were not executed and/or skipped. Today you only know if a function is skipped by comparing the functions in the FunctionResultList to those in the Kptfile.

Requesting rendering does not stop if a validation function fails

This is a feature request to execute all Kptfile validation functions, even if one of the validation functions fails. An advantage of this is that this will allow us to show the user all validation failures at one time, not just the first validation failure.

Original issue comments:
Comment user: https://github.com/ChristopherFry
Comment created at: 2022-10-27T00:00:04Z
Comment last updated at: 2022-10-27T00:00:04Z
Comment body: cc @droot

Comment user: https://github.com/ChristopherFry
Comment created at: 2022-10-28T15:49:00Z
Comment last updated at: 2022-10-28T15:49:00Z
Comment body: > Inconsistent field casing
The casing of the fields in FunctionResultList is inconsistent with Porch and kpt fn render. The fields in kpt fn render are camelCase whereas the same fields in Porch are mostly PascalCase.

This is resolved with #3638.

@tliron tliron transferred this issue from nephio-project/porch-issue-transfer Apr 23, 2024
@liamfallon liamfallon added bug Something isn't working and removed enhancement New feature or request labels May 17, 2024
@liamfallon liamfallon added this to the R3 milestone May 17, 2024
@liamfallon
Copy link
Member Author

Triaged
Triage Comment: Need to look at it to see how serious it is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform area/porch Porch related issues bug Something isn't working triaged
Projects
Status: Todo
Development

No branches or pull requests

3 participants