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

Create volumes in outpost for outpost instances #561

Merged
merged 1 commit into from
Sep 30, 2020

Conversation

ayberk
Copy link
Contributor

@ayberk ayberk commented Sep 15, 2020

Is this a bug fix or adding new feature?
Bug Fix

What is this PR about? / Why do we need it?
Outposts Documentation for background.

Currently, we're running oblivious to outposts. Volumes are created in the parent region instead of the outpost itself. In order to fix it, we need to pass the outpostArn to CreateVolume.

I'm leveraging the outpost-arn instance metadata to determine if we're in an outpost instance. There are two possible responses from the endpoint:

  • The outpost-arn if it's an outpost instance
  • 404 if it's a regular non-outpost instance.

I've verified the behavior using this test code on a test outpost.

What testing is done?
Tested on an actual outpost instance and verified it works.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 15, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @ayberk. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 15, 2020
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 15, 2020
@coveralls
Copy link

coveralls commented Sep 15, 2020

Pull Request Test Coverage Report for Build 1231

  • 87 of 92 (94.57%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.9%) to 81.134%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/driver/controller.go 46 48 95.83%
pkg/cloud/cloud.go 7 10 70.0%
Totals Coverage Status
Change from base Build 1225: 0.9%
Covered Lines: 1574
Relevant Lines: 1940

💛 - Coveralls

@wongma7
Copy link
Contributor

wongma7 commented Sep 15, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 15, 2020
@ayberk ayberk force-pushed the outpost_fix branch 4 times, most recently from 24d2a10 to 77006e8 Compare September 16, 2020 18:45
@ayberk
Copy link
Contributor Author

ayberk commented Sep 16, 2020

/retest

@ayberk ayberk force-pushed the outpost_fix branch 2 times, most recently from cb5f99f to f58f2e9 Compare September 16, 2020 21:06
@ayberk ayberk changed the title [WIP] Create volumes in outpost for outpost instances Create volumes in outpost for outpost instances Sep 16, 2020
@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 Sep 16, 2020
@ayberk ayberk requested a review from wongma7 September 17, 2020 00:00
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 19, 2020
@ayberk ayberk force-pushed the outpost_fix branch 2 times, most recently from 8d5671f to a7ad0a0 Compare September 19, 2020 01:57
@ayberk
Copy link
Contributor Author

ayberk commented Sep 21, 2020

/retest

1 similar comment
@ayberk
Copy link
Contributor Author

ayberk commented Sep 21, 2020

/retest

@wongma7
Copy link
Contributor

wongma7 commented Sep 22, 2020

lgtm but i think there is an edgecase where GetDiskByName returns an existing volume that needs tobe handled.
/hold
for end to end test with real outpost

@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 Sep 22, 2020
@wongma7
Copy link
Contributor

wongma7 commented Sep 23, 2020

/lgtm
nice work!!!
Please post testing result, like the output of kubectl get pv -o yaml <name of some outpost pv created by the driver> and/or kubectl get csinodeinfo -o yaml <name of some outpost node where the csi driver is installed> for sanity check then let's unhold and merge.. If you create a cluster with only outpost nodes, it should be possible to reuse the e2e tests, if there is a mix of outpost with non outpost nodes then I guess it will be random whether the test pods get scheduled to the outpost nodes or not...

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2020
With this commit we start passing the outpost arn (if it's present) to EBS.
Before this commit, when the request came from an outpost instance,
we would ask the EBS to create the volume in the parent AZ, which is not
the expected behavior.

In order to determine if we're running in an outpost instance, we use
the ec2 instance metadata "outpost-arn". It returns a '404' for
non-outpost instances or the outpost-arn as string otherwise. Now we
include it in the topology requiment and pass it along to CreateVolume
request if it's present.

Automated e2e tests can be considerd impossible for this case as getting
an outpost rack is not an easy task.
@ayberk
Copy link
Contributor Author

ayberk commented Sep 30, 2020

I made minor changes and was able to run the dynamic provisioning example on a test outpost. Volume was attached properly.

ubuntu@ip-10-0-1-194:~/aws-ebs-csi-driver/examples/kubernetes/dynamic-provisioning$ kubectl describe pv pvc-ecbd15ab-0a37-4910-972d-2f9a1487a8e6
Name:              pvc-ecbd15ab-0a37-4910-972d-2f9a1487a8e6
Labels:            <none>
Annotations:       pv.kubernetes.io/provisioned-by: ebs.csi.aws.com
Finalizers:        [kubernetes.io/pv-protection external-attacher/ebs-csi-aws-com]
StorageClass:      ebs-sc
Status:            Bound
Claim:             default/ebs-claim
Reclaim Policy:    Delete
Access Modes:      RWO
VolumeMode:        Filesystem
Capacity:          4Gi
Node Affinity:
  Required Terms:
    Term 0:        topology.ebs.csi.aws.com/partition in [aws]
                   topology.ebs.csi.aws.com/account-id in [foo]
                   topology.ebs.csi.aws.com/outpost-id in [bar]
                   topology.ebs.csi.aws.com/zone in [us-east-1d]
                   topology.ebs.csi.aws.com/region in [us-east-1]
Message:
Source:
    Type:              CSI (a Container Storage Interface (CSI) volume source)
    Driver:            ebs.csi.aws.com
    FSType:            ext4
    VolumeHandle:      vol-0372dc90eb689430d
    ReadOnly:          false
    VolumeAttributes:      storage.kubernetes.io/csiProvisionerIdentity=1601421491105-8081-ebs.csi.aws.com
Events:                <none>
ubuntu@ip-10-0-1-194:~/aws-ebs-csi-driver/examples/kubernetes/dynamic-provisioning$ kubectl describe node ip-10-0-1-194
Name:               ip-10-0-1-194
Roles:              master
Labels:             
                    beta.kubernetes.io/arch=amd64
                    beta.kubernetes.io/os=linux
                    kubernetes.io/arch=amd64
                    kubernetes.io/hostname=ip-10-0-1-194
                    kubernetes.io/os=linux
                    node-role.kubernetes.io/master=
                    topology.ebs.csi.aws.com/account-id=foo
                    topology.ebs.csi.aws.com/outpost-id=bar
                    topology.ebs.csi.aws.com/partition=aws
                    topology.ebs.csi.aws.com/region=us-east-1
                    topology.ebs.csi.aws.com/zone=us-east-1d
Annotations:        csi.volume.kubernetes.io/nodeid: {"ebs.csi.aws.com":"i-0954d4e23b3bf2da0"}
                    kubeadm.alpha.kubernetes.io/cri-socket: /var/run/dockershim.sock
                    node.alpha.kubernetes.io/ttl: 0
                    projectcalico.org/IPv4Address: 10.0.1.194/24
                    projectcalico.org/IPv4VXLANTunnelAddr: 192.168.245.0
                    volumes.kubernetes.io/controller-managed-attach-detach: true
ubuntu@ip-10-0-1-194:~$ kubectl get csinode -o yaml ip-10-0-1-194
apiVersion: storage.k8s.io/v1
kind: CSINode
metadata:
  creationTimestamp: "2020-09-29T21:33:12Z"
  managedFields:
  - apiVersion: storage.k8s.io/v1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:ownerReferences:
          .: {}
          k:{"uid":"64ba5bc7-45f6-494f-8e71-af9575184612"}:
            .: {}
            f:apiVersion: {}
            f:kind: {}
            f:name: {}
            f:uid: {}
      f:spec:
        f:drivers:
          k:{"name":"ebs.csi.aws.com"}:
            .: {}
            f:allocatable:
              .: {}
              f:count: {}
            f:name: {}
            f:nodeID: {}
            f:topologyKeys: {}
    manager: kubelet
    operation: Update
    time: "2020-09-30T00:29:58Z"
  name: ip-10-0-1-194
  ownerReferences:
  - apiVersion: v1
    kind: Node
    name: ip-10-0-1-194
    uid: 64ba5bc7-45f6-494f-8e71-af9575184612
  resourceVersion: "29252"
  selfLink: /apis/storage.k8s.io/v1/csinodes/ip-10-0-1-194
  uid: 41e397d9-eb3a-4376-b60c-96a2b6c81ba2
spec:
  drivers:
  - allocatable:
      count: 25
    name: ebs.csi.aws.com
    nodeID: i-0954d4e23b3bf2da0
    topologyKeys:
    - topology.ebs.csi.aws.com/account-id
    - topology.ebs.csi.aws.com/outpost-id
    - topology.ebs.csi.aws.com/partition
    - topology.ebs.csi.aws.com/region
    - topology.ebs.csi.aws.com/zone

@ayberk
Copy link
Contributor Author

ayberk commented Sep 30, 2020

/retest

@wongma7
Copy link
Contributor

wongma7 commented Sep 30, 2020

/lgtm
/approve

@wongma7
Copy link
Contributor

wongma7 commented Sep 30, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 30, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ayberk, 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 Sep 30, 2020
@k8s-ci-robot k8s-ci-robot merged commit 406b203 into kubernetes-sigs:master Sep 30, 2020
@ayberk ayberk deleted the outpost_fix branch September 30, 2020 22:48
@sandeep173254
Copy link

sandeep173254 commented Jul 11, 2024

@ayberk I am trying to do a poc with outpost but every time the volume is created in the parent region (no OutpostARN). my ec2 instance metadata returns OutpostARN. Any pointers would be helpful

Error message that i get because my instance is in Outpost but the volume is not:

AttachVolume.Attach failed for volume "pvc-1155bee4-0186-4a43-8047-a30b33d3bcdb" : rpc error: code = Internal desc = Could not attach volume "vol-0a41058da77cf512a" to node "i-09027f5cfb7d1455f": could not attach volume "vol-0a41058da77cf512a" to node "i-09027f5cfb7d1455f": operation error EC2: AttachVolume, https response error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants