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

Turn CSIMigrationAWS on by default #106098

Merged
merged 1 commit into from Nov 17, 2021
Merged

Turn CSIMigrationAWS on by default #106098

merged 1 commit into from Nov 17, 2021

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented Nov 2, 2021

What type of PR is this?

/kind feature

What this PR does / why we need it:

Turn the feature on by default.
kubernetes/enhancements#1487

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Changed feature CSIMigrationAWS to on by default. This feature requires the AWS EBS CSI driver to be installed.

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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 Nov 2, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Nov 2, 2021

/sig storage

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 2, 2021
@reylejano reylejano mentioned this pull request Nov 15, 2021
Closed
@wongma7 wongma7 changed the title WIP: Turn CSIMigrationAWS on by default Turn CSIMigrationAWS on by default Nov 16, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2021
@wongma7

This comment has been minimized.

Sign in to view
@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 16, 2021
@wongma7

This comment has been minimized.

Sign in to view
@wongma7
Copy link
Contributor Author

wongma7 commented Nov 16, 2021

just that subPath test failing.

[Fail] [ebs-csi-migration] EBS CSI Migration [Driver: aws] [Testpattern: Dynamic PV (ntfs)][Feature:Windows] subPath [It] should support creating multiple subpath from same volumes [Slow] 
/home/ANT.AMAZON.COM/mattwon/go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/vendor/k8s.io/kubernetes/test/e2e/storage/testsuites/subpath.go:183

Ran 14 of 485 Specs in 2835.675 seconds
FAIL! -- 13 Passed | 1 Failed | 0 Pending | 471 Skipped
--- FAIL: TestEBSCSI (2835.68s)
FAIL

Ginkgo ran 1 suite in 47m19.30787132s
Test Suite Failed

details:

$ k version --short
Client Version: v1.16.15-eks-213c83-5-g704ce28f5eac91-dirty
Server Version: v1.20.7-eks-d88609
$ k get node ip-192-168-30-131.us-west-2.compute.internal -o wide
NAME                                           STATUS   ROLES    AGE   VERSION              INTERNAL-IP      EXTERNAL-IP      OS-IMAGE                         KERNEL-VERSION    CONTAINER-RUNTIME
ip-192-168-30-131.us-west-2.compute.internal   Ready    <none>   32d   v1.20.7-eks-135321   192.168.30.131   54.185.146.255   Windows Server 2019 Datacenter   10.0.17763.2183   docker://20.10.7
$ bash -c 'ginkgo --nodes=1 -v --focus="ebs-csi-migration.*ntfs" --skip="LinuxOnly|expand" ./tests/e2e-kubernetes/... -- --kubeconfig=$HOME/.kube/config --node-os-distro=windows --gce-zone=us-west-2a -- > windows.log.0'
$ cat tests/e2e-kubernetes/manifests.yaml 
# Manifest for Kubernetes external tests.
# See https://github.com/kubernetes/kubernetes/tree/master/test/e2e/storage/external

ShortName: ebs
StorageClass:
  FromFile:
    storageclass.yaml
    #SnapshotClass:
    #FromFile: snapshotclass.yaml
DriverInfo:
  Name: ebs.csi.aws.com
  SupportedSizeRange:
    Min: 1Gi
    Max: 16Ti
  SupportedFsType:
    #xfs: {}
    #ext4: {}
    ntfs: {}
  SupportedMountOption:
    dirsync: {}
  TopologyKeys: ["topology.ebs.csi.aws.com/zone"]
  Capabilities:
    persistence: true
    fsGroup: true
    block: true
    exec:
      true
      #snapshotDataSource: true
    pvcDataSource: false
    multipods: true
    controllerExpansion: true
    nodeExpansion: true
    volumeLimits: true
    topology: true

@wongma7 wongma7 mentioned this pull request Nov 16, 2021
Closed
@wongma7
Copy link
Contributor Author

wongma7 commented Nov 16, 2021 •
edited

I ran the test with up-to-date kubelet and csi-proxy and the vol_data.json: The system cannot find the file specified. logs went away however the test still failed.

I ran the test with the in-tree driver (with CSIMigration off) and the test failed as well.

So strictly speaking I don't think the failure must block migration since the same bug affects both in-tree and CSI...

Details on the bug:

The volume never gets detached, so PV never gets deleted, so the test fails. The fix is to take the volume offline, then detach succeeds. AWS official documentation recommends doing this https://docs.aws.amazon.com/AWSEC2/latest/WindowsGuide/ebs-detaching-volume.html#detach but neither the in-tree nor CSI driver do it, instead they just clear the write cache and remove all paths referencing the volume. This works in all other test cases so it's not clear yet why taking the volume offline is necessary only in the should support creating multiple subpath from same volumes case. Regardless the plugins should be following the official documentation

CSI issue just filed: kubernetes-sigs/aws-ebs-csi-driver#1116

in either case kernel prints this so I guess it has something to do with how kubelet does the subpath mounts/unmounts and taking disk offline avoids it?

The application System with process id 4 stopped the removal or ejection for the device PCI\VEN_1D0F&DEV_8061&SUBSYS_00001D0F&REV_00\3&13c0b0c5&1&F8.
The application \Device\HarddiskVolume1\Program Files\Kubernetes\kubelet.exe with process id 3456 stopped the removal or ejection for the device PCI\VEN_1D0F&DEV_8061&SUBSYS_00001D0F&REV_00\3&13c0b0c5&1&F8.

@wongma7
Copy link
Contributor Author

wongma7 commented Nov 16, 2021

/hold cancel

My testing is done, there is one failure that represents a bug in in-tree/CSI windows implementation.

@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 16, 2021
@nckturner
Copy link
Contributor

kubernetes-sigs/aws-ebs-csi-driver#1116 seems sufficient to track this, so:

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2021
@msau42
Copy link
Member

msau42 commented Nov 17, 2021

/lgtm
/approve

@msau42
Copy link
Member

msau42 commented Nov 17, 2021

/milestone v1.23

@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Nov 17, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, wongma7

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 17, 2021
@msau42
Copy link
Member

msau42 commented Nov 17, 2021

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 17, 2021
@k8s-ci-robot k8s-ci-robot merged commit 400ec74 into kubernetes:master Nov 17, 2021
vgramer added a commit to vgramer/kubeone that referenced this pull request Feb 7, 2022
@vgramer
AWS: install csi driver if k8s >1.23 even if cloud.external=false
6aeed9f 
since k8s 1.23 CSIMigrationAWS is turn by default and require installation of AWS EBS CSI driver even if we don't use external ccm (kubernetes/kubernetes#106098)

Signed-off-by: Vincent Gramer <vincent@kubermatic.com>
@vgramer vgramer mentioned this pull request Feb 7, 2022
Merged
vgramer added a commit to vgramer/kubeone that referenced this pull request Feb 7, 2022
@vgramer
AWS: install csi driver if k8s >1.23 even if cloud.external=false
1e61ecf 
since k8s 1.23 CSIMigrationAWS is turn by default and require installation of AWS EBS CSI driver even if we don't use external ccm (kubernetes/kubernetes#106098)

Signed-off-by: Vincent Gramer <vincent@kubermatic.com>
vgramer added a commit to vgramer/kubeone that referenced this pull request Feb 7, 2022
@vgramer
AWS: install csi driver if k8s >1.23 even if cloud.external=false
0e89d93 
since k8s 1.23 CSIMigrationAWS is turn by default and require installation of AWS EBS CSI driver even if we don't use external ccm (kubernetes/kubernetes#106098)

Signed-off-by: Vincent Gramer <vincent@kubermatic.com>
vgramer added a commit to vgramer/kubeone that referenced this pull request Feb 8, 2022
@vgramer
AWS: install csi driver if k8s >1.23 even if cloud.external=false
0cfc76d 
since k8s 1.23 CSIMigrationAWS is turn by default and require installation of AWS EBS CSI driver even if we don't use external ccm (kubernetes/kubernetes#106098)

Signed-off-by: Vincent Gramer <vincent@kubermatic.com>
vgramer added a commit to vgramer/kubeone that referenced this pull request Feb 9, 2022
@vgramer
AWS: install csi driver if k8s >1.23 even if cloud.external=false
4baedc9 
since k8s 1.23 CSIMigrationAWS is turn by default and require installation of AWS EBS CSI driver even if we don't use external ccm (kubernetes/kubernetes#106098)

Signed-off-by: Vincent Gramer <vincent@kubermatic.com>
kubermatic-bot pushed a commit to kubermatic/kubeone that referenced this pull request Feb 9, 2022
@vgramer
AWS: install csi driver if k8s >1.23 even if cloud.external=false (#1815
efd8f0c 
)

* refactor: move function MustParseConstraint from pkg/apis/kubeone/validation/validation.go to pkg/semverutil/helper.go

Signed-off-by: Vincent Gramer <vincent@kubermatic.com>

* AWS: install csi driver if k8s >1.23 even if cloud.external=false

since k8s 1.23 CSIMigrationAWS is turn by default and require installation of AWS EBS CSI driver even if we don't use external ccm (kubernetes/kubernetes#106098)

Signed-off-by: Vincent Gramer <vincent@kubermatic.com>
@carlory carlory mentioned this pull request Jan 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendandburns brendandburns Awaiting requested review from brendandburns

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@nckturner nckturner

@msau42 msau42

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/feature Categorizes issue or PR as related to a new feature. 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wongma7 @nckturner @msau42 @k8s-ci-robot