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 support of Azure Blob storage to VFS #10258

Merged
merged 1 commit into from
Nov 22, 2020
Merged

Add support of Azure Blob storage to VFS #10258

merged 1 commit into from
Nov 22, 2020

Conversation

kenji-cloudnatix
Copy link
Contributor

The schema is "azureblob".

azureClient provides two ways to set up credential. One approach is to
use an account key stored in env var. This approach is used when
accessing Blob from kops CLI. The second approach is to retrieve
credentials from Instance Metadata Service. This works only when
azureClient is created on a VM that has sufficient privilege to access
a specified blob. This approach is used from nodeup, etcd-manager,
etc.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 18, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @kenji-cloudnatix. Thanks for your PR.

I'm waiting for a kubernetes 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 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 18, 2020
@kenji-cloudnatix kenji-cloudnatix mentioned this pull request Nov 18, 2020
Merged
@hakman
Copy link
Member

hakman commented Nov 18, 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 Nov 18, 2020
hakman
hakman requested changes Nov 18, 2020
View reviewed changes
Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

Looks like a great start for merging the big PR. Just a few nits from me.

util/pkg/vfs/azureblob.go Outdated Show resolved Hide resolved
util/pkg/vfs/azureblob.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2020
kenji-cloudnatix
kenji-cloudnatix commented Nov 18, 2020
View reviewed changes
Copy link
Contributor Author

@kenji-cloudnatix kenji-cloudnatix left a comment

Choose a reason for hiding this comment

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

Thanks for the quick review, @hakman ! Changed AzureBlobPath to implement HashHash. Also inlined the cost as suggested.

util/pkg/vfs/azureblob.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2020
rifelpet
rifelpet requested changes Nov 19, 2020
View reviewed changes
util/pkg/vfs/azureblob.go Show resolved Hide resolved
util/pkg/vfs/azureblob.go Outdated Show resolved Hide resolved
util/pkg/vfs/azureclient.go Show resolved Hide resolved
kenji-cloudnatix
kenji-cloudnatix commented Nov 19, 2020
View reviewed changes
Copy link
Contributor Author

@kenji-cloudnatix kenji-cloudnatix left a comment

Choose a reason for hiding this comment

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

Thanks for the review, @rifelpet !

util/pkg/vfs/azureblob.go Show resolved Hide resolved
util/pkg/vfs/azureblob.go Outdated Show resolved Hide resolved
util/pkg/vfs/azureclient.go Show resolved Hide resolved
@rifelpet
Copy link
Member

This looks good to me but I'll wait for input from others before merging. We can bring it up at the Kops office hours tomorrow
/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 19, 2020
@hakman
Copy link
Member

hakman commented Nov 19, 2020

I am also OK with this.
/lgtm

@kenji-cloudnatix
Copy link
Contributor Author

Thanks, both! How should I merge? Rebase and squash all the commits into one?

@hakman
Copy link
Member

hakman commented Nov 19, 2020

We will have a short review on Friday during office hours and probably the final LGTM.
I think would be a good idea to squash, so that it can be merged if no other issues are found.

@kenji-cloudnatix
Add support of Azure Blob storage to VFS
4555c0b 
The schema is "azureblob".

azureClient provides two ways to set up credential. One approach is to
use an account key stored in env var. This approach is used when
accessing Blob from kops CLI. The second approach is to retrieve
credentials from Instance Metadata Service. This works only when
azureClient is created on a VM that has sufficient privilege to access
a specified blob. This approach is used from nodeup, etcd-manager,
etc.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2020
@kenji-cloudnatix
Copy link
Contributor Author

Sounds good! Squashed. Will also join the office hour.

@hakman
Copy link
Member

hakman commented Nov 19, 2020

Sounds good! Squashed. Will also join the office hour.

Perfect. Thanks!

@hakman
Copy link
Member

hakman commented Nov 22, 2020

/cc @justinsb

justinsb
justinsb reviewed Nov 22, 2020
View reviewed changes
util/pkg/vfs/context.go
}

if u.Scheme != "azureblob" {
return nil, fmt.Errorf("invalid Azure Blob schem: %q", p)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: scheme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks for catching! Will create a small PR to fix.

@justinsb
Copy link
Member

This looks great - so excited to see this - thanks @kenji-cloudnatix and all the reviewers :-)

/approve
/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 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, kenji-cloudnatix

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 22, 2020
@k8s-ci-robot k8s-ci-robot merged commit 47b407c into kubernetes:master Nov 22, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@rifelpet rifelpet rifelpet requested changes

@hakman hakman hakman requested changes

@KashifSaadat KashifSaadat Awaiting requested review from KashifSaadat

@olemarkus olemarkus Awaiting requested review from olemarkus

Assignees

@justinsb justinsb

@rifelpet rifelpet

@hakman hakman

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kenji-cloudnatix @k8s-ci-robot @hakman @rifelpet @justinsb