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

Gather Kuryr specific data #142

Merged
merged 1 commit into from Mar 25, 2020
Merged

Gather Kuryr specific data #142

merged 1 commit into from Mar 25, 2020

Conversation

MaysaMacedo
Copy link
Contributor

This commit ensures that all Kuryr related data
is gathered.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 7, 2020
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 7, 2020
@MaysaMacedo
Copy link
Contributor Author

/test e2e-aws

gryf
gryf approved these changes Feb 13, 2020
View reviewed changes
Copy link
Member

@gryf gryf left a comment

Choose a reason for hiding this comment

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

One small issue with condition.

collection-scripts/gather_network_logs Outdated Show resolved Hide resolved
gryf
gryf suggested changes Feb 13, 2020
View reviewed changes
Copy link
Member

@gryf gryf left a comment •
edited

Choose a reason for hiding this comment

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

Err, that UI. What I've mean is not "approve".

@gryf
Copy link
Member

gryf commented Feb 13, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 13, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 13, 2020
dulek
dulek suggested changes Feb 13, 2020
View reviewed changes
collection-scripts/gather_network_logs Outdated Show resolved Hide resolved
collection-scripts/gather_network_logs Outdated Show resolved Hide resolved
collection-scripts/gather_network_logs Outdated Show resolved Hide resolved
@MaysaMacedo MaysaMacedo force-pushed the gather-kuryr-associated-data branch 3 times, most recently from 72c38be to 3cf56d1 Compare February 24, 2020 17:15
@MaysaMacedo
Copy link
Contributor Author

/test e2e-aws

@luis5tb
Copy link

luis5tb commented Feb 25, 2020

LGTM, just a note about couple of extra information that we may want to grab, router info and quota info

@dulek
Copy link
Contributor

dulek commented Feb 25, 2020

Luis comments makes sense, besides that LGTM.

@luis5tb
Copy link

luis5tb commented Feb 25, 2020

/lgtm

@MaysaMacedo
Copy link
Contributor Author

/hold

Working on removing the need for openstackcli package

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2020
@MaysaMacedo
Copy link
Contributor Author

/hold cancel
OpenStack cli not needed anymore.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2020
@dulek
Copy link
Contributor

dulek commented Mar 16, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2020
pecameron
pecameron reviewed Mar 17, 2020
View reviewed changes
Copy link
Contributor

@pecameron pecameron left a comment

Choose a reason for hiding this comment

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

@MaysaMacedo Looks good. I like the functions. PTAL PR 145. I have made some changes in the base capture. Maybe we could combine these PRs?

@MaysaMacedo
Copy link
Contributor Author

@MaysaMacedo Looks good. I like the functions. PTAL PR 145. I have made some changes in the base capture. Maybe we could combine these PRs?

@pecameron Thanks for taking a look! Sure, we can combine. How would you prefer to combine them?

@pecameron
Copy link
Contributor

@MaysaMacedo I don't have a preference, do you? We could merge one and edit the other. I tested by bringing up ovn and sdn clusters and running the script (PR145) so they gather expected data. I have been looking into how they get used. This is my first time working in this repo, so I am still trying to understand how it all works.

@MaysaMacedo
Copy link
Contributor Author

MaysaMacedo commented Mar 17, 2020 •
edited

@pecameron No preferences either.

@dcbw
Copy link

dcbw commented Mar 21, 2020

@pecameron @MaysaMacedo slight prference for merging this one and then Phil updating his to match. I do like having separate functions for each network plugin type.

@dcbw dcbw mentioned this pull request Mar 24, 2020
Merged
@MaysaMacedo
Copy link
Contributor Author

@sferich888 This PR is ready for another look, as it was checked by folks from other SDN teams.

@MaysaMacedo
Copy link
Contributor Author

/retest

@MaysaMacedo
Gather Kuryr specific data
a31a537 
This commit ensures that all kuryr related data
is gathered.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2020
@dulek
Copy link
Contributor

dulek commented Mar 24, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2020
@MaysaMacedo
Copy link
Contributor Author

/assign @sferich888

sferich888
sferich888 reviewed Mar 25, 2020
View reviewed changes
collection-scripts/gather_network_logs

function gather_kuryr_data {
CONTROLLER_POD=$(oc -n openshift-kuryr get pods --no-headers -o custom-columns=":metadata.name" -l app=kuryr-controller)
oc -n openshift-kuryr exec $CONTROLLER_POD -- bash -c \
Copy link
Contributor

Choose a reason for hiding this comment

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

If your having networking issues is this risky?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review!

Good point. But, to avoid having to rely on the openstackcli being available on the host this was the only alternative. Also, we tried to follow the same pattern other SDNs are doing (executing commands from the network pods).

@sferich888
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek, gryf, luis5tb, MaysaMacedo, sferich888

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2020
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit d867279 into openshift:master Mar 25, 2020
@MaysaMacedo MaysaMacedo deleted the gather-kuryr-associated-data branch March 25, 2020 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dulek dulek dulek requested changes

@sferich888 sferich888 sferich888 left review comments

@luis5tb luis5tb luis5tb left review comments

@pecameron pecameron pecameron left review comments

@gryf gryf gryf approved these changes

@RickJWagner RickJWagner Awaiting requested review from RickJWagner

Assignees

@dulek dulek

@gryf gryf

@sferich888 sferich888

@luis5tb luis5tb

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@MaysaMacedo @gryf @luis5tb @dulek @pecameron @dcbw @sferich888 @openshift-ci-robot @openshift-bot @openshift-merge-robot