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

Relax csiNodeIDMaxLength to longer limit #98753

Merged
merged 1 commit into from Mar 9, 2021

Conversation

Jiawei0227
Copy link
Contributor

@Jiawei0227 Jiawei0227 commented Feb 4, 2021

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 kind/cleanup size/XS sig/storage cncf-cla: yes labels Feb 4, 2021
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Feb 4, 2021

@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 needs-priority labels Feb 4, 2021
@k8s-ci-robot k8s-ci-robot requested review from deads2k and saad-ali Feb 4, 2021
@Jiawei0227
Copy link
Contributor Author

@Jiawei0227 Jiawei0227 commented Feb 4, 2021

/retest

1 similar comment
@Jiawei0227
Copy link
Contributor Author

@Jiawei0227 Jiawei0227 commented Feb 4, 2021

/retest

Copy link
Member

@saad-ali saad-ali left a comment

/lgtm
/approve

/hold

Hold until CSI 1.4 is cut with this change

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold lgtm labels Feb 5, 2021
@saad-ali
Copy link
Member

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

@saad-ali 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 and removed lgtm labels Mar 8, 2021
Update csiNodeIDMaxLength to 192 bytes
@k8s-ci-robot k8s-ci-robot added size/L and removed size/XS 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

@Jiawei0227 Jiawei0227 commented Mar 8, 2021

/retest

1 similar comment
@Jiawei0227
Copy link
Contributor Author

@Jiawei0227 Jiawei0227 commented Mar 9, 2021

/retest

Copy link
Member

@liggitt liggitt left a comment

/lgtm
/approve

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

@liggitt liggitt Mar 9, 2021

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

@Jiawei0227 Jiawei0227 Mar 9, 2021

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 approved labels Mar 9, 2021
@Jiawei0227 Jiawei0227 changed the title Relax csiNodeIDMaxLength to lenger limit Relax csiNodeIDMaxLength to longer limit Mar 9, 2021
@fejta-bot
Copy link

@fejta-bot fejta-bot commented Mar 9, 2021

/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
Copy link

@ohbus ohbus left a comment

/lgtm

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 9, 2021

@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

@k8s-ci-robot k8s-ci-robot commented Mar 9, 2021

[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
15 of 17 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 9, 2021
@vpnachev
Copy link
Contributor

@vpnachev 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

@Jiawei0227 Jiawei0227 commented Apr 5, 2021

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

I updated the release notes. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved cncf-cla: yes kind/cleanup lgtm needs-priority needs-triage release-note sig/apps sig/storage size/L
Projects
API Reviews
API review completed, 1.21
Development

Successfully merging this pull request may close these issues.

None yet

7 participants