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

Add initial support for OpenRC #73101

Merged
merged 1 commit into from May 10, 2019

Conversation

@oz123
Copy link
Contributor

commented Jan 19, 2019

  • Gentoo has init scripts for kubelet
  • Added a new method of the InitSystem Interface
    This helps issuing nicer messages when not on systemd.

This is a partial fix for

kubernetes/kubeadm#1295

@neolit123 ping.

What type of PR is this?

/kind feature

Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#1295

Special notes for your reviewer:

I am a new comer to go.
It seems to me we can reduce a lot of copy paste by adding another function:

func assertServiceCondition(cmd string, args []string, service string, shouldbe string) bool {
	outBytes, _ := exec.Command(cmd, args...).CombinedOutput()
	output := string(outBytes)
	if strings.Contains(output, shouldbe) {
		return false
	}
	return true
}

But this seemed already too much in one go. I'd be happy if this go merged first. A second PR can improve this.

Does this PR introduce a user-facing change?:

util/initsystem: add support for the OpenRC init system
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2019

Hi @oz123. 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.

pkg/util/initsystem/initsystem.go Outdated Show resolved Hide resolved
@dims

This comment has been minimized.

Copy link
Member

commented Jan 19, 2019

/ok-to-test

@neolit123
Copy link
Member

left a comment

thank you for working on this @oz123
i'm sure the Alpine users would appreciated this!

added some minor comments.
but overall LGTM.

pkg/util/initsystem/initsystem.go Outdated Show resolved Hide resolved
pkg/util/initsystem/initsystem.go Outdated Show resolved Hide resolved
pkg/util/initsystem/initsystem.go Outdated Show resolved Hide resolved
pkg/util/initsystem/initsystem.go Show resolved Hide resolved
pkg/util/initsystem/initsystem.go Outdated Show resolved Hide resolved
pkg/util/initsystem/initsystem.go Outdated Show resolved Hide resolved
pkg/util/initsystem/initsystem.go Outdated Show resolved Hide resolved
pkg/util/initsystem/initsystem.go Show resolved Hide resolved
pkg/util/initsystem/initsystem.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/preflight/checks.go Outdated Show resolved Hide resolved
@neolit123

This comment has been minimized.

Copy link
Member

commented Jan 19, 2019

/priority backlog

@neolit123

This comment has been minimized.

@oz123 oz123 force-pushed the oz123:kubeadm_openrc_support branch from e22b90e to 5417ada Jan 19, 2019

@oz123

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2019

/retest

@oz123

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2019

@neolit123 thank you for the quick and positive review.

@oz123 oz123 force-pushed the oz123:kubeadm_openrc_support branch from 5417ada to 26c26a3 Jan 19, 2019

@neolit123
Copy link
Member

left a comment

/approve
/assign @thockin @dchen1107

hopefully the initsystem.go maintainers see this PR soon.

@neolit123

This comment has been minimized.

Copy link
Member

commented Jan 19, 2019

@oz123

actually let's remove the kubeadm change...
my point was that we should either include all kubeadm changes at once or none of them. :)

let's keep the scope of this PR only to initsystem.go.

given that, please change the release note to:

util/initsystem: add support for the OpenRC init system

thank you.

@neolit123

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

/unassign @thockin
sorry missed the assigned luxas and unassigned thockin on Jan 26

@neolit123

This comment has been minimized.

@dixudx
dixudx approved these changes Mar 21, 2019
Copy link
Member

left a comment

/lgtm

@dmolik

This comment has been minimized.

Copy link

commented Apr 8, 2019

So what's the hold up? Is there anything I can do you help?

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

So what's the hold up? Is there anything I can do you help?

we need approvers for this actual area of the code we are changing.
they are already assigned.

Add initial support for OpenRC
 * Gentoo has init scripts for kubelet
 * Added a new method of the InitSystem Interface
   This helps issuing nicer messages when not on systemd.
 * OpenRCInitSystem.ServiceExists uses CombinedOutput because
   the behaviour of OpenRC is different from systemd.

This is a partial fix for

 kubernetes/kubeadm#1295

@oz123 oz123 force-pushed the oz123:kubeadm_openrc_support branch from 653c066 to 2a40ef4 Apr 8, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 8, 2019

@mrueg

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

OpenRC support for kubeadm would be a great addition, I'm one of the kubernetes packages for Gentoo. Anything I can do to help out here and get this change included?

@neolit123

This comment has been minimized.

Copy link
Member

commented May 6, 2019

we can try pinging SIG Node about this PR or bringing it as an agenda item for their Zoom meeting.
if anyone has the bandwidth for that, please go ahead.

@timothysc timothysc added the approved label May 10, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented May 10, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 10, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: neolit123, oz123

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 merged commit aa84b99 into kubernetes:master May 10, 2019

18 checks passed

cla/linuxfoundation oz123 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Context retired. Status moved to "pull-kubernetes-dependencies".
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@dmolik

This comment has been minimized.

Copy link

commented May 10, 2019

👍 good news for Gentoo users!

@oz123

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

It's still not done. But it's definitely a start. Going to continue the work now...

@oz123 oz123 referenced this pull request Aug 14, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.