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

OSD-19779 Use local monitoring plugin on 4.14+ clusters #292

Conversation

Tafhim
Copy link
Contributor

@Tafhim Tafhim commented Dec 13, 2023

What type of PR is this?

bug

What this PR does / Why we need it?

Since 4.14, the monitoring-plugin is required to be run locally. The plugin is running in cluster as a service so port forwarding is the only way to access it otherwise.
This PR sets up the locally running backplane console to include a locally running monitoring-plugin container in it's network and access it via a nginx server. The serer is pre-set in the monitoring-plugin container so we don't have other ways to access it.

This does not change the behavior for pre-4.14 clusters. It only adds the new container when the cluster is 4.14 and above.

Which Jira/Github issue(s) does this PR fix?

OSD-19779

Special notes for your reviewer

This PR introduces a temporary nginx configuration and a function to run monitoring-plugin in a separate local container.
This change only takes effect on cluster with version 4.14 and above.

With this change:

  • An additional container is run locally for monitoring-plugin
  • The new container is run under the same network infrastructure of the console cotainer
  • Both the console container and the monitoring-plugin container run in daemon mode
  • Both containers use randomly reserved ports to listen for requests
  • monitoring-plugin container uses an inbuilt nginx server that accepts a mounted nginx configuration. For this reason we generate a temporary nginx configuration in backplane configuration directory and mount that to the container.
  • When both containers start up successfully, the terminal waits for an interrupt. Upon receiving the interrupt signal, both containers are stopped and removed.

Pre-checks (if applicable)

  • Ran unit tests locally
  • Validated the changes in a cluster
  • Included documentation changes with PR

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2023
@Tafhim
Copy link
Contributor Author

Tafhim commented Dec 13, 2023

/retest

@Tafhim Tafhim force-pushed the OSD-19779-use-local-monitoring-plugin branch from 395467c to e44df5a Compare December 13, 2023 06:35
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2023

Codecov Report

Merging #292 (c457949) into main (d01e44d) will decrease coverage by 1.68%.
The diff coverage is 4.90%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #292      +/-   ##
==========================================
- Coverage   46.91%   45.24%   -1.68%     
==========================================
  Files          53       53              
  Lines        3568     3720     +152     
==========================================
+ Hits         1674     1683       +9     
- Misses       1633     1769     +136     
- Partials      261      268       +7     
Files Coverage Δ
pkg/cli/config/config.go 59.49% <0.00%> (-4.89%) ⬇️
cmd/ocm-backplane/console/console.go 32.46% <5.09%> (-9.78%) ⬇️

@Tafhim Tafhim force-pushed the OSD-19779-use-local-monitoring-plugin branch 2 times, most recently from 1d7df26 to c8f5353 Compare December 13, 2023 11:29
@Tafhim Tafhim changed the title [WIP] OSD-19779 Use local monitoring plugin on 4.14+ clusters OSD-19779 Use local monitoring plugin on 4.14+ clusters Dec 13, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2023
@Tafhim Tafhim force-pushed the OSD-19779-use-local-monitoring-plugin branch from c8f5353 to 420d739 Compare December 13, 2023 13:01
Copy link
Contributor

@samanthajayasinghe samanthajayasinghe left a comment

Choose a reason for hiding this comment

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

This is a great PR that fixes the console issue by installing a monitoring plugin.
Can you pls add some unit tests to cover new methods?

cmd/ocm-backplane/console/console.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/console/console.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/console/console.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/console/console.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/console/console.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/console/console.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/console/console.go Show resolved Hide resolved
cmd/ocm-backplane/console/console.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/console/console.go Show resolved Hide resolved
cmd/ocm-backplane/console/console.go Show resolved Hide resolved
@Tafhim Tafhim force-pushed the OSD-19779-use-local-monitoring-plugin branch from d1603ef to 1426955 Compare December 14, 2023 05:27
This commit introduces a temporary nginx configuration and a function to run monitoring-plugin in a separate local container.
This change only takes effect on cluster with version 4.14 and above.

With this change:
- An additional container is run locally for monitoring-plugin
- The new container is run under the same network infrastructure of the console cotainer
- Both the console container and the monitoring-plugin container run in daemon mode
- Both containers use randomly reserved ports to listen for requests
- monitoring-plugin container uses an inbuilt nginx server that accepts a mounted nginx configuration. For this reason we generate a temporary nginx configuration in backplane configuration directory and mount that to the container.
- When both containers start up successfully, the terminal waits for an interrupt. Upon receiving the interrupt signal, both containers are stopped and removed.
@Tafhim Tafhim force-pushed the OSD-19779-use-local-monitoring-plugin branch from 1426955 to c457949 Compare December 14, 2023 05:34
Copy link
Contributor

openshift-ci bot commented Dec 14, 2023

@Tafhim: all tests passed!

Full PR test history. Your PR dashboard.

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.

@samanthajayasinghe
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2023
Copy link
Contributor

openshift-ci bot commented Dec 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: samanthajayasinghe, Tafhim

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit db5c0f6 into openshift:main Dec 14, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants