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.

Sign up for GitHub

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

Add translation logic for EBS storage class fstype parameter #85010

Merged
merged 1 commit into from Nov 14, 2019
Merged

Add translation logic for EBS storage class fstype parameter #85010

merged 1 commit into from Nov 14, 2019

Conversation

leakingtapan
Copy link

@leakingtapan leakingtapan commented Nov 8, 2019 •
edited

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Add more CSI translation logic for EBS CSI migration. The logic covers the translation of storage class parameter from fstype to csi.storage.k8s.io/fstype. Also added unit test for it.

/cc @davidz627 @ddebroy

TODOS:

  • run migration e2e test

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

None

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

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 8, 2019
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 8, 2019
@leakingtapan
Copy link
Author

/hold for e2e testing

@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 Nov 8, 2019
@leakingtapan
Copy link
Author

/retest

ddebroy
ddebroy reviewed Nov 9, 2019
View reviewed changes
Copy link
Member

@ddebroy ddebroy left a comment •
edited

Choose a reason for hiding this comment

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

/lgtm

staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go
for k, v := range sc.Parameters {
switch strings.ToLower(k) {
case "fstype":
params["csi.storage.k8s.io/fstype"] = v
Copy link
Member

@ddebroy ddebroy Nov 9, 2019 •
edited

Choose a reason for hiding this comment

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

Do you also plan to convert zones to topologies here in the future like in

kubernetes/staging/src/k8s.io/csi-translation-lib/plugins/gce_pd.go

Line 120 in 41d9522

generatedTopologies = generateToplogySelectors(GCEPDTopologyKey, []string{v})

Copy link
Author

Choose a reason for hiding this comment

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

Since zone and zones are deprecated, I would like to not to do these translations if possible. Let me know if you think otherwise @ddebroy @davidz627

Copy link
Member

Choose a reason for hiding this comment

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

IMO we need to support the API. A cluster that is updated to migrated version and uses zone and zones should keep working.

Copy link
Author

Choose a reason for hiding this comment

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

Does that mean zone and zones are not really deprecated? And thinking further, any parameters within storageclass parameters are not deprecateale? If this is the case, the deprecation here is confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

@leakingtapan deprecation does not mean we do not support it anymore - I believe it means we don't recommend usage and that support will be removed in a future release. Therefore until that functionality is "removed" (not sure when) we will need to support it during migration

Copy link
Member

Choose a reason for hiding this comment

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

We can argue under which paragraph in deprecation policy it falls, IMO it's part of storage.k8s.io/v1 StorageClass API object. And thus we may remove things from there only after introducing storage.k8s.io/v2.

Copy link
Author

Choose a reason for hiding this comment

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

@jsafrane @davidz627 thx for the inputs and they are fair enough. I will add this translation then. But could I open a new PR for the zone/zones translation? since I'm not sure if I have time for it this week since the code freeze is soon.

@davidz627
Copy link
Contributor

/sig storage
/priority important-soong
/lgtm
/approve

@leakingtapan to open a new PR ASAP for the topology/zone/zones fix.

@k8s-ci-robot
Copy link
Contributor

@davidz627: The label(s) priority/important-soong cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

/sig storage
/priority important-soong
/lgtm
/approve

@leakingtapan to open a new PR ASAP for the topology/zone/zones fix.

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 sig/storage Categorizes an issue or PR as relevant to SIG Storage. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 13, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, leakingtapan

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2019
@leakingtapan
Copy link
Author

tested by building a new provisioner and it works that fstype is translated to csi.storage.k8s.io/fstype

/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 Nov 14, 2019
@davidz627
Copy link
Contributor

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 14, 2019
@leakingtapan leakingtapan mentioned this pull request Nov 14, 2019
Merged
@k8s-ci-robot k8s-ci-robot merged commit 346e6b5 into kubernetes:master Nov 14, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsafrane jsafrane jsafrane left review comments

@ddebroy ddebroy ddebroy left review comments

@davidz627 davidz627 Awaiting requested review from davidz627

Assignees

@davidz627 davidz627

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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@leakingtapan @davidz627 @k8s-ci-robot @jsafrane @ddebroy