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

Inconsistent behaviour by kpt if there are 2 repository CR pointing to same git repo but different directory inside the repoo #628

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

Comments

@liamfallon
Copy link
Member

Original issue URL: kptdev/kpt#3935
Original issue user: https://github.com/Cbkhare
Original issue created at: 2023-04-27T16:45:50Z
Original issue last updated at: 2023-04-29T00:47:54Z
Original issue body: Issue:
kpt alpha rpkg init is working incorrectly when I apply 2 Repository CR in the system which are pointing to the same private repository.

Details of my 2 CRs

$ cat test_ct_repo.yaml
apiVersion: config.porch.kpt.dev/v1alpha1
kind: Repository
metadata:
  name: test-catalogue
  namespace: test
spec:
  description: test-catalogue
  content: Package
  deployment: false
  type: git
  git:
    repo: https://github.com/********/**********************.git
    directory: /
    branch: main
    createBranch: true
    secretRef:
      name: cred
$ cat test_ct_repo_out.yaml
apiVersion: config.porch.kpt.dev/v1alpha1
kind: Repository
metadata:
  name: test-catalogue-out
  namespace: test-out
spec:
  description: test-catalogue-out
  content: Package
  deployment: false
  type: git
  git:
    repo: https://github.com/********/**********************.git
    directory: test-pkg-out
    branch: main
    createBranch: true
    secretRef:
      name: cred

Notes:

  1. In both of the above CR, repo path is same. Since, its a private repo, I have replaced the username and repo name with *.
  2. Namespace and CR name is different.
  3. directory is different.
  4. secret was created in both the namespaces.
  5. test-catalogue was applied first. And then test-catalogue-out
$ kubectl get repositories -A
NAMESPACE   NAME                 TYPE   CONTENT   DEPLOYMENT   READY   ADDRESS
test-out    test-catalogue-out   git    Package   false        True    https://github.com/******/*********************.git
test        test-catalogue       git    Package   false        True    https://github.com/******/*********************.git

Below are my observations after Creating the above CRs.

  1. Incorrect revision creation.
$ kpt alpha rpkg init test-pkg11 --workspace=v1 --repository=test-catalogue --namespace=test
NAMESPACE   NAME                                                           PACKAGE     WORKSPACENAME   REVISION   LATEST   LIFECYCLE   REPOSITORY
test-out    test-catalogue-out-233a4ca4388204556f5bceff99a9799d1a86ed1a   test-pkg11    v1              v1         false     Draft   test-catalogue-out

As you can notice in the above command, the namespace and repository passed are test and test-catalogue respectively. Yet, the revision which was created had the details of the repository and namespace of the test-catalogue-out Repository CR.

After facing this issue, I deleted and re-created the porch. And recreated the 2 repo CR as above, this time I was getting a different issue, as below.

  1. Not able to init the package.
$ kpt alpha rpkg init test-pkg-repo1 --workspace=v1 --repository=test-catalogue --namespace=test
Error: Internal error occurred: create not allowed while custom resource definition is terminating

Original issue comments:
Comment user: https://github.com/mortent
Comment created at: 2023-04-28T21:28:39Z
Comment last updated at: 2023-04-28T21:28:39Z
Comment body: @Cbkhare Thanks for reporting this and the detailed description. I am able to reproduce weird behavior here, but I haven't been able to fully understand what is going on yet.

I see that one of the repositories point to the root of the git repo, while the other point to a subdirectory. This probably isn't a good way to do this, since Porch assumes each package is a direct subdirectory of the directory provided for a repository. So for the test-catalogue repository, you should probably set the directory property to something like test-pkg. However, I'm still seeing odd behavior when I do this, so I will look into it.

Comment user: https://github.com/mortent
Comment created at: 2023-04-29T00:47:53Z
Comment last updated at: 2023-04-29T00:47:53Z
Comment body: So dug deeper into this. This is actually a rather significant bug in Porch, where some parts of the code doesn't account for the fact that it might be multiple Repository objects pointing to the same repository, but with different settings (which probably is a pretty common case).
The cache that keeps track of repositories and sets up the sync between git/oci and Porch, uses the repository URL as the key for the inventory of repositories:

https://github.com/GoogleContainerTools/kpt/blob/239a679abe979e058e53fbdcb60a456060cda81c/porch/pkg/cache/cache.go#L120-L125

This is reasonable, as we don't want to poll the same repository multiple times in parallel. But the internal representation of a repository, which has a one-to-one mapping to a git Repository due to the repo url being the key, also includes information specific to the KRM Repository CR:

https://github.com/GoogleContainerTools/kpt/blob/239a679abe979e058e53fbdcb60a456060cda81c/porch/pkg/cache/cache.go#L133-L141

This is something we need to fix.

@tliron tliron transferred this issue from nephio-project/porch-issue-transfer Apr 23, 2024
@efiacor efiacor self-assigned this May 16, 2024
@efiacor
Copy link
Contributor

efiacor commented May 16, 2024

@liamfallon
Copy link
Member Author

See also #626

@efiacor
Copy link
Contributor

efiacor commented May 21, 2024

This seems to have been addressed in kptdev/kpt#4097
Some issues regarding polling of redundant repos still exist due to missing watch.Modify Impl - https://github.com/nephio-project/porch/blob/main/pkg/registry/porch/background.go#L136 but are tolerable.

Closing this with a view to address the Repo CR update/modify in a future release.

@efiacor efiacor closed this as completed May 21, 2024
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
Projects
Status: Done
Development

No branches or pull requests

2 participants