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

Add resources config option to cni metrics helper #2141

Merged
merged 3 commits into from
Nov 17, 2022

Conversation

jcogilvie
Copy link
Contributor

@jcogilvie jcogilvie commented Nov 15, 2022

What type of PR is this?
feature

Which issue does this PR fix:
#2140

What does this PR do / Why do we need it:
Adds an option to set resources in the cni metrics helper helm chart

Testing done on this change:

  • helm template run against the cni metrics helper chart with a values file containing:
resources:
  requests:
    memory: 256Mi
    cpu: 10m
  limits:
    memory: 512Mi
    cpu: 50m

i.e. helm template ./charts/cni-metrics-helper --values=test-resources.yaml --debug

  • Visually inspected that the resources are correct:
---
# Source: cni-metrics-helper/templates/deployment.yaml
kind: Deployment
apiVersion: apps/v1
metadata:
  name: cni-metrics-helper
  namespace: default
  labels:
    k8s-app: cni-metrics-helper
spec:
  selector:
    matchLabels:
      k8s-app: cni-metrics-helper
  template:
    metadata:
      labels:
        k8s-app: cni-metrics-helper
    spec:
      containers:
      - env:
        - name: AWS_CLUSTER_ID
          value: ""
        - name: USE_CLOUDWATCH
          value: "true"
        resources:
          limits:
            cpu: 50m
            memory: 512Mi
          requests:
            cpu: 10m
            memory: 256Mi
        name: cni-metrics-helper
        image: "602401143452.dkr.ecr.us-west-2.amazonaws.com/cni-metrics-helper:v1.11.4"
      serviceAccountName: cni-metrics-helper
  • applied the chart to my running cluster (eks 1.22)

Will this PR introduce any new dependencies?:
No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No; tested an upgrade and it worked fine.

Does this change require updates to the CNI daemonset config files to work?:
No

Does this PR introduce any user-facing change?:
Yes

Added `resources` block to `cni-metrics-helper` helm chart

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jdn5126 jdn5126 self-assigned this Nov 15, 2022
@jdn5126
Copy link
Contributor

jdn5126 commented Nov 16, 2022

@jcogilvie these changes conceptually look good to me. My only comment is that for resources, we should either have default values to render or not render the field when it is empty.

@jcogilvie jcogilvie marked this pull request as ready for review November 16, 2022 18:19
@jcogilvie jcogilvie requested a review from a team as a code owner November 16, 2022 18:19
@jdn5126 jdn5126 merged commit 9dc83c6 into aws:master Nov 17, 2022
@jcogilvie jcogilvie deleted the cni-metrics-helper-resources branch November 17, 2022 14:59
@jayanthvn jayanthvn added this to the v1.12.1 milestone Nov 23, 2022
jdn5126 added a commit that referenced this pull request Dec 12, 2022
* create publisher with logger (#2119)

* Add missing rules when NodePort support is disabled (#2026)

* Add missing rules when NodePort support is disabled

* the rules that need to be installed for NodePort support and SNAT
  support are very similar. The same traffic mark is needed for both. As
  a result, rules that are currently installed only when NodePort
  support is enabled should also be installed when external SNAT is
  disabled, which is the case by default.
* remove "-m state --state NEW" from a rule in the nat table. This is
  always true for packets that traverse the nat table.
* fix typo in one rule's name (extra whitespace).

Fixes #2025

Co-authored-by: Quan Tian <[email protected]>

Signed-off-by: Antonin Bas <[email protected]>

* Fix typos and unit tests

Signed-off-by: Antonin Bas <[email protected]>

* Minor improvement to code comment

Signed-off-by: Antonin Bas <[email protected]>

* Address review comments

* Delete legacy nat rule
* Fix an unrelated log message

Signed-off-by: Antonin Bas <[email protected]>

Signed-off-by: Antonin Bas <[email protected]>
Co-authored-by: Jayanth Varavani <[email protected]>
Co-authored-by: Sushmitha Ravikumar <[email protected]>

* downgrade test go.mod to align with root go.mod (#2128)

* skip addon installation when addon info is not available (#2131)

* Merging test/Makefile and test/go.mod to the root Makefil and go.mod, adjust the .github/workflows and integration test instructions (#2129)

* update troubleshooting docs for CNI image (#2132)

fix location where make command is run

* fix env name in test script (#2136)

* optionally allow CLUSTER_ENDPOINT to be used rather than the cluster-ip (#2138)

* optionally allow CLUSTER_ENDPOINT to be used rather than the kubernetes cluster ip

* remove check for kube-proxy

* add version to readme

* Add resources config option to cni metrics helper (#2141)

* Add resources config option to cni metrics helper

* Remove default-empty resources block; replace with conditional

* Add metrics for ec2 api calls made by CNI and expose via prometheus (#2142)

Co-authored-by: Jay Deokar <[email protected]>

* increase workflow role duration to 4 hours (#2148)

* Update golang 1.19.2 EKS-D (#2147)

* Update golang

* Move to EKS distro builds

* [HELM]: Move CRD resources to a separate folder as per helm standard (#2144)

Co-authored-by: Jay Deokar <[email protected]>

* VPC-CNI minimal image builds (#2146)

* VPC-CNI minimal image builds

* update dependencies for ginkgo when running integration tests

* address review comments and break up init main function

* review comments for sysctl

* Simplify binary installation, fix review comments

Since init container is required to always run, let binary installation
for external plugins happen in init container. This simplifies the main
container entrypoint and the dockerfile for each image.

* when IPAMD connection fails, try to teardown pod network using prevResult (#2145)

* add env var to enable nftables (#2155)

* fix failing weekly cron tests (#2154)

* Deprecate AWS_VPC_K8S_CNI_CONFIGURE_RPFILTER and remove no-op setter (#2153)

* Deprecate AWS_VPC_K8S_CNI_CONFIGURE_RPFILTER

* update release version comments

Signed-off-by: Antonin Bas <[email protected]>
Co-authored-by: Jeffrey Nelson <[email protected]>
Co-authored-by: Antonin Bas <[email protected]>
Co-authored-by: Jayanth Varavani <[email protected]>
Co-authored-by: Sushmitha Ravikumar <[email protected]>
Co-authored-by: Jerry He <[email protected]>
Co-authored-by: Brandon Wagner <[email protected]>
Co-authored-by: Jonathan Ogilvie <[email protected]>
Co-authored-by: Jay Deokar <[email protected]>
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