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

Bug 1421643 - Use existing openshift/origin image instead of new openshift/diagnostics-deployer #12982

Merged
merged 3 commits into from
Feb 23, 2017

Conversation

pravisankar
Copy link

Any new image like 'openshift/diagnostics-deployer' incurs build/lifecycle costs to maintian and
diagnostics-deployer image has only small block of shell code. To alleviate this problem,
now the script is embedded into the pod definition and openshift/origin is used as diagnostics deployer
image. On dev machines, currently openshift/origin is close to 800MB but we expect the size to be under 200MB when it is released (compressed, debug headers removed).

@pravisankar
Copy link
Author

@openshift/networking @sdodson PTAL

@danwinship
Copy link
Contributor

weird that this even works, but lgtm I guess

@dcbw
Copy link
Contributor

dcbw commented Feb 16, 2017

lgtm

@pravisankar
Copy link
Author

@smarterclayton can you please review/merge?

@smarterclayton
Copy link
Contributor

Outside the script looks fine. Lgtm as well [merge] thanks.

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@pravisankar
Copy link
Author

Unrelated errors:
[ERROR] PID 8: hack/build-rpm-release.sh:35: tito tag --undo --offline exited with status 1
fatal: [localhost]: FAILED! => {"failed": true, "msg": "could not locate file in lookup: /usr/share/ansible/openshift-ansible-gce/playbooks/files/gce.json"}

re[test]

@@ -13,7 +13,7 @@ import (
)

const (
diagnosticsImage = "openshift/diagnostics-deployer"
diagnosticsImage = "openshift/origin"
Copy link
Member

Choose a reason for hiding this comment

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

Can this be updated to use the image format string so that we don't have to patch OCP?

Copy link
Author

@pravisankar pravisankar Feb 17, 2017

Choose a reason for hiding this comment

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

I don't see existing image format string for 'openshift/origin', there is openshift/origin-${component}:${version} for system components but that doesn't match with what we need.
Supporting this as config involves creating Network diags section with an option in master-config and this needs to be propagated all the way to GetNetworkDiagnosticsPod().
Is it easy to patch OCP instead?

Copy link
Member

Choose a reason for hiding this comment

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

Is it appropriate to use the image format string and strip the hyphen? That's the image name we'd want 'openshift/origin' and 'openshift3/ose'

Ravi Sankar Penta added 2 commits February 19, 2017 15:13
…shift/diagnostics-deployer

Any new image like 'openshift/diagnostics-deployer' incurs build/lifecycle costs to maintian and
diagnostics-deployer image has only small block of shell code. To alleviate this problem,
now the script is embedded into the pod definition and openshift/origin is used as diagnostics deployer
image. On dev machines, currently openshift/origin is close to 800MB but we expect the size to be under 200MB
when it is released (compressed, debug headers removed).
@pravisankar
Copy link
Author

Added a new commit that makes network diagnostics pod image configurable (default: openshift/origin)
For OCP, user can use openshift3/origin (any image that has shell support)
@sdodson @dcbw @danwinship PTAL

@eparis
Copy link
Member

eparis commented Feb 20, 2017

Looks like this fails on verifying completions. Maybe other things... Please leave a note when you fix the verifications.

@pravisankar
Copy link
Author

Updated auto generated docs/bash completions

@pravisankar
Copy link
Author

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to cdc8ab9

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/364/) (Base Commit: 3f36d2e)

@eparis
Copy link
Member

eparis commented Feb 20, 2017

[merge]

1 similar comment
@eparis
Copy link
Member

eparis commented Feb 21, 2017

[merge]

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 22, 2017 via email

@eparis
Copy link
Member

eparis commented Feb 22, 2017

[merge] is this some sick joke at this point?

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 22, 2017 via email

@eparis
Copy link
Member

eparis commented Feb 22, 2017

[merge] if this fails, i'm just going to hit the big green button on all the PRs that are killing me.

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 22, 2017 via email

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to cdc8ab9

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 23, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/464/) (Base Commit: 2ba2ff0) (Image: devenv-rhel7_5959)

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 23, 2017 via email

@openshift-bot openshift-bot merged commit 1dc7754 into openshift:master Feb 23, 2017
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 this pull request may close these issues.

7 participants