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

Adding policy for CPU Limits for all container types #1067

Merged
merged 7 commits into from
Aug 1, 2024

Conversation

nsagark
Copy link
Contributor

@nsagark nsagark commented Jul 9, 2024

Related Issue(s)

Description

Checklist

  • I have read the policy contribution guidelines.
  • I have added test manifests and resources covering both positive and negative tests that prove this policy works as intended.
  • I have added the artifacthub-pkg.yml file and have verified it is complete and correct.

Signed-off-by: nsagark <sagar@nirmata.com>
…ests and Kyverno CLI tests

Signed-off-by: nsagark <sagar@nirmata.com>
@nsagark
Copy link
Contributor Author

nsagark commented Jul 18, 2024

@chipzoller I have updated the files as per your comments. Please review

@nsagark nsagark requested a review from chipzoller July 18, 2024 03:17
@chipzoller
Copy link
Contributor

Are all comments resolved?

@nsagark
Copy link
Contributor Author

nsagark commented Jul 26, 2024

Hi @chipzoller Yes, I have updated the files. Just to make sure to address below comment,

This test resource manifest needs to cover cases where there are multiple containers (both containers[] and initContainers[]) in which one container sets a limit while the other does not

I have added below in resource.yaml file. Is that what you meant or something else?

`apiVersion: v1
kind: Pod
metadata:
name: badpod07
spec:
initContainers:

  • name: initcontainer01
    image: dummyimagename
  • name: initcontainer02
    image: dummyimagename
    containers:
  • name: container01
    image: dummyimagename
    resources:
    limits:
    cpu: "500m"
  • name: container02
    image: dummyimagename `

@chipzoller
Copy link
Contributor

Yes

@nsagark
Copy link
Contributor Author

nsagark commented Jul 29, 2024

With that, I have addressed all the comments. Please review.

Copy link
Contributor

@chipzoller chipzoller left a comment

Choose a reason for hiding this comment

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

Please ensure to resolve all conversations when complete.

Signed-off-by: nsagark <sagar@nirmata.com>
@nsagark
Copy link
Contributor Author

nsagark commented Jul 29, 2024

@chipzoller All conversations are resolved.

@chipzoller
Copy link
Contributor

Several conversations unresolved.

@nsagark
Copy link
Contributor Author

nsagark commented Jul 31, 2024

@chipzoller Sorry, I have resolved previous conversations. Please check now

Copy link
Contributor

@chipzoller chipzoller 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 other than the AH digest needing to be updated.

@nsagark
Copy link
Contributor Author

nsagark commented Jul 31, 2024

Hi @chipzoller The AH digest seems to be correct. Am I doing anything wrong below?

Sagar@DESKTOP-VS85098 MINGW64 /c/users/sagar/Downloads/kyverno-prs/policies/other/require-cpu-limits (require-cpu-limits)
$ sha256sum require-cpu-limits.yaml
2fe9fa043fbb4c85da0204a00a60ce6732ac0dc0e3a5ece4f7b741982a1003d1 *require-cpu-limits.yaml

Sagar@DESKTOP-VS85098 MINGW64 /c/users/sagar/Downloads/kyverno-prs/policies/other/require-cpu-limits (require-cpu-limits)
$ ls -lrht
total 8.0K
-rw-r--r-- 1 Sagar 197121 1.5K Jul 31 09:47 require-cpu-limits.yaml
-rw-r--r-- 1 Sagar 197121 1.5K Jul 31 09:47 artifacthub-pkg.yml

@chipzoller
Copy link
Contributor

You're not doing anything wrong, it's just the expected digest is calculated in the pipeline under Linux. You're doing it under Windows which means the digest may be slightly different. The one being reported in the pipeline is 1d9998010342080ae64f309befdf52065de557cdcc10ddf68d8476b5af93d505. Go ahead and put that in the AH metadata file and it should pass.

Signed-off-by: nsagark <sagar@nirmata.com>
@nsagark
Copy link
Contributor Author

nsagark commented Jul 31, 2024

Hi @chipzoller I have updated the digest. Thanks for pointing it out.

@nsagark
Copy link
Contributor Author

nsagark commented Aug 1, 2024

Hi @chipzoller I don't see the changes yet in the main branch. Are we good here or anything missing?

@chipzoller chipzoller merged commit 4854f24 into kyverno:main Aug 1, 2024
265 checks passed
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.

2 participants