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

openshift/origin-ovn-kubernetes image does not contain kubectl binary #5

Closed
danwinship opened this issue Apr 23, 2019 · 8 comments · Fixed by #6
Closed

openshift/origin-ovn-kubernetes image does not contain kubectl binary #5

danwinship opened this issue Apr 23, 2019 · 8 comments · Fixed by #6
Assignees

Comments

@danwinship
Copy link
Contributor

danwinship commented Apr 23, 2019

As currently built, the openshift/origin-ovn-kubernetes image does not contain kubectl, meaning that even with #4 the node still can't start

@pecameron
Copy link
Contributor

Can we just eliminate the need for kubectl in the image? It is quite large.

@danwinship
Copy link
Contributor Author

Not trivially; ovnkube.sh uses it to find the address of the nbdb and sbdb to pass to ovnkube --init-node.

In theory we could change ovnkube to let us say --nb-service=openshift-ovn-kubernetes/ovn-northd --sb-service=openshift-ovn-kubernetes/ovn-southd instead, and then ovnkube would do the lookup rather than having the wrapper script do it. That might be nice.

@danwinship
Copy link
Contributor Author

In theory we could change ovnkube to let us say --nb-service=openshift-ovn-kubernetes/ovn-northd

actually we don't even need to do that; we can just create kube Services for northd and southd and then add, eg

env:
- name: OVN_NORTH
  value: tcp://ovn-northd.openshift-ovn-kubernetes.svc:6641
- name: OVN_SOUTH
  value: tcp://ovn-southd.openshift-ovn-kubernetes.svc:6642

and it can resolve the service IP via DNS. But that still requires rewriting stuff because currently it always uses kubectl to check the ovnkube-master service to figure out when it can start the ovn node

@squeed
Copy link
Contributor

squeed commented Apr 25, 2019

Maybe, though we often come up before kube-dns, so it's probably better to add native service support directly.

@dcbw
Copy link
Contributor

dcbw commented Apr 25, 2019

@squeed @danwinship isn't this (and some other PRs) diverging from the upstream script? Shouldn't we be copying upstream into our downstream repo first, and anything we need to fix, doing that upstream first and then pulling downstream?

@danwinship
Copy link
Contributor Author

Maybe, though we often come up before kube-dns, so it's probably better to add native service support directly.

ah, indeed. meh

isn't this (and some other PRs) diverging from the upstream script? Shouldn't we be copying upstream into our downstream repo first, and anything we need to fix, doing that upstream first and then pulling downstream?

In my most recent comments I meant to imply making those changes upstream, not here.

For the moment, I am trying to get this repo to a point where we can get ovn-kube to actually come up and work. Once that happens, I'll start upstreaming fixes and rebasing. It doesn't make sense to spend time filing PRs upstream now since we can't even test them yet, and don't actually know if they're going to work.

@pecameron
Copy link
Contributor

Maybe it would be good to merge upstream onto this repo and start with that?

@dcbw
Copy link
Contributor

dcbw commented Apr 25, 2019

@danwinship Right, but there's a pretty big delta between upstream and what's in openshift's downstream repo. IMHO it would be better to copy over what's upstream, and start from there, otherwise you have to do some work twice.

openshift-merge-robot pushed a commit that referenced this issue Mar 29, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Bug 2055378: [release-4.7] support new ingress pipeline option for ACLs
Billy99 added a commit to Billy99/ovn-kubernetes that referenced this issue Nov 2, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Fix Test 3c - Command was inadvertently deleted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants