Skip to content

Fix network diagnostic default image path #17315

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
Merged
merged 1 commit into from
Dec 10, 2017
Merged

Fix network diagnostic default image path #17315

merged 1 commit into from
Dec 10, 2017

Conversation

pravisankar
Copy link

  • variable.DefaultImagePrefix is populated from {node,master}-config.yaml
    (imageConfig.format) and if this value is not specified in the config,
    openshift ansible defaults it to 'registry.access.redhat.com'.

  • Network diagnostics is run as a admin CLI command so there is no config for
    variable.DefaultImagePrefix and it defaults to 'registry.access.redhat.com'
    which may not be true for AWS or some other openshift environments.

  • This change will remove the registry path from the default image so that it
    defaults to registry configured on the openshift node.

Example:
Earlier image path: registry.access.redhat.com/openshift3/ose
New image path: openshift3/ose
On AWS node, this will resolve to registry.reg-aws.openshift.com:443/openshift3/ose

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 14, 2017
@pravisankar pravisankar added component/diagnostics and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 14, 2017
@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2017
@pravisankar
Copy link
Author

@openshift/networking @knobunc @sosiouxme PTAL

@stevekuznetsov
Copy link
Contributor

/unassign @stevekuznetsov @liggitt
/assign @knobunc @sosiouxme

@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 28, 2017
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 30, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2017
@pravisankar
Copy link
Author

@knobunc @sosiouxme rebased, PTAL

Looks like DiagnosticPod diagnostics '--images' flag has similar issue: https://bugzilla.redhat.com/show_bug.cgi?id=1475680
I will submit a separate PR for that.

@pravisankar
Copy link
Author

/retest

knobunc
knobunc approved these changes Dec 1, 2017
View reviewed changes
Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

LGTM

Can we get some unit tests of the trim function please?

@sosiouxme
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2017
@knobunc
Copy link
Contributor

knobunc commented Dec 1, 2017 •
edited
Loading

/hold

until we have unit tests

Fix network diagnostic default image path
fe892c5 
- variable.DefaultImagePrefix is populated from {node,master}-config.yaml
(imageConfig.format) and if this value is not specified in the config,
openshift ansible defaults it to 'registry.access.redhat.com'.

- Network diagnostics is run as a admin CLI command so there is no config for
variable.DefaultImagePrefix and it defaults to 'registry.access.redhat.com'
which may not be true for AWS or some other openshift environments.

- This change will remove the registry path from the default image so that it
defaults to registry configured on the openshift node.

Example:
  Earlier image path: registry.access.redhat.com/openshift3/ose
  New image path: openshift3/ose
  On AWS node, this will resolve to registry.reg-aws.openshift.com:443/openshift3/ose
@openshift-ci-robot openshift-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 1, 2017
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 1, 2017
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2017
@pravisankar
Copy link
Author

@knobunc @sosiouxme Added unit test for trimRegistryPath(), PTAL

knobunc
knobunc approved these changes Dec 1, 2017
View reviewed changes
Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

Great, thank you

@knobunc
Copy link
Contributor

knobunc commented Dec 1, 2017

/hold cancel
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2017
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knobunc, pravisankar, sosiouxme

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-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@sosiouxme
Copy link
Member

failure looks like a flake.
/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

9 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit d1ca9a7 into openshift:master Dec 10, 2017
sosiouxme added a commit to sosiouxme/origin that referenced this pull request Jan 24, 2018
@sosiouxme
diagnostics: make network images consistent
dc5fb7f 
Use the default image templates the same as they are used everywhere
else - as visible defaults, and allow the user to provide templates
with elements to be expanded.

This reverts the intent of openshift#17315
with the implication that https://bugzilla.redhat.com/show_bug.cgi?id=1516589
is NOTABUG. We intend the defaults to include registry.access.redhat.com
so that OCP users must explicitly override the image to get an image
that's not shipped by RH. This doesn't affect origin users.
@sosiouxme sosiouxme mentioned this pull request Jan 24, 2018
Closed
sosiouxme added a commit to sosiouxme/origin that referenced this pull request Jan 24, 2018
@sosiouxme
diagnostics: make network images consistent
0a5345d 
Use the default image templates the same as they are used everywhere
else - as visible defaults, and allow the user to provide templates
with elements to be expanded.

This reverts the intent of openshift#17315
with the implication that https://bugzilla.redhat.com/show_bug.cgi?id=1516589
is NOTABUG. We intend the defaults to include registry.access.redhat.com
so that OCP users must explicitly override the image to get an image
that's not shipped by RH. This doesn't affect origin users.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knobunc knobunc knobunc approved these changes

@sosiouxme sosiouxme Awaiting requested review from sosiouxme

Assignees

@sosiouxme sosiouxme

@knobunc knobunc

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

Successfully merging this pull request may close these issues.
8 participants
@pravisankar @stevekuznetsov @sosiouxme @knobunc @openshift-ci-robot @openshift-bot @openshift-merge-robot @liggitt