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

Ensure CNI dir exists before writing openshift CNI configuration under CNI dir #15064

Merged
merged 1 commit into from
Jul 13, 2017

Conversation

pravisankar
Copy link

@pravisankar pravisankar commented Jul 5, 2017

Found this issue on RHEL containerized openshift install.

Fixes: #14661
Fixes: #15147

@pravisankar
Copy link
Author

@openshift/networking @dcbw PTAL

Copy link
Contributor

@rajatchopra rajatchopra left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@@ -326,13 +332,17 @@ func (node *OsdnNode) Start() error {
}
}

if err := os.MkdirAll(CNI_DIR_PATH, os.ModePerm); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

FileMode could be 0755 maybe?

@pravisankar
Copy link
Author

[test]

Copy link
Contributor

@dcbw dcbw left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -38,6 +39,11 @@ import (
kexec "k8s.io/kubernetes/pkg/util/exec"
)

const (
CNI_DIR_PATH = "/etc/cni/net.d"
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't do constants like this - if this is only used in this package, should be camelCase

…r CNI dir

Found this issue on RHEL containerized openshift install.
@pravisankar
Copy link
Author

Changed constants to camelCase

@smarterclayton
Copy link
Contributor

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 23368fb

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3096/) (Base Commit: c52694b) (PR Branch Commit: 23368fb)

@smarterclayton smarterclayton merged commit afaf234 into openshift:master Jul 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants