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

Jump to bottom

Relax csiNodeIDMaxLength to longer limit #98753

Merged
merged 1 commit into from Mar 9, 2021
Merged

Relax csiNodeIDMaxLength to longer limit #98753

merged 1 commit into from Mar 9, 2021

Conversation

Jiawei0227
Copy link
Contributor

@Jiawei0227 Jiawei0227 commented Feb 4, 2021 •
edited

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
With CSI spec change in container-storage-interface/spec#464, the length for CSI node id can be extended to 192 bytes. This can unblock users that have a longer csi node id in their CSI driver.

For backwards compatibility, we will break this down to two release. This release we will prepare for the next release change on the actual limits.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Prepare to increased CSINodeIDMaxLength from 128  bytes to 192 bytes in 1.22.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig storage
/assign @saad-ali

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/storage Categorizes an issue or PR as relevant to SIG Storage. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 4, 2021
@k8s-ci-robot
Copy link
Contributor

@Jiawei0227: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 4, 2021
@vpnachev vpnachev mentioned this pull request Feb 4, 2021
Closed
@Jiawei0227
Copy link
Contributor Author

/retest

1 similar comment
@Jiawei0227
Copy link
Contributor Author

/retest

saad-ali
saad-ali approved these changes Feb 5, 2021
View reviewed changes
Copy link
Member

@saad-ali saad-ali left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

/hold

Hold until CSI 1.4 is cut with this change

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 5, 2021
@saad-ali
Copy link
Member

saad-ali commented Mar 8, 2021

/hold cancel

CSI 1.4.0 has been cut https://github.com/container-storage-interface/spec/releases/tag/v1.4.0

@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 Mar 8, 2021
@saad-ali
Copy link
Member

saad-ali commented Mar 8, 2021

/assign @liggitt

For API approval

@liggitt liggitt added this to Changes requested in API Reviews Mar 8, 2021
@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 8, 2021
@Jiawei0227
Relax csiNodeIDMaxLength to longer limit
1e16615 
Update csiNodeIDMaxLength to 192 bytes
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 8, 2021
@Jiawei0227 Jiawei0227 changed the title Update csiNodeIDMaxLength to 192 bytes Relax csiNodeIDMaxLength to lenger limit Mar 8, 2021
@liggitt liggitt moved this from Changes requested to In progress in API Reviews Mar 8, 2021
@Jiawei0227
Copy link
Contributor Author

/retest

1 similar comment
@Jiawei0227
Copy link
Contributor Author

/retest

liggitt
liggitt reviewed Mar 9, 2021
View reviewed changes
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

pkg/registry/storage/csinode/strategy.go
errorList := validation.ValidateCSINode(newCSINodeObj)
return append(errorList, validation.ValidateCSINodeUpdate(newCSINodeObj, oldCSINodeObj)...)
// in 1.21 on update, set AllowLongNodeID to true only if the old object already has a long node ID
// in 1.22 on update, set AllowLongNodeID=true
Copy link
Member

Choose a reason for hiding this comment

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

make sure there's a tracking issue open to change this in 1.22

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I created #99981 for it.

@liggitt liggitt moved this from In progress to API review completed, 1.21 in API Reviews Mar 9, 2021
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 9, 2021
@Jiawei0227 Jiawei0227 changed the title Relax csiNodeIDMaxLength to lenger limit Relax csiNodeIDMaxLength to longer limit Mar 9, 2021
@Jiawei0227 Jiawei0227 mentioned this pull request Mar 9, 2021
Closed
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

ohbus
ohbus approved these changes Mar 9, 2021
View reviewed changes
Copy link

@ohbus ohbus left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

@ohbus: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jiawei0227, liggitt, ohbus, saad-ali

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

@k8s-ci-robot k8s-ci-robot merged commit dc74b9d into kubernetes:master Mar 9, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 9, 2021
@vpnachev
Copy link
Contributor

vpnachev commented Apr 5, 2021

With this change, I think the current release note is not correct:

Increased CSINodeIDMaxLength from 128 bytes to 192 bytes.

This change will be applicable in 1.22, but in 1.21 the limit is still 128 bytes.

@Jiawei0227
Copy link
Contributor Author

This change will be applicable in 1.22, but in 1.21 the limit is still 128 bytes.

I updated the release notes. Thanks!

@Jiawei0227 Jiawei0227 mentioned this pull request Apr 19, 2021
Closed
CKchen0726 added a commit to CKchen0726/kubernetes that referenced this pull request Jun 3, 2021
@CKchen0726
update error info with the new limit in kubernetes#98753
918246d
@CKchen0726 CKchen0726 mentioned this pull request Jun 3, 2021
Merged
k8s-ci-robot added a commit that referenced this pull request Jun 14, 2021
@k8s-ci-robot
Merge pull request #102550 from CKchen0726/fix_log_error_for_mr98753
5aedca2 
update error info with the new limit in #98753
@pacoxu pacoxu mentioned this pull request Aug 5, 2021
Merged
@msau42 msau42 mentioned this pull request Aug 9, 2021
Closed
@Jiawei0227 Jiawei0227 mentioned this pull request Aug 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@saad-ali saad-ali saad-ali approved these changes

@ohbus ohbus ohbus approved these changes

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@liggitt liggitt

@saad-ali saad-ali

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
API Reviews
Status: API review completed, 1.21
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Jiawei0227 @k8s-ci-robot @saad-ali @fejta-bot @vpnachev @liggitt @ohbus