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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go
View file
Open in desktop
Expand Up @@ -45,8 +45,21 @@ func NewAWSElasticBlockStoreCSITranslator() InTreePlugin {
return &awsElasticBlockStoreCSITranslator{}
}

// TranslateInTreeStorageClassParametersToCSI translates InTree EBS storage class parameters to CSI storage class
// TranslateInTreeStorageClassToCSI translates InTree EBS storage class parameters to CSI storage class
func (t *awsElasticBlockStoreCSITranslator) TranslateInTreeStorageClassToCSI(sc *storage.StorageClass) (*storage.StorageClass, error) {
params := map[string]string{}

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.

default:
params[k] = v
}
}

sc.Parameters = params

return sc, nil
}

Expand Down
48 changes: 48 additions & 0 deletions staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs_test.go
View file
Open in desktop
Expand Up @@ -17,7 +17,10 @@ limitations under the License.
package plugins

import (
"reflect"
"testing"

storage "k8s.io/api/storage/v1"
)

func TestKubernetesVolumeIDToEBSVolumeID(t *testing.T) {
Expand Down Expand Up @@ -64,3 +67,48 @@ func TestKubernetesVolumeIDToEBSVolumeID(t *testing.T) {
})
}
}

func TestTranslateEBSInTreeStorageClassToCSI(t *testing.T) {
translator := NewAWSElasticBlockStoreCSITranslator()

cases := []struct {
name string
sc *storage.StorageClass
expSc *storage.StorageClass
expErr bool
}{
{
name: "translate normal",
sc: NewStorageClass(map[string]string{"foo": "bar"}, nil),
expSc: NewStorageClass(map[string]string{"foo": "bar"}, nil),
},
{
name: "translate empty map",
sc: NewStorageClass(map[string]string{}, nil),
expSc: NewStorageClass(map[string]string{}, nil),
},

{
name: "translate with fstype",
sc: NewStorageClass(map[string]string{"fstype": "ext3"}, nil),
expSc: NewStorageClass(map[string]string{"csi.storage.k8s.io/fstype": "ext3"}, nil),
},
}

for _, tc := range cases {
t.Logf("Testing %v", tc.name)
got, err := translator.TranslateInTreeStorageClassToCSI(tc.sc)
if err != nil && !tc.expErr {
t.Errorf("Did not expect error but got: %v", err)
}

if err == nil && tc.expErr {
t.Errorf("Expected error, but did not get one.")
}

if !reflect.DeepEqual(got, tc.expSc) {
t.Errorf("Got parameters: %v, expected :%v", got, tc.expSc)
}

}
}