-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
KEP-2133: add beta milestone and prod readiness review #2457
Conversation
/assign @derekwaynecarr @deads2k |
@@ -22,12 +22,12 @@ replaces: | |||
- "/keps/sig-cloud-provider/20191004-out-of-tree-credential-providers.md" | |||
|
|||
# The target maturity stage in the current dev cycle for this KEP. | |||
stage: alpha | |||
stage: beta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff doesn't show it, but beta graduation was already merged in a previous PR and listed here:
|
||
* **What specific metrics should inform a rollback?** | ||
|
||
TBD for beta. | ||
This feature does not have metrics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metrics for how a credential exec plugin is performing seems pretty reasonable and low cardinality. And it would be used to inform how well this feature and the exec plugin itself are working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, I'll add something about this
|
||
* **Is the rollout accompanied by any deprecations and/or removals of features, APIs, | ||
fields of API types, flags, etc.?** | ||
|
||
TBD for beta. | ||
Yes, this feature was added to remove the in-tree kubelet credential providers for AWS, Azure and GCP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without metrics on performance and reliability of the exec plugin flow, how will these providers have confidence that they can move?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added metrics
- [ ] Other (treat as last resort) | ||
- Details: | ||
- [X] Other (treat as last resort) | ||
- Details: the kubelet has several error-level logs for when exec plugins time out or return a non-zero exit code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a reasonable metric to build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added metrics
the exact metric names and labels can be finessed during implementation, but the intent here is clear now. Thanks. /lgtm |
/lgtm |
The original graduation criteria had specified at least one working plugin implementation. Is there a specific working plugin implementation that we are targeting for integration with e2e test? The rest of this PR looks fine to me. |
@derekwaynecarr yes the ECR plugin has been merged a couple months back (https://github.com/kubernetes/cloud-provider-aws/tree/master/cmd/ecr-credential-provider) and we are hoping that will go through some testing prior to promoting this feature to beta. |
Signed-off-by: Andrew Sy Kim <[email protected]>
Signed-off-by: Andrew Sy Kim <[email protected]>
Signed-off-by: Andrew Sy Kim <[email protected]>
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, deads2k, derekwaynecarr 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 |
Adding beta milestone and beta requirements for PRR for the Kubelet Credential Provider KEP.
Signed-off-by: Andrew Sy Kim [email protected]