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

feat!: central cache with symlinks #274

Merged
merged 59 commits into from
May 10, 2024
Merged

feat!: central cache with symlinks #274

merged 59 commits into from
May 10, 2024

Conversation

Zebradil
Copy link
Member

@Zebradil Zebradil commented Apr 21, 2024

This PR is built on top of #257 and changes it to use symlinks for maintaining the central cache. Tests pass but I might have missed something (also, there are no new tests for new functions, it's TBD for later).

TODO:

  • cleanup old links from the vendor directory
  • exclude content's path from cache name generation

Create tasks to:

fritzduchardt and others added 30 commits March 24, 2024 17:20
Every vendir directory is processed individually and placed into a central cache.
@fritzduchardt
Copy link
Collaborator

Hi @Zebradil, I would like to talk about this one in a call. To give you a head start:

Does not render fully:

apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
- path: charts/
  contents:
  - path: cert-manager
    lazy: true
    helmChart:
      name: cert-manager
      version: v1.14.2
      repository:
        url: https://charts.jetstack.io
- path: charts/
  contents:
  - path: multus-cni
    lazy: true
    helmChart:
      name: multus-cni
      version: 1.1.10
      repository:
        url: https://charts.bitnami.com/bitnami

Does not work at all:

apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
- path: charts/
  contents:
  - path: cert-manager
    lazy: true
    helmChart:
      name: cert-manager
      version: v1.14.2
      repository:
        url: https://charts.jetstack.io
  - path: multus-cni
    lazy: true
    helmChart:
      name: multus-cni
      version: 1.1.10
      repository:
        url: https://charts.bitnami.com/bitnami

@Zebradil
Copy link
Member Author

Zebradil commented May 1, 2024

Thank you for the examples. I'll take a closer look later today. But right now I can tell that the second example doesn't work because I explicitly restrict vendir configs to have only a single content per directory.

@Zebradil
Copy link
Member Author

Zebradil commented May 1, 2024

@fritzduchardt The first example is not valid vendir config:

$ cat <<EOF | vendir sync -f-
apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
  - path: charts/
    contents:
      - path: cert-manager
        lazy: true
        helmChart:
          name: cert-manager
          version: v1.14.2
          repository:
            url: https://charts.jetstack.io
  - path: charts/
    contents:
      - path: multus-cni
        lazy: true
        helmChart:
          name: multus-cni
          version: 1.1.10
          repository:
            url: https://charts.bitnami.com/bitnami
EOF
vendir: Error: Parsing resource config '-':
  Unmarshaling config:
    Validating config:
      Expected to not manage overlapping paths: 'charts/' and 'charts/'

If I change the config in the following way, sync and render work correctly:

--- orig.yaml	2024-05-01 15:36:33.513465042 +0200
+++ changed.yaml	2024-05-01 15:36:07.393463391 +0200
@@ -1,18 +1,18 @@
 apiVersion: vendir.k14s.io/v1alpha1
 kind: Config
 directories:
-  - path: charts/
+  - path: charts/cert-manager
     contents:
-      - path: cert-manager
+      - path: .
         lazy: true
         helmChart:
           name: cert-manager
           version: v1.14.2
           repository:
             url: https://charts.jetstack.io
-  - path: charts/
+  - path: charts/multus-cni
     contents:
-      - path: multus-cni
+      - path: .
         lazy: true
         helmChart:
           name: multus-cni

We can implement support for that, but I'd approach this as a separate topic. What we should definitely have is a proper error message in this case.

@fritzduchardt
Copy link
Collaborator

Thanks for fixing the second example. However, rendering with this only leads to the first chart being rendered for me.

@Zebradil
Copy link
Member Author

Zebradil commented May 2, 2024

Thanks for fixing the second example

But I didn't fix it. The second example won't work and it was my deliberate decision. I mentioned this in a comment earlier: #274 (comment)

I also briefly touched on that during our last call, but that probably was missed among lots of other details we discussed.

The reason for that was some logical complications of that state of the code, it could be already not an issue, I'll double-check that.

However, rendering with this only leads to the first chart being rendered for me.

Sorry, I didn't get. With wich one? Could you post the config which doesn't work for you?

@fritzduchardt
Copy link
Collaborator

fritzduchardt commented May 5, 2024

My bad - I meant the first example, the one you fixed. That one only renders the first workload.

@fritzduchardt
Copy link
Collaborator

Damn, I keep testing vendir.yamls that have overlapping paths (I put the dot at directory path level), which is now properly validated by vendir. However, this is not validated by myks, so it tries to sync and render those invalid vendir.yamls, without feedback to the user leading to unexpected results. I overlooked your comment regarding this a while back, sorry. Let's get this one merged, but put in a follow-up to ensure that we don't lose vendir validation.

internal/myks/sync_vendir.go Outdated Show resolved Hide resolved
internal/myks/sync_vendir.go Outdated Show resolved Hide resolved
return err
}

vendorDir := a.expandPath(a.e.g.VendorDirName)
return v.cleanupVendorDir(a, vendorDir, vendirConfigPath)
if err := os.Remove(linkFullPath); err != nil && !os.IsNotExist(err) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could fail, if the dir is not empty, e.g. if the user shortens a path in the vendir yaml, e.g. going from "ytt/grafana-dashboards" to just "ytt". Also, old directories are not cleaned up on path changes. How about simply deleting all previously created dirs before syncing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I now delete the whole vendor directory before creating any links.

vendirConfigPath := filepath.Join(cacheDir, a.e.g.VendirConfigFileName)
vendirLockPath := filepath.Join(cacheDir, a.e.g.VendirLockFileName)
if err := v.runVendirSync(a, vendirConfigPath, vendirLockPath, vendirSecrets); err != nil {
log.Error().Err(err).Msg(a.Msg(v.getStepName(), "Vendir sync failed"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing I noticed while using this branch for my home automation: We should consider removing the created cache dir on failure. Failure will usually be linked to wrong vendir.yaml config and thus the prior created entries are obsolete and mess up the cache. Using the cache to look at helm charts is quite useful during development, therefore keeping it tidy is worthwhile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, implemented.

@Zebradil Zebradil changed the title feat: central cache with symlinks feat!: central cache with symlinks May 10, 2024
@Zebradil Zebradil merged commit fd450cd into main May 10, 2024
8 checks passed
@Zebradil Zebradil deleted the central-cache-symlinks branch May 10, 2024 22:16
@Zebradil Zebradil linked an issue May 15, 2024 that may be closed by this pull request
@Zebradil Zebradil mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Central Caching
2 participants