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

common/plugins: replaced sysctl invocation with echo redirect #2789

Merged
merged 1 commit into from
Feb 13, 2018
Merged

common/plugins: replaced sysctl invocation with echo redirect #2789

merged 1 commit into from
Feb 13, 2018

Conversation

aanm
Copy link
Member

@aanm aanm commented Feb 11, 2018

Signed-off-by: André Martins andre@cilium.io

Summary of changes: replaced sysctl invocation with echo redirect

replaced sysctl invocation with echo redirects

@aanm aanm added kind/bug This is a bug in the Cilium logic. priority/medium This is considered important, but not urgent. area/daemon Impacts operation of the Cilium daemon. release-note/bug This PR fixes an issue in a previous release of Cilium. kind/community-report This was reported by a user in the Cilium community, eg via Slack. labels Feb 11, 2018
@aanm aanm requested a review from a team as a code owner February 11, 2018 16:56
@aanm aanm force-pushed the replace-sysctl-with-echo-redirect branch from b343bce to f26e5a0 Compare February 11, 2018 17:39
@aanm
Copy link
Member Author

aanm commented Feb 11, 2018

test-me-please

@aanm
Copy link
Member Author

aanm commented Feb 11, 2018

@joestringer can you take a look at this? it looks like it's failing because of cilium_health doesn't exist:

Feb 11 17:38:49 k8s2 cilium-agent[32746]: level=debug msg="Created veth pair" vethPair="[cilium cilium_health]"
Feb 11 17:38:49 k8s2 cilium-agent[32746]: level=debug msg="Cannot establish connection to local cilium instance" error="Get http://%2Fvar%2Frun%2Fcilium%2Fcilium.sock/v1beta/healthz: dial unix /var/run/cilium/cilium.sock: connect: no such file or directory"
Feb 11 17:38:49 k8s2 cilium-agent[32746]: level=fatal msg="Error while creating cilium-health veth" error="unable to disable rp_filter on cilium_health: (exit status 127) sh: 1: echo 0 > /proc/sys/net/ipv4/conf/cilium_health/rp_filter: not found\n"

@joestringer
Copy link
Member

Is exit status 127 coming from sh?

@aanm aanm force-pushed the replace-sysctl-with-echo-redirect branch 2 times, most recently from 3883a5f to 53468c3 Compare February 11, 2018 21:54
@aanm
Copy link
Member Author

aanm commented Feb 11, 2018

test-me-please

@aanm
Copy link
Member Author

aanm commented Feb 11, 2018

@joestringer I replicate the sysctl behavior so this will work.

@borkmann
Copy link
Member

I just pulled the PR locally and works fine for me.

borkmann
borkmann reviewed Feb 12, 2018
View reviewed changes
common/plugins/add_endpoint.go Outdated
return nil, nil, fmt.Errorf("unable to open rp_filter configuration file of %s: %s",
lxcIfName, err)
}
std := exec.Command("echo", "0")
Copy link
Member

Choose a reason for hiding this comment

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

Could we just open the file and write 4 zero-bytes directly into it? Then we don't need the additional exec with echo since this is doing the same, but writing 0 in there directly is more robust / less complicated. C equivalent I would just use fd = open(path, O_WRONLY) to open the procfs file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@borkmann the sysctl command uses the os.O_WRONLY|os.O_CREATE|os.O_TRUNC

Copy link
Member

Choose a reason for hiding this comment

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

Ok, lets keep it then.

@borkmann
Copy link
Member

There is also one more dependency on sysctl in cmdAdd() in plugins/cilium-cni/cilium-cni.go, so this would need similar conversion.

@aanm aanm force-pushed the replace-sysctl-with-echo-redirect branch from 53468c3 to 69bc9c7 Compare February 12, 2018 11:15
@aanm aanm requested a review from a team February 12, 2018 11:15
@aanm
Copy link
Member Author

aanm commented Feb 12, 2018

test-me-please

@raybejjani
Copy link
Contributor

Is this an opportunity to put the various ways we invoke sysctl (and now just echo) into a package? I haven't read the other changes but I'm guessing a lot of it is similar or the same. This would then allow us to do any detection and error reporting in a consistent manner too.
A drive-by comment, so feel free to ignore me.

borkmann
borkmann reviewed Feb 12, 2018
View reviewed changes
common/plugins/add_endpoint.go Outdated
return nil, nil, fmt.Errorf("unable to open rp_filter configuration file of %s: %s",
lxcIfName, err)
}
_, err = f.WriteString("0\n")
if err != nil {
return nil, nil, fmt.Errorf("unable to disable rp_filter on %s: %s",
Copy link
Member

Choose a reason for hiding this comment

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

Should we just f.Close() here in case write fails?

@aanm aanm force-pushed the replace-sysctl-with-echo-redirect branch from 69bc9c7 to d104fad Compare February 12, 2018 12:23
@aanm
Copy link
Member Author

aanm commented Feb 12, 2018

test-me-please

@aanm aanm force-pushed the replace-sysctl-with-echo-redirect branch from d104fad to 1c29ce0 Compare February 12, 2018 15:17
@aanm
Copy link
Member Author

aanm commented Feb 12, 2018

test-me-please

@aanm aanm force-pushed the replace-sysctl-with-echo-redirect branch from 1c29ce0 to e5e3b28 Compare February 12, 2018 16:09
@aanm
common/plugins: replaced sysctl invocation with echo redirect
fa65102 
Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm force-pushed the replace-sysctl-with-echo-redirect branch from e5e3b28 to fa65102 Compare February 12, 2018 16:09
@aanm
Copy link
Member Author

aanm commented Feb 12, 2018

test-me-please

borkmann
borkmann approved these changes Feb 12, 2018
View reviewed changes
Copy link
Member

@borkmann borkmann left a comment

Choose a reason for hiding this comment

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

LGTM. The last err check in f.Close() and the omitted one in case of write fail is due to potential EIO when write was not committed yet?

@aanm aanm merged commit 6a6ced4 into master Feb 13, 2018
@aanm aanm deleted the replace-sysctl-with-echo-redirect branch February 13, 2018 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borkmann borkmann borkmann approved these changes

@nebril nebril nebril approved these changes

@joestringer joestringer Awaiting requested review from joestringer

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. kind/bug This is a bug in the Cilium logic. kind/community-report This was reported by a user in the Cilium community, eg via Slack. priority/medium This is considered important, but not urgent. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aanm @joestringer @borkmann @raybejjani @nebril