Skip to content
This repository was archived by the owner on Feb 5, 2020. It is now read-only.

azure: add udev rules needed by k8s cloudprovider#1618

Merged
alexsomesan merged 2 commits intocoreos:masterfrom
colemickens:azure-udev
Aug 9, 2017
Merged

azure: add udev rules needed by k8s cloudprovider#1618
alexsomesan merged 2 commits intocoreos:masterfrom
colemickens:azure-udev

Conversation

@colemickens
Copy link
Copy Markdown
Contributor

@colemickens colemickens commented Aug 8, 2017

Fixes #1614 by adding udev rules needed by the Azure cloudprovider. See this comment for context: kubernetes/kubernetes#50150 (comment)

@coreosbot
Copy link
Copy Markdown

coreosbot commented Aug 8, 2017

Can one of the admins verify this patch?

1 similar comment
@coreosbot
Copy link
Copy Markdown

coreosbot commented Aug 8, 2017

Can one of the admins verify this patch?

@colemickens colemickens mentioned this pull request Aug 8, 2017
Closed
@colemickens
Copy link
Copy Markdown
Contributor Author

colemickens commented Aug 8, 2017

I'm not sure if it would be more appropriate to add this to Container Linux itself, but this seems like a potentially desirable fix in the case of expediency.

Also, this was tested with: https://gist.github.com/colemickens/0cd6a12fdbf5a8b5040724a0b8e900fd.

@squat
Copy link
Copy Markdown
Contributor

squat commented Aug 8, 2017

@colemickens, thanks for contributing this. In order to reduce code duplication, could you implement this in a module following the pattern that the tx-off.service uses?

squat
squat reviewed Aug 8, 2017
View reviewed changes
modules/azure/master-as/resources/66-azure-storage.rules Outdated
ENV{DEVTYPE}=="disk", SYMLINK+="disk/azure/$env{fabric_name}"
ENV{DEVTYPE}=="partition", SYMLINK+="disk/azure/$env{fabric_name}-part%n"

LABEL="azure_end" No newline at end of file
Copy link
Copy Markdown
Contributor

@squat squat Aug 8, 2017

Choose a reason for hiding this comment

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

This should probably have a newline

@colemickens
Copy link
Copy Markdown
Contributor Author

colemickens commented Aug 8, 2017

@squat Both requested changes have been made. Let me know and I can squash.

@alexsomesan
Copy link
Copy Markdown
Contributor

alexsomesan commented Aug 8, 2017

@colemickens I'm giving this a quick manual test and will then merge if everything works fine.

@philips
Copy link
Copy Markdown
Contributor

philips commented Aug 8, 2017

How does this work on other distros?

Quentin-M
Quentin-M reviewed Aug 8, 2017
View reviewed changes
modules/azure/worker-as/resources/66-azure-storage.rules
ENV{DEVTYPE}=="disk", SYMLINK+="disk/azure/$env{fabric_name}"
ENV{DEVTYPE}=="partition", SYMLINK+="disk/azure/$env{fabric_name}-part%n"

LABEL="azure_end" No newline at end of file
Copy link
Copy Markdown
Contributor

@Quentin-M Quentin-M Aug 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

missing new line

@alexsomesan
Copy link
Copy Markdown
Contributor

alexsomesan commented Aug 8, 2017

I was able to confirm this is working by spinning up a cluster from this branch.
I used the manifests in the gist provided by @colemickens.

 alex@Alexs-MBP  ~/go/src/github.com/coreos/tectonic-installer   azure-udev  kubectl get pvc
NAME                 STATUS    VOLUME                                     CAPACITY   ACCESSMODES   STORAGECLASS   AGE
dd-managed-fast-01   Bound     pvc-1da381a2-7c65-11e7-978b-000d3a2a4b59   128Gi      RWO           managedfast    8m
 alex@Alexs-MBP  ~/go/src/github.com/coreos/tectonic-installer   azure-udev  kubectl get pv
NAME                                       CAPACITY   ACCESSMODES   RECLAIMPOLICY   STATUS    CLAIM                        STORAGECLASS   REASON    AGE
pvc-1da381a2-7c65-11e7-978b-000d3a2a4b59   128Gi      RWO           Delete          Bound     default/dd-managed-fast-01   managedfast              7m
 alex@Alexs-MBP  ~/go/src/github.com/coreos/tectonic-installer   azure-udev  kubectl get pods
NAME                              READY     STATUS    RESTARTS   AGE
test-azure-disk-396068938-kdbnh   2/2       Running   0          8m

@colemickens
Copy link
Copy Markdown
Contributor Author

colemickens commented Aug 8, 2017

@philips In the latest 16.04 images (in Azure at least), these udev rules are included. On the upstream issue I've commented that this issue is likely to affect other distros and older ubuntu images.

alexsomesan
alexsomesan approved these changes Aug 9, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

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

This looks good so far. I'm going to merge it.
Thanks a lot, Cole!

Just FYI, there is a pending overhaul of the ignition modules into a single generic one, on which occasion this change will also go through a bit of refactoring.

@alexsomesan alexsomesan merged commit 90053fa into coreos:master Aug 9, 2017
@Quentin-M Quentin-M mentioned this pull request Aug 9, 2017
Merged
Quentin-M added a commit that referenced this pull request Aug 9, 2017
@Quentin-M
Merge pull request #1638 from Quentin-M/1.7.1_azpv
c468a26 
azure: add udev rules needed by k8s cloudprovider (#1618)
@euank euank mentioned this pull request Aug 10, 2017
Closed
squat pushed a commit to squat/tectonic-installer that referenced this pull request Sep 25, 2017
@colemickens @squat
azure: add udev rules needed by k8s cloudprovider (coreos#1618)
de32e84 
* azure: add udev rules needed by k8s cloudprovider

* respond to feedback
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@Quentin-M Quentin-M Quentin-M left review comments

@squat squat squat left review comments

@alexsomesan alexsomesan alexsomesan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alexsomesan alexsomesan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@colemickens @coreosbot @squat @alexsomesan @philips @Quentin-M