Skip to content

Conversation

@haircommander
Copy link
Contributor

  • One-line PR description:
  • Other comments:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 4, 2024
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 4, 2024
@haircommander haircommander mentioned this pull request Jun 4, 2024
23 tasks
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

based on prior feedback kubernetes/kubernetes#114847 (comment) - there was a request to consider splitting up the config items, one to specify the duration for rechecking, and another for whether or not cross pod checking is enabled on the node.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2024
@haircommander
Copy link
Contributor Author

based on prior feedback kubernetes/kubernetes#114847 (comment) - there was a request to consider splitting up the config items, one to specify the duration for rechecking, and another for whether or not cross pod checking is enabled on the node.

Thanks for the reminder! I have updated it

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2024
when the feature gate is on the second pod will receive an error message, where
before and with the feature gate off the second pod would be able to use the image
pulled with credentials by the first pod.
For the `Never` policy, the behavior also must change. Otherwise, a user who wishes to use the image of another pod could just use `Never` and hope
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to a reviewer: this is a net-new aspect of this KEP, PTAL

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

see comments..

@haircommander
Copy link
Contributor Author

I reworked the summary @mikebrow PTAL

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@johnbelamaric
Copy link
Member

Since PRR approval was nearly 3 years ago, I am sure the PRR questions have a few new ones since then. Please sync up with the latest template.

Also, glancing through it, rather than saying "see above" for the "default behavior" question, can you call out the significant user-visible default behavior changes? Something like "some pods may begin to see errors if they do not have imagePullSecrets defined under such-and-such conditions".

Also "going back to works as designed" is not very clear for the question on disablement. Maybe something like "pods will be able to use images without verifying their access to secrets" or something a little more complete.

May I also suggest you add a metric NOW, with the feature gate DISABLED, which identifies which Pods would have had an error, if the feature gate were enabled. This allows admins to see if they will get errors in their clusters, before they even anbel the feature gate.

@johnbelamaric
Copy link
Member

/hold for PRR question updates

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2024
…uration flag to define different behaviors"

This reverts commit b14b2ee.

As the suggestion to drop the persist caused it to be blocked at code merge in 1.30. Reintroduce
the cache concept

Signed-off-by: Peter Hunt <[email protected]>
Signed-off-by: Peter Hunt <[email protected]>
@haircommander
Copy link
Contributor Author

thanks @johnbelamaric! I checked and didn't see any PRR sections that had changed since this was filled out. I updated based on some of your feedback and embellished generally, PTAL

@johnbelamaric
Copy link
Member

/approve for PRR

I am leaving the hold on for SIG approval. Once there is SIG approval, you can release the hold.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, johnbelamaric, mikebrow, mrunalp

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

@mrunalp
Copy link
Contributor

mrunalp commented Jun 12, 2024

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2024
@k8s-ci-robot k8s-ci-robot merged commit aa8a8ce into kubernetes:master Jun 12, 2024
@k8s-ci-robot k8s-ci-robot added this to the v1.31 milestone Jun 12, 2024
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. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

Development

Successfully merging this pull request may close these issues.

6 participants