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

Trigger PRR for kep 4369 #4478

Closed
wants to merge 1 commit into from

Conversation

HirazawaUi
Copy link
Contributor

@HirazawaUi HirazawaUi commented Feb 4, 2024

  • One-line PR description: Trigger PRR for kep 4369
  • Other comments:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 4, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: HirazawaUi
Once this PR has been reviewed and has the lgtm label, please assign wojtek-t for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Feb 4, 2024
@HirazawaUi
Copy link
Contributor Author

/cc @thockin

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 4, 2024
@HirazawaUi HirazawaUi changed the title Trigger PRR for kep 4373 Trigger PRR for kep 4369 Feb 4, 2024
# of http://git.k8s.io/enhancements/OWNERS_ALIASES
kep-number: 4369
alpha:
approver: "@thockin"
Copy link
Member

Choose a reason for hiding this comment

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

I am not a PRR approver.

@johnbelamaric @deads2k @jpbetz @wojtek-t

This one will hopefully be easy - "Allow almost all printable ASCII characters in environment variables"

Copy link
Contributor

Choose a reason for hiding this comment

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

@jpbetz volunteered for this one.

Copy link
Contributor

@jpbetz jpbetz Feb 5, 2024

Choose a reason for hiding this comment

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

I can review for PRR. Go ahead and swap my name in here.

Since this is targeting alpha, we don't need all the PRR sections complete. But I did notice that they have all been filled out, so I looked them over. "Monitoring Requirements" is marked as "N/A". I'd be okay with addressing this in Beta, but I think there are monitoring needs. @deads2k pointed out to me that kubelet/CRI implementations could fail on pods using this enhancement and we should be able to detect and monitor that (is there a metric that exists today that we can show surfaces this sort of problem?). Can we update the "Monitoring Requirements" to reflect that we will look into this in Beta and add a note to the graduation criteria?

We should also include (for GA) some "Troubleshooting" details for how to deal with incompatible kubelet/CRI implementations.

I also agree with the concerns in #4373 (comment)). We should articulate in the KEP somewhere what our "beta time duration" is going to be. A change like this limits v1 pods portability between clusters to those that are in a compatible version range, so I'd like that range to be sufficiently wide (all supported versions?) before GA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kubelet/CRI implementations could fail on pods using this enhancement and we should be able to detect and monitor that

I did a test on whether the CRI implementation accepts special characters in env var names. The container runtime is more lenient than k8s in accepting special characters in env var names.

ref: https://github.com/HirazawaUi/verfiy-container-env

But I agree that we should do some monitoring of it to avoid unexpected problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we update the "Monitoring Requirements" to reflect that we will look into this in Beta and add a note to the graduation criteria?

We should also include (for GA) some "Troubleshooting" details for how to deal with incompatible kubelet/CRI implementations.

so I'd like that range to be sufficiently wide (all supported versions?) before GA.

Okay, I'll add those to the KEP.

@HirazawaUi
Copy link
Contributor Author

I merged the commit of this PR to #4482

@HirazawaUi HirazawaUi closed this Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants