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: syntax errors in kube-state-metrics.libsonnet #2454

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

jeffmccune
Copy link
Contributor

Previously customizing kube-prometheus 1 failed with the following
error.

❯ ./build.sh example.jsonnet
+ set -o pipefail
+ rm -rf manifests
+ mkdir -p manifests/setup
+ jsonnet -J vendor -m manifests example.jsonnet
+ xargs '-I{}' sh -c 'cat {} | gojsontoyaml > {}.yaml' -- '{}'
RUNTIME ERROR: vendor/github.com/kubernetes/kube-state-metrics/jsonnet/kube-state-metrics/kube-state-metrics.libsonnet:392:21-22 Expected token OPERATOR but got "{"
        vendor/kube-prometheus/components/kube-state-metrics.libsonnet:51:19-124        function <anonymous>
        vendor/kube-prometheus/main.libsonnet:136:21-64 object <anonymous>
        vendor/kube-prometheus/platforms/platforms.libsonnet:37:22-40   +:
        example.jsonnet:33:90-109       thunk from <$>
        <std>:1539:24-25        thunk from <function <anonymous>>
        <std>:1539:5-33 function <anonymous>
        example.jsonnet:33:73-110       $
        example.jsonnet:33:1-112
        example.jsonnet:33:1-112
        During evaluation

With this patch, the build succeeds:

❯ bash build.sh example.jsonnet ; echo $?
+ set -o pipefail
+ rm -rf manifests
+ mkdir -p manifests/setup
+ jsonnet -J vendor -m manifests example.jsonnet
+ xargs '-I{}' sh -c 'cat {} | gojsontoyaml > {}.yaml' -- '{}'
+ find manifests -type f '!' -name '*.yaml' -delete
+ rm -f kustomization
0

What this PR does / why we need it:

Quick follow up fix to syntax errors introduced in #2388 affecting kube-prometheus customization.

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)

Does not change.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Unknown, see #2388

Copy link

linux-foundation-easycla bot commented Jul 24, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jeffmccune / name: Jeff McCune (3aebcf7)

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 24, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @jeffmccune!

It looks like this is your first PR to kubernetes/kube-state-metrics 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kube-state-metrics has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 24, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 24, 2024
@CatherineF-dev
Copy link
Contributor

/lgtm Thanks!

@mrueg
Copy link
Member

mrueg commented Jul 25, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 25, 2024
@mrueg
Copy link
Member

mrueg commented Jul 25, 2024

Looks like there are further changes needed. Can you regenerate it running make generate?

@dgrisonnet
Copy link
Member

/assign @mrueg
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 25, 2024
selector{
matchLabels: {app.kubernetes.io/name': shardksmname}
}
selector: {
Copy link
Contributor

@CatherineF-dev CatherineF-dev Jul 26, 2024

Choose a reason for hiding this comment

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

Thx for the fix.

  1. move local shardksmname = ksm.name + "-unscheduled-pods-fetching"; to
    deploymentNoNodePods:
      # <--------here 
      local c = ksm.deployment.spec.template.spec.containers[0] {
  1. generate manifests make generate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CatherineF-dev Thanks, I made this change in a860c17 and ran make generate, but the CI failure, make validate-manifests is still failing locally for me. Could you help me understand why and what needs to be done to fix the CI job?

make validate-manifests
diff --git a/examples/daemonsetsharding/deployment-no-node-pods-service.yaml b/examples/daemonsetsharding/deployment-no-node-pods-service.yaml
index 5f287b7a..ddf08923 100644
--- a/examples/daemonsetsharding/deployment-no-node-pods-service.yaml
+++ b/examples/daemonsetsharding/deployment-no-node-pods-service.yaml
@@ -4,7 +4,7 @@ metadata:
   labels:
     app.kubernetes.io/component: exporter
     app.kubernetes.io/name: kube-state-metrics-no-node-pods
-    app.kubernetes.io/version: 2.12.0
+    app.kubernetes.io/version: 2.13.0
   name: kube-state-metrics-no-node-pods
   namespace: kube-system
 spec:
diff --git a/examples/daemonsetsharding/deployment-no-node-pods.yaml b/examples/daemonsetsharding/deployment-no-node-pods.yaml
index eac87a53..c3b6c00b 100644
--- a/examples/daemonsetsharding/deployment-no-node-pods.yaml
+++ b/examples/daemonsetsharding/deployment-no-node-pods.yaml
@@ -3,20 +3,20 @@ kind: Deployment
 metadata:
   labels:
     app.kubernetes.io/component: exporter
-    app.kubernetes.io/name: kube-state-metrics-no-node-pods
+    app.kubernetes.io/name: kube-state-metrics-unscheduled-pods-fetching
     app.kubernetes.io/version: 2.13.0
-  name: kube-state-metrics-no-node-pods
+  name: kube-state-metrics-unscheduled-pods-fetching
   namespace: kube-system
 spec:
   replicas: 1
   selector:
     matchLabels:
-      app.kubernetes.io/name: kube-state-metrics-no-node-pods
+      app.kubernetes.io/name: kube-state-metrics-unscheduled-pods-fetching
   template:
     metadata:
       labels:
         app.kubernetes.io/component: exporter
-        app.kubernetes.io/name: kube-state-metrics-no-node-pods
+        app.kubernetes.io/name: kube-state-metrics-unscheduled-pods-fetching
         app.kubernetes.io/version: 2.13.0
     spec:
       automountServiceAccountToken: true
@@ -31,7 +31,7 @@ spec:
             port: http-metrics
           initialDelaySeconds: 5
           timeoutSeconds: 5
-        name: kube-state-metrics-no-node-pods
+        name: kube-state-metrics-unscheduled-pods-fetching
         ports:
         - containerPort: 8080
           name: http-metrics
make: *** [validate-manifests] Error 1

Copy link
Member

Choose a reason for hiding this comment

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

Can you run make generate -B ?

Copy link
Contributor Author

@jeffmccune jeffmccune Aug 5, 2024

Choose a reason for hiding this comment

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

My mistake, I forgot to commit the generated examples. 3aebcf7 should be good, passes locally for me.

Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
        modified:   examples/daemonsetsharding/deployment-no-node-pods-service.yaml
        modified:   examples/daemonsetsharding/deployment-no-node-pods.yaml

@mrueg
Copy link
Member

mrueg commented Jul 30, 2024

@jeffmccune friendly ping, do you have time to fix the CI and include @CatherineF-dev's comments?

@rgarcia89
Copy link

Any update here? https://github.com/prometheus-operator/kube-prometheus is currently not updating anymore due to this issue

@jeffmccune
Copy link
Contributor Author

Yes I'll take a look at this today.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2024
Previously customizing kube-prometheus [1] failed with the following
error.

    ❯ ./build.sh example.jsonnet
    + set -o pipefail
    + rm -rf manifests
    + mkdir -p manifests/setup
    + jsonnet -J vendor -m manifests example.jsonnet
    + xargs '-I{}' sh -c 'cat {} | gojsontoyaml > {}.yaml' -- '{}'
    RUNTIME ERROR: vendor/github.com/kubernetes/kube-state-metrics/jsonnet/kube-state-metrics/kube-state-metrics.libsonnet:392:21-22 Expected token OPERATOR but got "{"
            vendor/kube-prometheus/components/kube-state-metrics.libsonnet:51:19-124        function <anonymous>
            vendor/kube-prometheus/main.libsonnet:136:21-64 object <anonymous>
            vendor/kube-prometheus/platforms/platforms.libsonnet:37:22-40   +:
            example.jsonnet:33:90-109       thunk from <$>
            <std>:1539:24-25        thunk from <function <anonymous>>
            <std>:1539:5-33 function <anonymous>
            example.jsonnet:33:73-110       $
            example.jsonnet:33:1-112
            example.jsonnet:33:1-112
            During evaluation

With this patch, the build succeeds:

    ❯ bash build.sh example.jsonnet ; echo $?
    + set -o pipefail
    + rm -rf manifests
    + mkdir -p manifests/setup
    + jsonnet -J vendor -m manifests example.jsonnet
    + xargs '-I{}' sh -c 'cat {} | gojsontoyaml > {}.yaml' -- '{}'
    + find manifests -type f '!' -name '*.yaml' -delete
    + rm -f kustomization
    0

[1]: https://github.com/prometheus-operator/kube-prometheus/blob/main/docs/customizing.md
@CatherineF-dev
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CatherineF-dev, jeffmccune, mrueg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 312b298 into kubernetes:main Aug 5, 2024
3 checks passed
@CatherineF-dev
Copy link
Contributor

I didn't know /lgtm will merge this PR without waiting presubmits to finish.

@jeffmccune
Copy link
Contributor Author

I didn't know /lgtm will merge this PR without waiting presubmits to finish.

Me either, but I think we're good, it pulled in 3aebcf7 which should pass anyway.

@jeffmccune jeffmccune deleted the jeff/kp branch August 5, 2024 15:50
@mrueg
Copy link
Member

mrueg commented Aug 5, 2024

This looks like a race condition happened. I'll follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants