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

Recover lost broker-info.subm #467

Merged
merged 1 commit into from
Mar 15, 2023
Merged

Recover lost broker-info.subm #467

merged 1 commit into from
Mar 15, 2023

Conversation

Jaanki
Copy link
Contributor

@Jaanki Jaanki commented Dec 23, 2022 •
edited
Loading

Since there are multiple places where a broker installation check on a cluster is needed in multiple places (show brokers, uninstall and recover-brokerinfo (soon)), add broker field as part of the clusterinfo and a function that checks if the cluster has Broker installed.

This simplies code at multiple places.

Depends on: submariner-io/submariner-operator#2432

Signed-off-by: Janki Chhatbar [email protected]

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr467/Jaanki/broker_cluster_info
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@Jaanki Jaanki marked this pull request as draft December 23, 2022 12:12
@Jaanki Jaanki self-assigned this Dec 23, 2022
@Jaanki Jaanki marked this pull request as ready for review December 23, 2022 12:16
skitt
skitt requested changes Dec 23, 2022
View reviewed changes
Copy link
Member

@skitt skitt left a comment

Choose a reason for hiding this comment

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

The PR description mentions that this simplifies code in a number of places, the PR should demonstrate this.

pkg/cluster/info.go Outdated Show resolved Hide resolved
@Jaanki Jaanki mentioned this pull request Dec 26, 2022
Closed
19 tasks
@Jaanki Jaanki marked this pull request as draft December 26, 2022 11:10
@Jaanki Jaanki marked this pull request as ready for review January 4, 2023 14:18
@Jaanki Jaanki requested a review from skitt January 4, 2023 14:29
skitt
skitt requested changes Jan 6, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
pkg/cluster/info.go Outdated Show resolved Hide resolved
pkg/cluster/info.go Outdated Show resolved Hide resolved
tpantelis
tpantelis requested changes Jan 6, 2023
View reviewed changes
internal/show/brokers.go Outdated Show resolved Hide resolved
pkg/cluster/info.go Outdated Show resolved Hide resolved
@Jaanki Jaanki requested review from skitt and tpantelis January 9, 2023 14:10
tpantelis
tpantelis reviewed Jan 9, 2023
View reviewed changes
internal/show/brokers.go Outdated Show resolved Hide resolved
@Jaanki Jaanki requested a review from tpantelis January 10, 2023 11:08
@Jaanki Jaanki marked this pull request as draft January 12, 2023 09:39
@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Feb 21, 2023
@Jaanki Jaanki enabled auto-merge (rebase) February 22, 2023 12:59
mkolesnik
mkolesnik requested changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@mkolesnik mkolesnik left a comment

Choose a reason for hiding this comment

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

Some comments

cmd/subctl/recover_broker_info.go Outdated Show resolved Hide resolved
cmd/subctl/recover_broker_info.go Outdated Show resolved Hide resolved
cmd/subctl/recover_broker_info.go Outdated Show resolved Hide resolved
cmd/subctl/recover_broker_info.go Outdated Show resolved Hide resolved
cmd/subctl/recover_broker_info.go Outdated Show resolved Hide resolved
pkg/cluster/info.go Outdated Show resolved Hide resolved
pkg/broker/recover_broker_info.go
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func RecoverData(
Copy link
Contributor

Choose a reason for hiding this comment

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

You missed the point, if you use a single function and feed it the necessary parameters, should the broker info change then both callers will be either doing the same thing or would both need to be updated.

If you use seperate functions, the changes need to be remembered to be made in both by devs (which i can guarantee wont happen)

I have reused data.writeToFile that broker.WriteInfoToFile also uses to write the collected data to broker-info.subm

That just outputs the info to file, nothing fancy

@Jaanki Jaanki force-pushed the broker_cluster_info branch 3 times, most recently from 6523425 to 89f061e Compare March 3, 2023 11:47
@skitt skitt mentioned this pull request Mar 3, 2023
Merged
@mkolesnik
Copy link
Contributor

The commit message doesn't make much sense right now:

Add recover-brokerinfo command

and add system tests

Epic: https://github.com/submariner-io/enhancements/issues/143

Signed-off-by: Janki Chhatbar <[email protected]>

Get data and populate broker-info.subm

Epic: https://github.com/submariner-io/enhancements/issues/143

Signed-off-by: Janki Chhatbar <[email protected]>

mkolesnik
mkolesnik reviewed Mar 8, 2023
View reviewed changes
cmd/subctl/recover_broker_info.go Outdated Show resolved Hide resolved
cmd/subctl/recover_broker_info.go Outdated Show resolved Hide resolved
@Jaanki
Recover broker-info.subm file
c314237 
This PR does the following:

1. Adds a `subctl recover-broker-info` command to recover
   broker-info.subm file from an existing cluster setup
2. Adds logic to collect necessary information from Broker and
   Submariner clusters to populate broker-info.subm file
3. Add system tests for various scenarios

Epic: submariner-io/enhancements#143

Signed-off-by: Janki Chhatbar <[email protected]>
@Jaanki Jaanki requested a review from mkolesnik March 15, 2023 14:44
mkolesnik
mkolesnik approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@mkolesnik mkolesnik left a comment

Choose a reason for hiding this comment

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

Please make sure to update design and remove the --broker flags

@Jaanki Jaanki merged commit f9c18d3 into submariner-io:devel Mar 15, 2023
@submariner-bot
Copy link
Contributor

🤖 Updating dependent PRs: #588

@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr467/Jaanki/broker_cluster_info]

@maayanf24 maayanf24 added release-note-needed Should be mentioned in the release notes docs-needed labels Mar 27, 2023
@dfarrell07
Copy link
Member

Release notes are here: #630

@dfarrell07 dfarrell07 added release-note-handled and removed release-note-needed Should be mentioned in the release notes labels May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skitt skitt skitt approved these changes

@mkolesnik mkolesnik mkolesnik approved these changes

@tpantelis tpantelis tpantelis approved these changes

@Oats87 Oats87 Awaiting requested review from Oats87 Oats87 is a code owner

@sridhargaddam sridhargaddam Awaiting requested review from sridhargaddam sridhargaddam is a code owner

Assignees

@Jaanki Jaanki

Labels
docs-needed ready-to-test When a PR is ready for full E2E testing release-note-handled
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Jaanki @submariner-bot @tpantelis @mkolesnik @dfarrell07 @skitt @maayanf24