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

Helm: Add revision history limit for master replica #1782

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

omerap12
Copy link
Member

Fixes: #1759

Tested locally. Output:

---
# Source: node-feature-discovery/templates/master.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name:  release-name-node-feature-discovery-master
  namespace: default
  labels:
    helm.sh/chart: node-feature-discovery-0.2.1
    app.kubernetes.io/name: node-feature-discovery
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "master"
    app.kubernetes.io/managed-by: Helm
    role: master
spec:
  replicas: 1
  revisionHistoryLimit: 3
  selector:
    matchLabels:
      app.kubernetes.io/name: node-feature-discovery
      app.kubernetes.io/instance: release-name
      role: master
  template:
    metadata:
      labels:
        app.kubernetes.io/name: node-feature-discovery
        app.kubernetes.io/instance: release-name
        role: master
    spec:
      serviceAccountName: release-name-node-feature-discovery
      enableServiceLinks: false
      securityContext:
        {}
      containers:
        - name: master
          securityContext:
            allowPrivilegeEscalation: false
            capabilities:
              drop:
              - ALL
            readOnlyRootFilesystem: true
            runAsNonRoot: true
          image: "gcr.io/k8s-staging-nfd/node-feature-discovery:master"
          imagePullPolicy: Always
          livenessProbe:
            grpc:
              port: 8082
            initialDelaySeconds: 10
            periodSeconds: 10
          readinessProbe:
            grpc:
              port: 8082
            initialDelaySeconds: 5
            periodSeconds: 10
            failureThreshold: 10
          ports:
          - containerPort: 8080
            name: grpc
          - containerPort: 8081
            name: metrics
          env:
          - name: NODE_NAME
            valueFrom:
              fieldRef:
                fieldPath: spec.nodeName
          command:
            - "nfd-master"
          resources:
            limits:
              memory: 4Gi
            requests:
              cpu: 100m
              memory: 128Mi
          args:
            ## By default, disable crd controller for other than the default instances
            - "-crd-controller=true"
            # Go over featureGates and add the feature-gate flag
            - "-feature-gates=NodeFeatureAPI=true"
            - "-feature-gates=NodeFeatureGroupAPI=false"
            - "-metrics=8081"
          volumeMounts:
            - name: nfd-master-conf
              mountPath: "/etc/kubernetes/node-feature-discovery"
              readOnly: true
      volumes:
        - name: nfd-master-conf
          configMap:
            name: release-name-node-feature-discovery-master-conf
            items:
              - key: nfd-master.conf
                path: nfd-master.conf
      affinity:
        nodeAffinity:
          preferredDuringSchedulingIgnoredDuringExecution:
          - preference:
              matchExpressions:
              - key: node-role.kubernetes.io/master
                operator: In
                values:
                - ""
            weight: 1
          - preference:
              matchExpressions:
              - key: node-role.kubernetes.io/control-plane
                operator: In
                values:
                - ""
            weight: 1
      tolerations:
        - effect: NoSchedule
          key: node-role.kubernetes.io/master
          operator: Equal
          value: ""
        - effect: NoSchedule
          key: node-role.kubernetes.io/control-plane
          operator: Equal
          value: ""

@k8s-ci-robot k8s-ci-robot requested review from adrianchiris and kad July 11, 2024 12:44
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 11, 2024
Copy link

netlify bot commented Jul 11, 2024

Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit 920306c
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/669149d2dd52a8000858dc11
😎 Deploy Preview https://deploy-preview-1782--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Thanks @omerap12 for the contribution!

For the sake of consistency, I think we should add this parameter to nfd-gc, too (templates/nfd-gc.yaml, gc.revisionHistoryLimit in values). Also, we need to document the new parameter(s) in docs/deployment/helm.md.

@omerap12
Copy link
Member Author

Thanks @omerap12 for the contribution!

For the sake of consistency, I think we should add this parameter to nfd-gc, too (templates/nfd-gc.yaml, gc.revisionHistoryLimit in values). Also, we need to document the new parameter(s) in docs/deployment/helm.md.

sure. adding

@omerap12 omerap12 force-pushed the issue_1759 branch 2 times, most recently from 2fe8188 to c26d609 Compare July 11, 2024 13:12
@omerap12
Copy link
Member Author

fixed @TessaIO

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 11, 2024
@omerap12 omerap12 requested a review from TessaIO July 11, 2024 13:52
docs/deployment/helm.md Outdated Show resolved Hide resolved
docs/deployment/helm.md Outdated Show resolved Hide resolved
@omerap12 omerap12 requested a review from TessaIO July 11, 2024 14:21
docs/deployment/helm.md Outdated Show resolved Hide resolved
docs/deployment/helm.md Outdated Show resolved Hide resolved
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
@omerap12
Copy link
Member Author

Friendly reminder :) @marquiz

Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Looks good, now 👍

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz, omerap12, TessaIO

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2024
@marquiz
Copy link
Contributor

marquiz commented Jul 15, 2024

/assign @TessaIO

@k8s-ci-robot
Copy link
Contributor

@AhmedThresh: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@TessaIO
Copy link
Member

TessaIO commented Jul 15, 2024

/lgtm

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

LGTM label has been added.

Git tree hash: 9f11225ec91e940defb754615380867aed0646c4

@k8s-ci-robot k8s-ci-robot merged commit 25ffe9c into kubernetes-sigs:master Jul 15, 2024
9 checks passed
@marquiz
Copy link
Contributor

marquiz commented Jul 15, 2024

@ArangoGutierrez @TessaIO maybe we should consider cherry-picking this to v0.16.3. It's such a tiny change even if not strictly a bug fix, WDYT?

@TessaIO
Copy link
Member

TessaIO commented Jul 15, 2024

no objections, we can go with that

@marquiz
Copy link
Contributor

marquiz commented Jul 15, 2024

/cherry-pick release-0.16

@k8s-infra-cherrypick-robot

@marquiz: #1782 failed to apply on top of branch "release-0.16":

Applying: Add revision history limit for master replica and for garbage collector
.git/rebase-apply/patch:40: trailing whitespace.
  
.git/rebase-apply/patch:42: trailing whitespace.
  revisionHistoryLimit: 
.git/rebase-apply/patch:51: trailing whitespace.
  revisionHistoryLimit: 
warning: 3 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	deployment/helm/node-feature-discovery/templates/master.yaml
M	deployment/helm/node-feature-discovery/templates/nfd-gc.yaml
M	deployment/helm/node-feature-discovery/values.yaml
M	docs/deployment/helm.md
Falling back to patching base and 3-way merge...
Auto-merging docs/deployment/helm.md
CONFLICT (content): Merge conflict in docs/deployment/helm.md
Auto-merging deployment/helm/node-feature-discovery/values.yaml
Auto-merging deployment/helm/node-feature-discovery/templates/nfd-gc.yaml
Auto-merging deployment/helm/node-feature-discovery/templates/master.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add revision history limit for master replica and for garbage collector
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@rouke-broersma
Copy link
Contributor

@omerap12 is there a specific reason you didn't add the field for the daemonset? I think the same applies, no?

@omerap12
Copy link
Member Author

@omerap12 is there a specific reason you didn't add the field for the daemonset? I think the same applies, no?

Hey, no good reason. I can raise another PR to add this field to the daemon set as well if you'd like :)

@rouke-broersma
Copy link
Contributor

Already done :)

@marquiz marquiz mentioned this pull request Dec 12, 2024
24 tasks
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for revisionHistoryLimit to Helm Chart
6 participants