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

Porch: PackageVariant should set a readinessgate #615

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

Porch: PackageVariant should set a readinessgate #615

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

Comments

@liamfallon
Copy link
Member

Original issue URL: kptdev/kpt#3979
Original issue user: https://github.com/johnbelamaric
Original issue created at: 2023-06-01T01:36:52Z
Original issue last updated at: 2023-07-06T21:56:34Z
Original issue body: ### Expected behavior
If PackageVariant fails to inject or otherwise properly mutate the Draft it creates, we need to prevent the package from being approved. This could be done by adding a False condition and readinessGate immediately after cloning (or better yet, implementing a porch server feature that allows that to be done atomically).

Actual behavior

Because we cannot atomically "clone and apply PV mutations", the actual package revision process happens in two independent stages: clone, then apply mutations. If applying the mutations fails, the newly cloned Draft is still there, and unless you check the PV conditions, it looks fine and can be merged (in fact the auto-approval controller we built for Nephio goes ahead and approves it, since it only inspect the package revision, and doesn't know where it came from).

Original issue comments:
Comment user: https://github.com/johnbelamaric
Comment created at: 2023-07-06T21:56:34Z
Comment last updated at: 2023-07-06T21:56:34Z
Comment body: As a workaround for this, the approval controller now checks the PV readiness status, and so do some of our other controllers. But that wouldn't help a human user.

@tliron tliron transferred this issue from nephio-project/porch-issue-transfer Apr 23, 2024
@efiacor efiacor added this to R4 May 14, 2024
@liamfallon liamfallon removed this from R4 May 21, 2024
@liamfallon liamfallon added this to R4 May 21, 2024
@liamfallon
Copy link
Member Author

Triage Comments:
In the case of a PackageVariant, it will clone a package that has no readiness gate in it and it seems fine. Then as the package moves through the pipeline, there is no way to track whether of how much of the mutation in the pipeline has been executed. The PacakgeRevision creation is a 2 step process:

  1. Clone the raw Package
  2. Mutate the package with a pipeline or otherwise

The current implementation sets the package as ready after step 1.

To be considered in rearchitecture.

@JamesMcDermott JamesMcDermott self-assigned this Dec 9, 2024
@JamesMcDermott JamesMcDermott moved this to In Progress in R4 Dec 10, 2024
JamesMcDermott added a commit to Nordix/porch that referenced this issue Dec 10, 2024
- PackageVariant controller now uses a readiness gate to allow
  a PackageRevision to complete its mutation pipeline before it
  is allowed to be proposed/approved
- refactored conversion of Kptfiles to YAML since readiness condition
  information is stored in the package Kptfile
  - unified all cases to the same kyaml/yaml-based method
    (KptFile.ToYamlString() and ToYamlString(*fn.KubeObject))
  - this ensures consistency in the YAML (indentation, field order etc.)
  - and reduces the chances of Git conflicts when setting and updating
    readiness conditions
- added more info to error message in case of Git conflict when applying
  a patch

nephio-project/nephio#615
JamesMcDermott added a commit to Nordix/porch that referenced this issue Dec 12, 2024
- previous solution broke existing functionality to copy comments between Kptfile objects
  - resulting in failing unit tests
    - fix for that brought back the Git conflicts caused by indentation, field order etc.
    - re-refactored conversion of Kptfiles to YAML
      - KubeObject-related conversion methods (KubeObject/YAML and YAML/KubeObject)
        now live out in the util package as they aren't Kptfile-specific any more

nephio-project/nephio#615
JamesMcDermott added a commit to Nordix/porch that referenced this issue Dec 17, 2024
- PackageVariant controller now uses a readiness gate to allow
  a PackageRevision to complete its mutation pipeline before it
  is allowed to be proposed/approved
- refactored conversion of Kptfiles to YAML since readiness condition
  information is stored in the package Kptfile
  - unified all cases to the same kyaml/yaml-based method
    (KptFile.ToYamlString() and ToYamlString(*fn.KubeObject))
  - this ensures consistency in the YAML (indentation, field order etc.)
  - and reduces the chances of Git conflicts when setting and updating
    readiness conditions
- added more info to error message in case of Git conflict when applying
  a patch

nephio-project/nephio#615
JamesMcDermott added a commit to Nordix/porch that referenced this issue Dec 17, 2024
- previous solution broke existing functionality to copy comments between Kptfile objects
  - resulting in failing unit tests
    - fix for that brought back the Git conflicts caused by indentation, field order etc.
    - re-refactored conversion of Kptfiles to YAML
      - KubeObject-related conversion methods (KubeObject/YAML and YAML/KubeObject)
        now live out in the util package as they aren't Kptfile-specific any more

nephio-project/nephio#615
JamesMcDermott added a commit to Nordix/porch that referenced this issue Dec 17, 2024
- PackageVariant controller now uses a readiness gate to allow
  a PackageRevision to complete its mutation pipeline before it
  is allowed to be proposed/approved
- refactored conversion of Kptfiles to YAML since readiness condition
  information is stored in the package Kptfile
  - unified all cases to the same kyaml/yaml-based method
    (KptFile.ToYamlString() and ToYamlString(*fn.KubeObject))
  - this ensures consistency in the YAML (indentation, field order etc.)
  - and reduces the chances of Git conflicts when setting and updating
    readiness conditions
- added more info to error message in case of Git conflict when applying
  a patch

nephio-project/nephio#615
JamesMcDermott added a commit to Nordix/porch that referenced this issue Dec 17, 2024
- previous solution broke existing functionality to copy comments between Kptfile objects
  - resulting in failing unit tests
    - fix for that brought back the Git conflicts caused by indentation, field order etc.
    - re-refactored conversion of Kptfiles to YAML
      - KubeObject-related conversion methods (KubeObject/YAML and YAML/KubeObject)
        now live out in the util package as they aren't Kptfile-specific any more

nephio-project/nephio#615
JamesMcDermott added a commit to Nordix/porch that referenced this issue Dec 20, 2024
… pipeline-passed condition

- review comments to increase applicability of pipeline-passed readiness condition
  - to enable this: if an incoming PackageRevision update changes only that
    readiness condition, detect that, skip the pipeline/rendering/etc., and
    only update the Kptfile with the readiness condition update
- other tidy-up/wording comments

nephio-project/nephio#615
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: In Progress
Development

No branches or pull requests

3 participants