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

Fix daemonset does not generate manifest #434

Closed
wants to merge 1 commit into from
Closed

Fix daemonset does not generate manifest #434

wants to merge 1 commit into from

Conversation

c0ffeec0der
Copy link

Fix issue #433

@c0ffeec0der
Copy link
Author

I am not sure why linting fail.. It works when I run loki with tanka. Please take a look at this as it is breaking loki tanka installation

@c0ffeec0der
Copy link
Author

Hello anyone looking at this? This PR will fix issue the lib implementation in loki. Thank you

@c0ffeec0der
Copy link
Author

@Duologic would you be able to review this? It's breaking the lib implementaion in loki. I use this code and it works. I checked the linting error and it doesn't tell much why it is error. It shouldn't be error because it works when I run with loki

@Duologic
Copy link
Member

Can you make a minimal reproducible example that fails before and succeeds after this fix?

@Duologic Duologic self-requested a review January 31, 2021 21:32
@c0ffeec0der
Copy link
Author

Before code change,

  1. Install loki with https://grafana.com/docs/loki/latest/installation/tanka/#install-loki-with-tanka
  2. Error occurs (Build using Tanka failed because grafana/jsonnet-libs had been refactored loki#3192)
evaluating jsonnet: RUNTIME ERROR: function expected 1 positional argument(s), but got 2
        C:\main\repos\1-infra\2-services\3-monitoring\vendor/ksonnet-util/grafana.libsonnet:27:9-50     function <anonymous>    
        C:\main\repos\1-infra\2-services\3-monitoring\vendor/promtail/promtail.libsonnet:67:5-71        object <anonymous>      
        During manifestation
  1. After code change, the error is gone

It seems this line
super.new(name, containers, podLabels={}) +

is referring to the generated library instead of the old ksonnet util lib. CMIIW

@Duologic
Copy link
Member

Duologic commented Feb 1, 2021

Your example contains many elements that make it hard to determine the cause. A minimal reproducible example should help us get to the root of the problem.

@Duologic
Copy link
Member

Duologic commented Feb 1, 2021

The Loki library has locked the ksonnet-util version: https://github.com/grafana/loki/blob/b29bc19/production/ksonnet/loki/jsonnetfile.lock.json#L24-L33

I just did a quick run with this version and also get an error, imo the Loki library should not lock versions like that.

@c0ffeec0der
Copy link
Author

CMIIW the lock file exist for any library used in jsonnetfile.json. It is the concept for using jb (jsonnet bundler)

To see the version used, refer to jsonnetfile.json

https://github.com/grafana/loki/blob/b29bc19d7cf20285231a98ae0ec15b1421e7daa6/production/ksonnet/loki/jsonnetfile.json#L22-L30

Loki is using ksonnet-lib master version

Duologic added a commit to Duologic/temp-debug-ksonnet-util that referenced this pull request Feb 3, 2021
@Duologic
Copy link
Member

Duologic commented Feb 3, 2021

I cannot reproduce with above instructions.

I've tested this with this setup: https://github.com/Duologic/temp-debug-ksonnet-util/tree/1ebd3166c2b4d12e7da0c8f8d7b5a014eab54acb

Please have a look at your project jsonnetfile.lock.json to ensure it matches jsonnet-libs master.

@snoord
Copy link

snoord commented Feb 5, 2021

To preface this comment, I have only started using Jsonnet in the last day, so I apologise if this comment is incorrect or irrelevant.

That said, I was having the same issue as @c0ffeec0der - I have been trying to generate this DaemonSet manifest using the Tanka command tk show environments/default:

// environments/default/main.jsonnet
(import 'github.com/grafana/jsonnet-libs/ksonnet-util/kausal.libsonnet') +
{
  _config:: {
    example: {
      name: 'example',
      image: 'example/image:latest',
    },
  },

  local c = $._config,
  local container = $.core.v1.container,
  local daemonSet = $.apps.v1.daemonSet,
  local port = $.core.v1.containerPort,

  manifest: {
    daemonSet: daemonSet.new(c.example.name, containers=[
      container.new(c.example.name, c.example.image)
      + container.withPorts(
        port.newNamed(12345, 'example')
      ),
    ]),
  },
}

Using this manifest would result in the following error:

Error: evaluating jsonnet: RUNTIME ERROR: function expected 1 positional argument(s), but got 2
        /Users/samuel/code/jsonnet-example/vendor/github.com/grafana/jsonnet-libs/ksonnet-util/grafana.libsonnet:27:9-50        function <anonymous>
        /Users/samuel/code/jsonnet-example/environments/default/main.jsonnet:(17:16)-(22:7)     object <anonymous>
        During manifestation

I then tried using the modified code from @c0ffeec0der, which resulted in the successful generation of the DaemonSet manifest.

However, I remembered that my lib/k.libsonnet file had an additional import line that I added when I started trying to learn Tanka + Jsonnet:

// lib/k.libsonnet
(import 'github.com/jsonnet-libs/k8s-alpha/1.18/main.libsonnet') +
(import 'github.com/jsonnet-libs/k8s-alpha/1.18/extensions/kausal-shim.libsonnet')

I removed @c0ffeec0der's changes to ksonnet-util/grafana.libsonnet and the kasual-shim.libsonnet line from lib/k.libsonnet:

// lib/k.libsonnet
+ (import 'github.com/jsonnet-libs/k8s-alpha/1.18/main.libsonnet')
- (import 'github.com/jsonnet-libs/k8s-alpha/1.18/main.libsonnet') +
- (import 'github.com/jsonnet-libs/k8s-alpha/1.18/extensions/kausal-shim.libsonnet')

...and I could successfully generate the DaemonSet manifest!

❯ tk show environments/default
apiVersion: apps/v1
kind: DaemonSet
metadata:
  name: example
  namespace: default
spec:
  minReadySeconds: 10
  selector:
    matchLabels:
      name: example
  template:
    metadata:
      labels:
        name: example
    spec:
      containers:
      - image: example/image:latest
        imagePullPolicy: IfNotPresent
        name: example
        ports:
        - containerPort: 12345
          name: example
      tolerations:
      - effect: NoSchedule
        operator: Exists
  updateStrategy:
    type: RollingUpdate

Not really sure how or why this worked (as I said, I'm only at the beginning of my journey into Jsonnet), but I thought this might be useful to others who know a bit more than I do 🙂

@Duologic
Copy link
Member

Duologic commented Feb 5, 2021

Aha, that seems to be it, the kausal-shim is now integrated into ksonnet-util. Removing that is the right choice.

It looks like the docs have recently (8 days ago) been updated, it doesn't reference the kausal-shim anymore.

@c0ffeec0der Can you confirm?

@snoord Thank your for that insight.

@Duologic
Copy link
Member

I'm considering this solved.

@Duologic Duologic closed this Feb 12, 2021
@c0ffeec0der
Copy link
Author

@Duologic Apologies, I was concentrating on other stuff and went deep. Just taking a look at this one. Confirm this is solved

@snoord Thank you so much!

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.

3 participants