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

Add back origin-sdn-ovs #15468

Merged
merged 1 commit into from
Jul 28, 2017
Merged

Add back origin-sdn-ovs #15468

merged 1 commit into from
Jul 28, 2017

Conversation

andrewklau
Copy link
Contributor

For some reason this was removed. Without this pods created get a cni configuration issue.

@openshift-merge-robot openshift-merge-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 26, 2017
@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 26, 2017
@openshift-ci-robot
Copy link

Hi @andrewklau. Thanks for your PR.

I'm waiting for a openshift 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.

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. I understand the commands that are listed here.

@stevekuznetsov
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 26, 2017
@sdodson
Copy link
Member

sdodson commented Jul 26, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2017
@sdodson
Copy link
Member

sdodson commented Jul 26, 2017

/approve

@stevekuznetsov
Copy link
Contributor

/approve no-issue

@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewklau, sdodson, stevekuznetsov

Associated issue requirement bypassed by: stevekuznetsov

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2017
@andrewklau
Copy link
Contributor Author

/retest

@andrewklau
Copy link
Contributor Author

Not sure why these always fail for me :(

@0xmichalis
Copy link
Contributor

flaked on #11114

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 15468, 15446)

@openshift-merge-robot openshift-merge-robot merged commit 20ea54e into openshift:master Jul 28, 2017
@andrewklau
Copy link
Contributor Author

Should this also be added in the release-3.6 branch?

@0xmichalis
Copy link
Contributor

If you need it in 3.6, then yes.

@andrewklau andrewklau mentioned this pull request Jul 29, 2017
Merged
@andrewklau andrewklau deleted the sdnovs branch July 29, 2017 02:52
openshift-merge-robot added a commit that referenced this pull request Jul 31, 2017
@openshift-merge-robot
Merge pull request #15542 from andrewklau/ovssdn3.6
68c4c59 
Automatic merge from submit-queue

Add back origin-sdn-ovs

From master #15468
@dcbw
Copy link
Contributor

dcbw commented Aug 1, 2017

@adammhaile it was removed in that commit inadvertently, I believe. If @Kargakis meant to remove the SDN components from the node Dockerfiles completely, then he would have also changed the comment he touched to remove mention of "SDN components" too, I would expect:

-LABEL io.openshift.tags="openshift,node" \
-      io.k8s.display-name="OpenShift Origin Node" \
-      io.k8s.description="This is a component of OpenShift Origin and contains the software for individual nodes when using SDN."
+LABEL io.k8s.display-name="OpenShift Origin Node" \
+      io.k8s.description="This is a component of OpenShift Origin and contains the software for individual nodes when using SDN." \
+      io.openshift.tags="openshift,node"

The SDN components are meant to be distributed in the node image; I haven't heard any modification to that policy though I could be wrong.

What specific errors are you seeing in your downstream builds? Can we help fix those?

@dcbw
Copy link
Contributor

dcbw commented Aug 1, 2017 •
edited
Loading

@adammhaile can you enlighten me as to how the Dockerfiles in dist-git (eg, pkgs.devel openshift-enterprise-node-docker/Dockerfile) are synced with the corresponding ones in origin/ose?

For example, openshift-enterprise-dockerregistry-docker/Dockerfile contains INSTALL_PKGS="atomic-openshift-dockerregistry" && \ while in origin that corresponding line is images/dockerregistry/Dockerfile RUN INSTALL_PKGS="origin-dockerregistry" && \. What scripts do the change from 'origin->atomic-openshift', and if so, would they need to be updated to do origin-sdn-ovs -> atomic-openshift-sdn-ovs too? Or do they handle it automatically?

Feel free to email too if there's too much internal detail that would need to be communicated.

@stevekuznetsov
Copy link
Contributor

There is not really any relationship between dist-git and ose ... yet. Lots of work to try to get them to be identical but it seems to have stalled.

@stevekuznetsov
Copy link
Contributor

@smunilla and @jupierce will know more

@adammhaile
Copy link
Contributor

adammhaile commented Aug 1, 2017 via email

@dcbw
Copy link
Contributor

dcbw commented Aug 1, 2017

@adammhaile so it looks like what happened was that Origin containers got broken by 2f7d826#diff-b5321710e6dadcda38c38e45d57c6337L17 in March, but OCP got fixed up by @tdawson in 942aa9a9 to dist-git's openshift-enterprise-node-docker repo. @andrewklau just finally fixed Origin, but that was then automatically synced over to openshift-enterprise-node-docker which broke the builds, and then required manual fixup.

But it looks like everything is OK now? v3.6.173.0.1 1" of the openshift-enterprise-node-docker Dockerfile has the atomic-openshift-sdn-ovs RPM in the INSTALL_PKGS line, and does not have the offending "origin-sdn-ovs" package that Origin needs instead. Can you confirm?

@adammhaile
Copy link
Contributor

adammhaile commented Aug 1, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sdodson sdodson

@smarterclayton smarterclayton

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@andrewklau @openshift-ci-robot @stevekuznetsov @sdodson @openshift-merge-robot @0xmichalis @dcbw @adammhaile @smarterclayton